Skip to content

Conversation

@robertbrignull
Copy link
Contributor

@robertbrignull robertbrignull commented Aug 18, 2020

Adds https://github.com/vercel/pkg which can be used to create a standalone CLI binary. This seems to work and I was able to run the linux version it produces on a system without nodejs installed. It also produces binaries for other operating systems which I haven't yet tested but I have no reason to believe they won't work.

The only downside is the size of the executables, but I'm not sure there's anything we can do about this:

> ls -lh cli
total 208M
-rw-rw-r-- 1 robert robert 798K Aug 18 11:12 code-scanning-cli.js
-rwxrwxr-x 1 robert robert  69M Aug 18 11:12 code-scanning-cli-linux
-rwxrwxr-x 1 robert robert  70M Aug 18 11:12 code-scanning-cli-macos
-rw-rw-r-- 1 robert robert  70M Aug 18 11:12 code-scanning-cli-win.exe

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@aeisenberg
Copy link
Contributor

Since this commit adds megabytes of code and data to the repository, I wonder if there is a better way. My assumption is that either users will check out this repository and build the cli manually, or there will be a workflow that will rebuild the cli for each release and publish the artifact somewhere. Either way, since the pkg package is not needed for running the action, maybe it doesn't need to be checked into the repository.

I can think of two possibilities:

  1. Mark the pkg package as optional and use .gitignore filters to ensure it doesn't get checked in. And then create the proper scripts and workflows to ensure it is built properly as required.
  2. Create a sub-package inside of the main package that includes everything you need to build the cli.

I think 2 would be better since maintaining the gitignore would be complex.

Though, I may be missing some context here and my comment isn't relevant. So, please take that into consideration.

@robertbrignull
Copy link
Contributor Author

Worth noting the /cli directory where the built artefacts get placed is already .gitignore'd so don't need to worry about those. For the pkg package I agree it would be a shame to have those checked in to the repository. The same could be said of everything under devDependencies. Even though it doesn't actually make a difference right now we've been pretty good at putting stuff in dependencies or devDependencies as appropriate. Maybe we could avoid checking in everything from there.

I've not used sub-packages before. Do you have a link to any documentation? Or otherwise feel free to open a PR to do this if that's easier.

@aeisenberg
Copy link
Contributor

All I mean by subpackage is just creating a sub-directory maybe called packaging and placing a separate package.json there. You can put any packaging-related dependencies there and you just need to add the parent directory as a dependency. I think this would work and help avoid committing all of this.

@robertbrignull
Copy link
Contributor Author

That worked out really nicely. I've created a packaging directory with another package.json file that has only the deps needed to build the CLI. Turns out I didn't have to add the other package.json as a dependency and I could move some other existing deps over there as well.

One commit in this PR updates the node_modules directory and thus is huge, so I recommend reviewing commit-by-commit and skipping that one.

@robertbrignull
Copy link
Contributor Author

@aeisenberg are you ok if I assign you to review formally?

@aeisenberg
Copy link
Contributor

Glad it worked out. 😄 Taking a look now.

@aeisenberg
Copy link
Contributor

Congratulations on winning the prize for having the single most productive PR I have ever seen. 🤣

@robertbrignull
Copy link
Contributor Author

Congratulations on winning the prize for having the single most productive PR I have ever seen

Thanks!

I only just spotted the CI is failing. I think I've just misconfigured it as I've had it working on this branch today. I don't think it's a major problem.

"private": true,
"description": "CodeQL action - CLI packaging",
"scripts": {
"build-cli": "webpack --mode production && pkg dist/code-scanning-cli.js --out-path dist"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this script here, I think you also need to change the cli.yml workflow to point here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I hadn't updated cli.yml. I think there was also a problem because I left the build-cli script definition in the top-level package.json. Anyway I've fixed both these points now.


module.exports = {
entry: './src/cli.ts',
entry: '../src/cli.ts',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this cli.ts file isn't related to the actual action code, perhaps it should be moved to the packaging directory. Not necessary, but I think it would help enforce a distinction between the action code and the cli code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this. Having all the source together is also useful. I'd like to at least defer it. If we did this I'd like to also split the actions code out from the shared code as I think that would get us more benefit, but that will be much more difficult.

@aeisenberg
Copy link
Contributor

Also, it looks like this commit only adds the upload command to the cli. Are you planning on adding the remaining commands as well?

@robertbrignull
Copy link
Contributor Author

Also, it looks like this commit only adds the upload command to the cli. Are you planning on adding the remaining commands as well?

Yes, the CLI is only partially implemented. The other parts will be coming this week.

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Seems to be working for me.

@robertbrignull robertbrignull merged commit fe75660 into main Aug 25, 2020
@robertbrignull robertbrignull deleted the vercel/pkg branch August 25, 2020 16:42
@github-actions github-actions bot mentioned this pull request Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants