Skip to content

Add some dependencies for uploading artifacts #538

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.

Sign up for GitHub

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

Merged
edoardopirovano merged 1 commit into from
Jun 2, 2021
Merged

Add some dependencies for uploading artifacts #538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/script/check-node-modules.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Copy link
Contributor

@aeisenberg aeisenberg Jun 1, 2021

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.

Copy link
Contributor Author

@edoardopirovano edoardopirovano Jun 1, 2021 •
edited
Loading

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

> [email protected] 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

> [email protected] 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.

Copy link
Contributor

@aeisenberg aeisenberg Jun 1, 2021

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.

git status
exit 1
fi
Expand Down
4 changes: 2 additions & 2 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Please note that this project is released with a [Contributor Code of Conduct][c

## Development and Testing

Before you start, ensure that you have a recent version of node installed. You can see which version of node is used by the action in `init/action.yml`.
Before you start, ensure that you have a recent version of node (14 or higher) installed, along with a recent version of npm (7 or higher). You can see which version of node is used by the action in `init/action.yml`.

### Common tasks

Expand All @@ -28,7 +28,7 @@ You may want to run `tsc --watch` from the command line or inside of vscode in o

Because CodeQL Action users consume the code directly from this repository, and there can be no build step during an GitHub Actions run, this repository contains all compiled artifacts and node modules. There is a PR check that will fail if any of the compiled artifacts are not up to date. Compiled artifacts are stored in the `lib/` directory. For all day-to-day development purposes, this folder can be ignored.

Only run `npm install` if you are explicitly changing the set of dependencies in `package.json`. The `node_modules` directory should be up to date when you check out, but if for some reason, there is an inconsistency use `npm ci && npm run removeNPMAbsolutePaths` to ensure the directory is in a state consistent with the `package-lock.json`. There is a PR check to ensure the consistency of the `node_modules` directory.
Only run `npm install` if you are explicitly changing the set of dependencies in `package.json`. The `node_modules` directory should be up to date when you check out, but if for some reason, there is an inconsistency use `npm ci && npm run removeNPMAbsolutePaths` to ensure the directory is in a state consistent with the `package-lock.json`. Note that due to a macOS-specific dependency, this command should be run on a macOS machine. There is a PR check to ensure the consistency of the `node_modules` directory.

### Running the action

Expand Down
Loading