fix: Generate shrinkwraps for the bundled vscode #5071
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5071 +/- ##
=======================================
Coverage 72.44% 72.44%
=======================================
Files 30 30
Lines 1673 1673
Branches 366 366
=======================================
Hits 1212 1212
Misses 398 398
Partials 63 63 Continue to review full report at Codecov.
|
|
So the failure seems to be because of some dependencies missing in the |
|
GitHub seems to have ignored my email response so I will paste it below, apologies if it does eventually come in and the comment gets duplicated: I love the detailed description!
My first thought when I mentioned this was that the user might have Buuuut...is it likely someone has Node installed but not npm? I know The second thought I had was that using the same package manager |
0323abb to
41fc135
Compare
41fc135 to
44c8865
Compare
|
Ok new tentative out, with:
(1) and (2) together (should) also help the other issue I was trying to track down, which happens because the tests were failing for the standalone release as it wasn't installing some of the devDependencies that we were removing too early from the shrinkwrap file. |
|
FWIW, I'm playing around in edvincent#1 (comment) to be able to run the CI/CD and pinpoint some of the culprits. To avoid the flood and needing approvals for the workflows to run. Sorry it's taking so long, but trying to do it the best possible way this time 💪 |
d551cc1 to
fe6a577
Compare
|
Ok, I think I got it this time! Moved to generate the shrinkwraps in the Also updated the overview of this PR to cover some of the reasoning. Thoughts? I think I still need to add something so that the NPM publish CI step calls this release:standalone too? |
|
Awesome work! I love your comments; very thorough and made it super easy to understand the rationale.
I think leaving it in |
|
Ohh I see in your PR description you are doing it there because it runs after |
|
Yep.. We need the The other solution would be to still do it in Unless we do it in Thoughts? |
|
Maybe we could just add |
Oh right we have no Node modules at all at that stage, we would need to remove that whole |
This is exactly why I have avoided doing that todo so far lol |
|
Today there is never a The only EDIT: Yep, what you just said now. Happy to try and take a stab at that if it seems to be the best. |
Ah yup it is done in a previous step and then this step has an option ( |
|
Yeah I say we go ahead with this plan. |
|
Damn it's actually a bit more nuanced than just removing the The other solution would be to come back to the original idea of generating the shrinkwrap files from the code-server repo itself, and copy them into release (after removing the devDependencies from it). i.e. we'd need to cd into BTW - Right now, that |
Yup, from what I recall initially we never kept the modules then later
I think it will be OK to let these get packaged into the tar although I suppose they will just end up getting overwritten later. We could create a new var |
|
Er wait I might have misunderstood. We do already run code-server/ci/steps/publish-npm.sh Line 86 in c35bf13 |
fe6a577 to
0934e07
Compare
|
No, I haven't forgotten about this/you folks ❤️ life got in the way... I'm back trying to fix this, sent a new approach PR (which gets a bit easier thanks to NodeJS 16!). Still a WIP and needs some tweaks, just sharing for awareness. Testing in https://github.com/edvincent/code-server/actions/runs/2883629793 HOWEVER, something that is a bit of a concern to me now seems to be a bug in NPM itself... Despite the shrinkwrap file, installing with the |
|
FYI - opened npm/cli#5325 to track that nasty bug about the shrinkwrap file not being respected. I'll still continue with the PR here (need to clean-up some of the code/options, I used Any thoughts on the approach proposed in the latest code? |
ci/build/build-standalone-release.sh
Outdated
| popd | ||
| } | ||
|
|
||
| create_shrinkwraps() { |
There was a problem hiding this comment.
The build-standalone-release.sh script takes the npm package generated in a previous step, installs deps, then packages the result so we can upload a standalone build complete with deps to GitHub. Assuming I have that right (sometimes I get confused with all the steps) generating a shrinkwrap here would not get uploaded to npm.
So I think we will have to do this in build-release.sh which is where we create and upload the npm package artifact that eventually gets uploaded to npm.
And then here we would install with npm instead of yarn.
There was a problem hiding this comment.
Ah damn yes, now I think I remember being bugged/blocked by something like that in the past... Gonna double-check that now!
There was a problem hiding this comment.
And then here we would install with
npminstead ofyarn.
Can I suggest to avoid doing too many things in the same PR if that works for you:
- (This PR) Just adding the
npm-shrinkwrap.jsonfile - which basically should not change anything else in the development/CI workflow. - Another PR to change the
release:standaloneand other CI scripts to start usingnpm ci - Another PR (when everything works) to amend the doc to recommend
npm(which can be done last, as using yarn will still always work).
There was a problem hiding this comment.
I like the incremental approach!
583dd8b to
b8dfaeb
Compare
|
Ok... This is what it looks like when doing it from the release script... Which is indeed where it's needed, as the tgz we use to publish are the artifacts from it - not from release standalone. Confirmed from the artifacts the shrinkwrap files and their content from downloading the build artifacts. It looks a bit worse because shrinkwrapping tries to alter the |
b8dfaeb to
ed8ae80
Compare
ed8ae80 to
5c0cfdc
Compare
|
Everything looks good to me so I am going to merge it in. Thanks a million! |
|
@edvincent really can't thank you enough - it's rare someone from the community takes on a hard problem, takes a break because life, then comes back to finish it 💪🏼 We appreciate you a ton! |
|
Anytime! I like giving back to the community too, especially for stuff I use daily! Plus I got to learn a LOT about NPM's dependency management, which I enjoyed. And thank to both of you for bearing with me during the (long) process. You can also thank @bpmct and the nice t-shirt he sent me couple of months ago, it definitely played a role in reminding me I still had this PR pending every time I wore it. 👀 Will still send a follow-up PR (likely next week) for the documentation to remind folks to use |
|
Also, whilst confirming the fix of #5174 (comment), realized that the bug npm/cli#5325 is actually less of an issue here - because it doesn't seem to be buggy when installing it from a remote repo! Which means... There now is a way to install |
|
Awesome news!! |
|
I love the detailed description!
What is used to install is completely meaningless to the end-user. If
only, by not using yarn even when invoked with yarn, we can guarantee
the deterministic versions for the bundled vscode. Thoughts?
My first thought when I mentioned this was that the user might have
`yarn` installed but not `npm` so switching the package manager from
under the user could cause installation failures (`npm: command not
found` or something like that).
Buuuut...is it likely someone has Node installed but not npm? I know
some distros package them separately but it still seems unlikely.
The second thought I had was that using the same package manager
throughout the entire installation process /feels/ more correct to me.
I doubt it would cause issues in practice but it seems like it would be
less of a surprise ("Wait why is it calling npm when I ran yarn?").
|
|
Ignore this lol GitHub seems to finally be posting comments I made via email months ago |
Fixes #5056 #5174
Because it only takes care of putting in the package-lock/shrinkwrap file the first level of dependencies, not the nested dependencies which we have from the yarn lockfile.
See the contents of https://unpkg.com/browse/code-server@4.2.0-5047-90c9adcded65d10004a9db71d0717cd90565baa3/npm-shrinkwrap.json, and how for example,
@mapbox/node-pre-gypis listed as a dependency ofargon2- but is not present anywhere in the shrinkwrap file.What initially drove me to use
synpwas the fact that thenpm shrinkwrapcommand would generate a lockfile that wasn't usable. Trying to install the package with that lockfile would fail with the following exception:This was actually due to
npm shrinkwrap(in NPM 6) choking with some packages not being anywhere in the lockfile but being present in thepackage.json.So I simply tracked down their dependency tree, saw they were coming from devDependencies, and added them there - which now prevents the
npm shrinkwrap-generated lockfile from breaking, and allows us to removesynp. This doesn't change the size of the dependency tree, and should be able to be removed when there are higher versions of NPM used.NPM 6's
npm cidoesn't deal very well with optional dependencies, which the bundled vscode needs for some Windows dependencies (namely,windows-process-treeand@vscode/windows-registry):npm shrinkwrapcommand doesn't add them to the lockfile, because the CI runs from Linux, so it doesn't install them - and the commands generates the lockfile based on what's innode_modules."optional": true), the install on a Linux machine would fail - because NPM 6'snpm cidoesn't respect them from annpm-shrinkwrap.jsonfile.npm ci, they are not installed - even on Windows machines.What I settled for was using
npm install --production(for now), that will always respect the lockfile version if an entry is there, and will try to get versions not listed there. This effectively means that those two packages, on windows, won't be following theyarn.lockthat is being used in the development package, but that seemed less worse.Note: Another idea was to hackily add the packages to the lockfile + enhance the
postinstall.shscript to conditionally use--no-optionalif we're running on a non-Windows machine - which skips those optional dependencies. I wasn't convinced by either the hacking around of the lockfile nor this conditional... But happy to hear thoughts?Because we are now generating the shrinkwrap file after stuff has been installed with
yarn --production, which meansnpm shrinkwrapwill only list the production dependencies.But also, because doing it with
jsongets a bit tricky, asjsonwould need to be listed as an actual dependency (rather thandevDependency) of code-server and vscode too to be able to be used... Not worth it.Switching the
jqcommands tojsonmight still be worth it, but no need to conflate the commits.