-
Notifications
You must be signed in to change notification settings - Fork 313
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
Add some dependencies for uploading artifacts #538
Conversation
1bd793f
to
91027e4
Compare
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.
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?
@@ -14,7 +14,7 @@ npm run removeNPMAbsolutePaths | |||
# Check that repo is still clean | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>&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?
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.
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.
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.
Oh...that sucks. It's not even possible to explicitly add the fsevents
dependency in the package.json?
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.
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"})
943e7b3
to
c33f830
Compare
Aha, good point. I actually did have an up to date |
c33f830
to
5b7c3a4
Compare
@@ -14,7 +14,7 @@ npm run removeNPMAbsolutePaths | |||
# Check that repo is still clean | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want clarification that this command only works on macos. This is surprising because the workflows themselves successfully run npm ci && npm run removeNPMAbsolutePaths
.
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.
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.
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.
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.
@@ -14,7 +14,7 @@ npm run removeNPMAbsolutePaths | |||
# Check that repo is still clean | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
5b7c3a4
to
9094b62
Compare
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