ci: remove vendored yarn, and use volta for Travis + GHA #19069
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,6 @@ | ||
| exclude: > | ||
| (?x)( | ||
| LICENSE$| | ||
| \.snap$| | ||
| \.map$| | ||
| \.map\.js$| | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,5 @@ | ||
| PIP := python -m pip --disable-pip-version-check | ||
| WEBPACK := NODE_ENV=production yarn webpack | ||
|
|
||
| # Currently, this is only required to install black via pre-commit. | ||
| REQUIRED_PY3_VERSION := $(shell awk 'FNR == 2' .python-version) | ||
|
|
@@ -80,17 +79,17 @@ setup-git: ensure-venv setup-git-config | |
| @echo "" | ||
|
|
||
| node-version-check: | ||
| @test "$$(node -v)" = v10.16.3 || (echo 'node version does not match 10.16.3. Recommended to use https://github.com/volta-cli/volta'; exit 1) | ||
|
There was a problem hiding this comment. Blocker: I think this test should not exist or if it does, we should be reading this value from There was a problem hiding this comment. Yeah, unfortunately, I wouldn't want to have people install My opinion is that it's not worth the effort, given that we rarely change node versions. When we do, it'd be easy to change this value. (It'd error pretty quickly in CI anyways.) There was a problem hiding this comment.
Yes, I'd be okay with a shitty grep. I'll even save you from the hassle: awk 'BEGIN { RS=",|: {\n"; FS="\""; } $2 == "node" { print $4 }' package.json
Trust me, when we need to change the node version, and we will soon as they are releasing a new security update soon, this will become a problem. Even worse, 6 months later, when someone who is not you (or even you) wants to update this, it will again be a big headache. There was a problem hiding this comment. I just don't see why it would be a "big headache" to update one thing that is even caught by CI. Thanks for the awk, I'll put it in. Just to clarify, I don't have a problem with doing this at all, I'm just interested in why you're so averse to this. There was a problem hiding this comment. Oh, your awk doesn't work. There was a problem hiding this comment.
It works on my machine 😛 Joking aside, I think this is one of those shitty BSD/GNU/whatever differences as I tested this on Ubuntu and guessing you are trying this on macOS. There was a problem hiding this comment.
For anyone reading this who is not me or Josh, we discussed this over Slack. Summary: because it is not obvious or easy to find/remember that this needs be changed until it fails. There was a problem hiding this comment. For posterity it's GNU awk vs BSD awk. I can come up with a fix. There was a problem hiding this comment. Oh, comments just refreshed. |
||
|
|
||
| install-js-dev: node-version-check | ||
| @echo "--> Installing Yarn packages (for development)" | ||
| # Use NODE_ENV=development so that yarn installs both dependencies + devDependencies | ||
| NODE_ENV=development yarn install --frozen-lockfile | ||
| # A common problem is with node packages not existing in `node_modules` even though `yarn install` | ||
| # says everything is up to date. Even though `yarn install` is run already, it doesn't take into | ||
| # account the state of the current filesystem (it only checks .yarn-integrity). | ||
| # Add an additional check against `node_modules` | ||
| yarn check --verify-tree || yarn install --check-files | ||
|
|
||
| install-py-dev: ensure-pinned-pip | ||
| @echo "--> Installing Sentry (for development)" | ||
|
|
@@ -154,24 +153,24 @@ test-cli: | |
|
|
||
| test-js-build: node-version-check | ||
| @echo "--> Running type check" | ||
| @yarn run tsc | ||
| @echo "--> Building static assets" | ||
| @$(WEBPACK) --profile --json > .artifacts/webpack-stats.json | ||
|
|
||
| test-js: node-version-check | ||
| @echo "--> Running JavaScript tests" | ||
| @yarn run test | ||
| @echo "" | ||
|
|
||
| test-js-ci: node-version-check | ||
| @echo "--> Running CI JavaScript tests" | ||
| @yarn run test-ci | ||
| @echo "" | ||
|
|
||
| # builds and creates percy snapshots | ||
| test-styleguide: | ||
| @echo "--> Building and snapshotting styleguide" | ||
| @yarn run snapshot | ||
| @echo "" | ||
|
|
||
| test-python: | ||
|
|
||
There was a problem hiding this comment.
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
.sofiles, 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☹️ )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Xenial's
libssl-devprovides the SO. https://packages.ubuntu.com/xenial/amd64/libssl-dev/filelistThere was a problem hiding this comment.
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.There was a problem hiding this comment.
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)There was a problem hiding this comment.
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-syscrate).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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.0package 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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!