Codecov Report

Merging #5071 (b1382c8) into main (33ee184) will not change coverage.
The diff coverage is n/a.

❗ Current head b1382c8 differs from pull request most recent head 5c0cfdc. Consider uploading reports for the commit 5c0cfdc to get more accurate results

Impacted file tree graph

@@ 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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33ee184...5c0cfdc. Read the comment docs.

So the failure seems to be because of some dependencies missing in the node_modules from the original install... Which my guess is because something is failing to be installed. Will be playing around with it (expect a bunch of updates to this PR to "test" the CI) and try to find the culprit.

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!

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?").

Ok new tentative out, with:

1. Using npm or yarn, depending on what was initially used.
2. Generating the shrinkwraps when generating the release, rather than initially.
3. Removing @parcel/node-addon-api from the shrinkwrap, as it's a folder in node_modules which is actually not a package and was making the whole process choke.

(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 💪

Ok, I think I got it this time! Moved to generate the shrinkwraps in the release:standalone command, which basically allows not having to know which package.json is being copied etc...

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 I still need to add something so that the NPM publish CI step calls this release:standalone too?

I think leaving it in build-release.sh is the right move. We use the result of that script in both the NPM publish step and standalone step.

Ohh I see in your PR description you are doing it there because it runs after yarn --production. Hmm...

Yep.. We need the node_modules to be populated to be able to generate the shrinkwrap file...

The other solution would be to still do it in build-release.sh, which means we'd need to yarn install in build-release.sh. That would address this TODO https://github.com/coder/code-server/blob/main/.github/workflows/ci.yaml#L240-L242 - and the NPM release would move to be a yarn pack (rather than a raw tar command). But I'm not sure how would having the node_modules folder from one arch would affect another arch - for the cross-compile sections...

Unless we do it in build-release.sh, with a yarn install, but then generate two type of artifacts - the current one (for the other platforms) and the current one...

Thoughts?

Maybe we could just add yarn --production to build-release.sh and it would not take too much longer since it only needs to remove files (but I have no idea how yarn works internally so maybe this takes longer for some reason).

We need the node_modules to be populated

Oh right we have no Node modules at all at that stage, we would need to remove that whole KEEP_MODULES behavior and just keep the modules (in addition to then running yarn --production at the end if I understand correctly).

But I'm not sure how would having the node_modules folder from one arch would affect another arch

This is exactly why I have avoided doing that todo so far lol

Today there is never a yarn in build-release.sh - so a yarn --production won't be removing files, but also installing stuff that was never installed.

The only yarn that gets done is at the root of the repo, not inside the release folder no?

EDIT: Yep, what you just said now. Happy to try and take a stab at that if it seems to be the best.

Today there is never a yarn in build-release.sh

Ah yup it is done in a previous step and then this step has an option (KEEP_MODULES) for whether to also copy the Node modules or skip them.

Yeah I say we go ahead with this plan.

Damn it's actually a bit more nuanced than just removing the KEEP_MODULES behavior... If we run yarn --production, it would also install some of the things we were excluding in the rsync like the node executable, the node_modules.ara and the lib/coder-cloud-agent. Which means the tar -czf command in the ci.yaml would need to replicate/hard-code some of those excludes...

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 lib/vscode/remote/ - rather than relying on the fact we copied the right package.json.

BTW - Right now, that KEEP_MODULES never seems used?

KEEP_MODULES never seems used

Yup, from what I recall initially we never kept the modules then later KEEP_MODULES was added to make building locally faster (so you would not have to yarn twice if you wanted to test a full build locally). So current the var is only ever used during local development.

it would also install some of the things we were excluding

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 KEEP_BINARIES or something which acts like the old var except we are keeping Node modules by default now?

Er wait I might have misunderstood. We do already run yarn --production so those things exist in the standalone already but now the npm package will include them when it did not before maybe? In that case maybe we add to the npmignore.