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

ci: remove vendored yarn, and use volta for Travis + GHA #19069

Merged
merged 30 commits into from May 29, 2020

Conversation

joshuarli
Copy link
Member

@joshuarli joshuarli commented May 28, 2020

We're pretty happy with Volta (cc @charlespierce), and it obviates the need for vendoring yarn.

I will note that the caching key is merely the presence of volta on the PATH, but I don't envision us changing yarn or node versions often, and when we do, we just have to bust travis cache manually.

getsentry: https://github.com/getsentry/getsentry/pull/3953

@joshuarli joshuarli changed the title ci: WIP remove vendored yarn, and use volta ci: remove vendored yarn, and use volta May 28, 2020
@joshuarli joshuarli marked this pull request as ready for review May 28, 2020 22:21
@joshuarli
Copy link
Member Author

Requesting changes for the one blocker I labeled. Also,

I will note that the caching key is merely the presence of volta on the PATH, but I don't envision us changing yarn or node versions often, and when we do, we just have to bust travis cache manually.

This is a no go as one would 100% forget invalidating the cache for a rare thing like this and wouldn't even notice it most of the time. That said I couldn't figure out exactly what cache/cache-key you were referring to from the code so I cannot offer alternatives.

It's command -v volta && exit 0.

Also, sorry, I don't envision us changing yarn or node versions often is inaccurate. I'm gonna separate

  node -v
  yarn -v
  yarn install --frozen-lockfile

So that we always attempt to install new node, yarn, and node dependencies. It'll only be volta that isn't redownloaded every time.

Copy link
Contributor

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Looks pretty solid with the updates. Will accept once the blocker is resolved.

Great job @joshuarli, and thank you for doing this 🙂

.travis.yml Outdated Show resolved Hide resolved
Copy link

@charlespierce charlespierce left a comment

Choose a reason for hiding this comment

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

A couple of comments / clarifications, as someone with more context in Volta. Happy to find places where our documentation isn't as clear and we can improve (I know using in CI in general is a hole in our docs).

.travis.yml Outdated
Comment on lines 21 to 25
# Xenial's libssl-dev provides an openssl 1.0 SO.
# Until we can upgrade the linux distribution, we're stuck here.
# Volta also ships binaries linked against openssl 1.1, but then we'd have to build it ourselves.
# There's also volta-cli/volta/pull/754 which builds volta with rustls to avoid this.
- libssl-dev

Choose a reason for hiding this comment

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

Forgive my ignorance of the CI environment, but does Xenial not ship OpenSSL at all? And if it doesn't, does it not provide a non-dev package to install, instead of the development one? For running Volta, you should just need the .so files, which I would expect to be installed by a non-dev package.

(For some reason I feel like I've had a similar discussion with @mitsuhiko before, but I can't find it in email now ☹️ )

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, sorry, that's libssl.so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, volta (master) links against libssl.so.1.1 => /usr/lib/x86_64-linux-gnu/libssl.so.1.1 (0x00007fa56d07f000)

Choose a reason for hiding this comment

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

There's a bit of nuance there: Building Volta from source on Linux will link against whatever version of OpenSSL is available on the machine doing the building (detected by the openssl-sys crate).

However, for the production builds, we compile and package two separate binaries, one linked against OpenSSL 1.0.x and the other against 1.1.x. So whichever version is available on the machine, we should have a binary that is compatible with it.

Copy link
Member Author

@joshuarli joshuarli May 29, 2020

Choose a reason for hiding this comment

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

TIL x2! Thanks a lot, that looks a lot better. libssl-dev was confusing to me, too.

Choose a reason for hiding this comment

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

FWIW, it looks like Xenial should have the libssl1.0.0 package by default: http://releases.ubuntu.com/xenial/ubuntu-16.04.6-desktop-amd64.manifest lists all the packages that are included in the base image.

Copy link
Member Author

@joshuarli joshuarli May 29, 2020

Choose a reason for hiding this comment

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

Yep, I'm testing a commit without any openssl packages, since it could have just been the case that 1.0.0 SO was there already. I didn't test that scenario yet.

Also that is the desktop image, I doubt travis runs that, but also TIL that those manifests are a thing. Not really familiar with Ubuntu.

Copy link
Member Author

Choose a reason for hiding this comment

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

Lol Travis simply isn't triggering. Good stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, it seems to be working without libssl1.0.0, presumably because Travis xenial has it. Thanks for the TILs!

.travis.yml Show resolved Hide resolved
Copy link
Contributor

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Unblocking as @joshuarli discovered gawk 😀

Joshua Li added 2 commits May 29, 2020 15:11
@joshuarli joshuarli merged commit cb4be04 into master May 29, 2020
@joshuarli joshuarli deleted the ci/remove-vendored-yarn-and-use-volta branch May 29, 2020 22:45
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants