Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use travis_apt_get_update and invoke apt-get update manually #3142

Closed

Conversation

codablock
Copy link

Something goes wrong in very rare cases when using travis_apt_get_update,
but no debug output is printed. We could use "travis_apt_get_update debug"
but I prefer to do the call manually as it makes more clear what's
happening. Also, travis_retry with travis_apt_get_update causes the "rm -rf"
to be re-executed for each try, making us start from the beginning again
and again which also increases the chance of multiple failures in a row.

Something goes wrong in very rare cases when using travis_apt_get_update,
but no debug output is printed. We could use "travis_apt_get_update debug"
but I prefer to do the call manually as it makes more clear what's
happening. Also, travis_retry with travis_apt_get_update causes the "rm -rf"
to be re-executed for each try, making us start from the beginning again
and again which also increases the chance of multiple failures in a row.
@codablock codablock added this to the 14.1 milestone Oct 10, 2019
@codablock
Copy link
Author

FYI, this is meant to fix or at least debug build failures as seen in https://travis-ci.org/dashpay/dash/jobs/595696047.

@UdjinM6
Copy link

UdjinM6 commented Oct 10, 2019

Looks good. The actual failure is due to missing gpg keys which can be fixed by smth like UdjinM6@4168c93 (if I get bash syntax right 🙈 )

@codablock
Copy link
Author

@UdjinM6 Not sure I like the commit. It allows every MITM to swap keys and our build would happily accept any modified package ;)

@codablock
Copy link
Author

I also feel it's more likely the "Hash Sum mismatch" error seen in the build logs. The GPG error is probably ignored later as we never try to install something from the repo with the missing key

@codablock
Copy link
Author

It looks like Travis got support for Ubuntu Bionic in the meantime, which has a newer APT version which also fixes the "Hash Sum mismatch" bug. Maybe the much better solution is to upgrade our .travis.yml to use Bionic instead. I'll create a another PR to test this out.

@@ -114,7 +114,8 @@ before_cache:
- docker save dash-builder-$BUILD_TARGET-$TRAVIS_JOB_NUMBER $(docker history -q dash-builder-$BUILD_TARGET-$TRAVIS_JOB_NUMBER | grep -v \<missing\>) | gzip -2 > $HOME/cache/docker/dash-builder-$BUILD_TARGET.tar.gz

before_install:
- travis_retry travis_apt_get_update
- sudo rm -rf "${TRAVIS_ROOT}/var/lib/apt/lists/"*
- travis_retry sudo sudo apt-get update 2>&1
- travis_retry sudo apt-get -yq --no-install-suggests --no-install-recommends install docker-ce realpath
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we remove this entirely if all we are installing is docker-ce and we have the services: docker set up above?

(Not sure what realpath is used for.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

realpath must be installed in Trusty as it's not available in the base install. In Bionic it became part of coreutils. I'm investigating an upgade to Bionic atm where I already had to remove the realpath package.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks. That was actually a tangent of my original question tho -- we shouldn't need to do this whole apt-get update / install thing at all since we have the docker service setup, right? Meaning all this would be unnecessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correct, I added this to make sure we're on a recent docker version. With the Bionic upgrade, that probably won't be needed as well.

Comment on lines 116 to 120
before_install:
- travis_retry travis_apt_get_update
- sudo rm -rf "${TRAVIS_ROOT}/var/lib/apt/lists/"*
- travis_retry sudo sudo apt-get update 2>&1
- travis_retry sudo apt-get -yq --no-install-suggests --no-install-recommends install docker-ce realpath

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion is to remove this entirely and rename the PR.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I can't figure out how to use the GitHub "suggestions" feature.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need, this whole PR will probably get closed as I'm working on #3143 now

@codablock
Copy link
Author

Closing in favor of #3143

@codablock codablock closed this Oct 14, 2019
@UdjinM6 UdjinM6 removed this from the 15 milestone Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants