Add some dependencies for uploading artifacts - #538
edoardopirovano merged 1 commit into
Conversation
Sorry, something went wrong.
aeisenberg
left a comment
There was a problem hiding this comment.
What version of npm are you using? I see that you downgraded the lock file from v2 to v1. And that has changed just about every file. Also, not sure why all of the lib/*.js files were changed, too. Maybe this implicitly changed the version of typescript?
So, can you recreate this change using node v14?
Also, if it's not already in the dev notes or contributing guide, could you add a mention that node >=14 is required?
Sorry, something went wrong.
| if [ ! -z "$(git status --porcelain)" ]; then | ||
| # If we get a fail here then the PR needs attention | ||
| >&2 echo "Failed: node_modules are not up to date. Run 'npm ci' and 'npm run removeNPMAbsolutePaths' to update" | ||
| >&2 echo "Failed: node_modules are not up to date. Run 'npm ci' and 'npm run removeNPMAbsolutePaths' on a Mac OS X machine to update" |
There was a problem hiding this comment.
| >&2 echo "Failed: node_modules are not up to date. Run 'npm ci' and 'npm run removeNPMAbsolutePaths' on a Mac OS X machine to update" | |
| >&2 echo "Failed: node_modules are not up to date. Run 'npm ci && npm run removeNPMAbsolutePaths' on a Mac OS X machine to update" |
Does this need to be on a macos X machine? Technically, BigSur is not X But more importantly, what if you're on linux or windows?s?
Sorry, something went wrong.
There was a problem hiding this comment.
Unfortunately, if the command is run on Linux or Windows rather than macOS, the dependency fsevents (which is Mac-specific) is not installed causing test failues (and likely also issues in production on Mac machines). I agree we should have a way of updating dependencies not on macOS but I'm not entirely sure how we can actually resolve the issue. npm isn't really designed for our workflow of checking in dependencies, so as far as I can tell there's no way to make it install a dependency that isn't compatible with the OS you're on.
Sorry, something went wrong.
There was a problem hiding this comment.
Oh...that sucks. It's not even possible to explicitly add the fsevents dependency in the package.json?
Sorry, something went wrong.
There was a problem hiding this comment.
Sadly not, if we explicitly add the dependency in package.json then doing npm install on Linux doesn't even work:
npm ERR! notsup Unsupported platform for fsevents@2.1.2: wanted {"os":"darwin"} (current: {"os":"linux","arch":"x64"})
Sorry, something went wrong.
|
Aha, good point. I actually did have an up to date node but had an outdated npm which was using the old lockfile format. I've updated that and sure enough the changes are somewhat more manageable now. I've updated the docs to say we want node 14+ and npm 7+. |
Sorry, something went wrong.
| if [ ! -z "$(git status --porcelain)" ]; then | ||
| # If we get a fail here then the PR needs attention | ||
| >&2 echo "Failed: node_modules are not up to date. Run 'npm ci' and 'npm run removeNPMAbsolutePaths' to update" | ||
| >&2 echo "Failed: node_modules are not up to date. Run 'npm ci && npm run removeNPMAbsolutePaths' on a macOS machine to update. Note it is important this command is run on macOS and not any other operating system as there is one dependency (fsevents) that is needed for macOS and may not be installed if the command is run on a Windows or Linux machine." |
There was a problem hiding this comment.
Just want clarification that this command only works on macos. This is surprising because the workflows themselves successfully run npm ci && npm run removeNPMAbsolutePaths.
Sorry, something went wrong.
There was a problem hiding this comment.
Locally for me (Ubuntu 20.04, node 14.17.0 and npm 7.15.1), running npm ci && npm run removeNPMAbsolutePaths removes the fsevents package:
edoardo@EdoardoDesktop ~/codeql-action db-upload-deps ± git reset --hard origin/db-upload-deps HEAD is now at 5b7c3a46 Add some dependencies for uploading artifacts edoardo@EdoardoDesktop ~/codeql-action db-upload-deps npm ci && npm run removeNPMAbsolutePaths added 564 packages, and audited 565 packages in 8s 56 packages are looking for funding run `npm fund` for details found 0 vulnerabilities > codeql@1.0.0 removeNPMAbsolutePaths > removeNPMAbsolutePaths . --force edoardo@EdoardoDesktop ~/codeql-action db-upload-deps ± git status On branch db-upload-deps Your branch is up-to-date with 'origin/db-upload-deps'. Changes not staged for commit: (use "git add/rm <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory) modified: node_modules/.package-lock.json deleted: node_modules/fsevents/LICENSE deleted: node_modules/fsevents/README.md deleted: node_modules/fsevents/fsevents.d.ts deleted: node_modules/fsevents/fsevents.js deleted: node_modules/fsevents/fsevents.node deleted: node_modules/fsevents/package.json no changes added to commit (use "git add" and/or "git commit -a")
I agree this is strange, because the PR check that runs the same step remotely also runs on Ubuntu 20.04 and doesn't fail. I even tried invoking the PR check script locally and this still fails:
edoardo@EdoardoDesktop ~/codeql-action db-upload-deps ± git reset --hard origin/db-upload-deps HEAD is now at 5b7c3a46 Add some dependencies for uploading artifacts edoardo@EdoardoDesktop ~/codeql-action db-upload-deps .github/workflows/script/check-node-modules.sh added 564 packages, and audited 565 packages in 8s 56 packages are looking for funding run `npm fund` for details found 0 vulnerabilities > codeql@1.0.0 removeNPMAbsolutePaths > removeNPMAbsolutePaths . --force Failed: node_modules are not up to date. Run 'npm ci && npm run removeNPMAbsolutePaths' on a macOS machine to update. Note it is important this command is run on macOS and not any other operating system as there is one dependency (fsevents) that is needed for macOS and may not be installed if the command is run on a Windows or Linux machine. On branch db-upload-deps Your branch is up-to-date with 'origin/db-upload-deps'. Changes not staged for commit: (use "git add/rm <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory) modified: node_modules/.package-lock.json deleted: node_modules/fsevents/LICENSE deleted: node_modules/fsevents/README.md deleted: node_modules/fsevents/fsevents.d.ts deleted: node_modules/fsevents/fsevents.js deleted: node_modules/fsevents/fsevents.node deleted: node_modules/fsevents/package.json no changes added to commit (use "git add" and/or "git commit -a") ✘ edoardo@EdoardoDesktop ~/codeql-action db-upload-deps ±
I'm completely stumped on why the PR check would work but locally it does not. I still think advising to only update dependencies on Mac OS is reasonable, since if the command is done there it always seems to work.
Sorry, something went wrong.
There was a problem hiding this comment.
Hmmm...tried this on my windows vm and I'm seeing the same behaviour. I think we can punt this for now. It's documented at least. Thanks for digging into this.
Sorry, something went wrong.
| if [ ! -z "$(git status --porcelain)" ]; then | ||
| # If we get a fail here then the PR needs attention | ||
| >&2 echo "Failed: node_modules are not up to date. Run 'npm ci' and 'npm run removeNPMAbsolutePaths' to update" | ||
| >&2 echo "Failed: node_modules are not up to date. Run 'npm ci && npm run removeNPMAbsolutePaths' on a macOS machine to update. Note it is important this command is run on macOS and not any other operating system as there is one dependency (fsevents) that is needed for macOS and may not be installed if the command is run on a Windows or Linux machine." |
There was a problem hiding this comment.
Hmmm...tried this on my windows vm and I'm seeing the same behaviour. I think we can punt this for now. It's documented at least. Thanks for digging into this.
Sorry, something went wrong.
This PR adds two dependencies that will be used for future steps that upload artifacts. In particular, the two dependencies added are:
The first is needed to upload artifacts. The second isn't strictly needed but makes it much easier to select whole folders with many files (such as CodeQL databases) for upload.
This PR also does an update of our dependencies since it seems reasonable to do this while we're touching the package.json.
Merge / deployment checklist