Remote Queries: Create packs for remote queries #985

aeisenberg · 2021-10-27T23:28:43Z

Remote Queries: Create packs for remote queries

This PR handles two cases:

There is a qlpack.yml or codeql-pack.yml file in the same directory
as the query to run remotely. In this case, run codeql pack packlist to determine what files to include (and also always include
the lock file and the query itself. Copy to a temp folder and run
pack install, then pack bundle. Finally upload.
There is no qlpack in the current directory. Just copy the single
file to the temp folder and generate a synthetic qlpack before
installing, bundling and uploading.

Two cases that are not handled:

The query file is part of a workspace. Peer dependencies will not be
found.
The query file and its qlpack file are not in the same directory.

These case should be possible to handle later.

This PR also includes integration tests that build the pack and verify that its components are what we expect.

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
@github/docs-content-codeql has been cc'd in all issues for UI or other user-facing changes made by this pull request.

This is still a bit rough, but handles two cases: 1. There is a qlpack.yml or codeql-pack.yml file in the same directory as the query to run remotely. In this case, run `codeql pack packlist` to determine what files to include (and also always include the lock file and the query itself. Copy to a temp folder and run `pack install`, then `pack bundle`. Finally upload. 2. There is no qlpack in the current directory. Just copy the single file to the temp folder and generate a synthetic qlpack before installing, bundling and uploading. Two cases that are not handled: 1. The query file is part of a workspace. Peer dependencies will not be found. 2. The query file and its qlpack file are not in the same directory. These should be possible to handle later. Also, need to create some unit and integration tests for this.

shati-patel

Thanks! A partial review so far: one of the steps doesn't work for me on Windows 😬

extensions/ql-vscode/src/run-remote-query.ts

extensions/ql-vscode/src/extension.ts

aeisenberg · 2021-10-28T15:16:40Z

Failing tests are now because the remote-queries commands will not work on older versions of the CLI. I will need to disable for < 2.6.3 (or maybe even 2.7.0).

Also: - Fix the count of copied files - A few typos - Ensure the correct settings are applied for remote queries before running tests.

Help understand why tests are failing.

rneatherway · 2021-11-01T17:11:23Z

There is a qlpack.yml or codeql-pack.yml file in the same directory
as the query to run remotely.

How would we lift this restriction in the end? Search upwards for an enclosing directory containing a qlpack.yml file?

extensions/ql-vscode/src/run-remote-query.ts

rneatherway · 2021-11-01T17:12:55Z

extensions/ql-vscode/src/run-remote-query.ts


-  // If the user has an explicit `.repositories` file, use that.


Does this PR remove support for .repositories entirely? Seems like a good change if so.

extensions/ql-vscode/src/run-remote-query.ts

rneatherway · 2021-11-01T17:28:32Z

extensions/ql-vscode/src/run-remote-query.ts

+      name: 'codeql-remote/query',
+      version: '1.0.0',
+      dependencies: {
+        [`codeql/${language}-all`]: '*',


This part interests me. I take it this is the equivalent of https://github.com/dsp-testing/qc-run2/blob/ff207ffe5d100a9eb55743bc01f032336abdcde7/src/codeql.ts#L46? How does this behave in the following cases:

I've made a qlpack that doesn't depend on the core libraries (perhaps it is language independent). Odd case, I know.

I have a qlpack that contains just a query and the qlpack yml file. I've modified one of the core libraries, for example to add a new sink.

I have a qlpack that adds some new library files and also uses the core libraries.

To be clear, this situation only exists when there is no qlpack associated with this query. So...

As long as there is a qlpack.yml that can be resolved and correctly declares itself to be no-lang, it should be fine.

I don't think we can (or should?) support this. The codeql pack create command compiles the current pack and also grabs all of its dependencies from the package cache, which get copied into the pack. Users are not supposed to manually edit these dependencies (it would be like manually editing your node_modules). If this is something that a user feels they need to do, then that's a missing feature we need to work on.

This is already handled by codeql pack create.

2. I don't think we can (or should?) support this. The codeql pack create command compiles the current pack and also grabs all of its dependencies from the package cache, which get copied into the pack. Users are not supposed to manually edit these dependencies (it would be like manually editing your node_modules). If this is something that a user feels they need to do, then that's a missing feature we need to work on.

That's fine for this PR but I think we are missing an important use-case where a CodeQL developer is working on a new or updated query that also requires changes to the core language libraries. I'll add it to the agenda for discussion.

Hmmm...yes, something to discuss since I don't think we should be encouraging users to change the core libraries. Maybe there's a use case I'm missing.

But, even so, when we run the pack create command, all paths in the vscode workspace are added to the --additional-packs path, so if the core libraries are in the workspace, they should be used instead of whatever is published in the package registry (I haven't tried this out yet, so not sure).

rneatherway · 2021-11-01T17:30:26Z

extensions/ql-vscode/src/run-remote-query.ts

+      message: 'Determining query target language'
+    });
+
+    // If the user has an explicit `.repositories` file, use that.


Ah, we do still have support for .repositories. I think that would be completely fine to remove at this point, but perhaps we should discuss in our next meeting.

I think that's worth talking about. @shati-patel may have some opinions on this.

rneatherway · 2021-11-01T17:47:26Z

extensions/ql-vscode/src/run-remote-query.ts

+  cliServer: cli.CodeQLCliServer,
+  credentials: Credentials,
+  uri: Uri | undefined,
+  dryRun: boolean,


As an alternative to an explicit option like this we have also used stubbing (e.g. https://github.com/github/codeql-action/blob/main/src/database-upload.test.ts), or you could pass in a mocked Octokit.

For the integration test I think it would be quite reasonable to call the API endpoint and run a query in its entirety. If it's against a one or a small number of repositories the cost is extremely small. If the results issue spam is annoying then you could use another repository as the controller.

All up to you.

I've been finding it very useful to have it as an explicit parameter. This is mostly because I've been testing locally and I don't want to actually upload anything most of the time.

I was even toying with adding a codeql.remote.dryRun undocumented setting, but then I changed my mind.

For now, when I try uploading anything, even though the initial request is successful, the workflow run on the controller repo fails. Once this feature is stable, it may make sense to remove the dry-run flag altogether and actually upload something to run.

Once this feature is stable, it may make sense to remove the dry-run flag altogether and actually upload something to run.

Hopefully it should all work now as we shipped the server-side changes yesterday.

shati-patel · 2021-11-01T17:52:37Z

I didn't spot this in my earlier review, but I've just tried with a quick query, and it doesn't seem to work for me 🤔 I'm getting: A fatal error occurred: Could not resolve library path ...

Possibly because the qlpack.yml for my quick query uses the old format:

# This is an automatically generated file.

name: quick-query
version: 1.0.0
libraryPathDependencies:
  - codeql-go

Do quick queries work for you?

aeisenberg · 2021-11-01T21:30:56Z

There is a qlpack.yml or codeql-pack.yml file in the same directory
as the query to run remotely.

How would we lift this restriction in the end? Search upwards for an enclosing directory containing a qlpack.yml file?

Yes. I don't think it will be hard to do, but I just want to focus on the simple parts first.

aeisenberg · 2021-11-01T21:52:13Z

Do quick queries work for you?

Hmm...let me check this...

Actually, it's for a trivial reason. The qlpack.yml file is missing a scope and pack create won't work with scopeless packages. I can check for this and make sure to add a synthetic scope before trying to upload.

When we generate the synthetic pack, just ensure that there is a valid name.

rneatherway · 2021-11-02T10:08:06Z

My comments are now limited to two discussion points for a future meeting, so I'm happy once @shati-patel is!

shati-patel · 2021-11-02T13:40:54Z

Thanks 🌞 PR looks good, apart from quick queries 👇🏽

Do quick queries work for you?

Hmm...let me check this...

Actually, it's for a trivial reason. The qlpack.yml file is missing a scope and pack create won't work with scopeless packages. I can check for this and make sure to add a synthetic scope before trying to upload.

Quick queries still aren't working for me (though possibly for a different reason now 🙃):

[2021-11-02 13:24:55] Plumbing command codeql pack resolve-dependencies completed:
                      { }
[2021-11-02 13:24:55] [PROGRESS] pack install> Dependencies resolved. Installing packages...
...
[2021-11-02 13:24:55] [PROGRESS] pack install> Nothing to install.

Can't seems to resolve any dependencies or install any packages... Which then causes other stuff to fail further down the line (like [ERROR] resolve library-path> ERROR: Referenced pack 'codeql-go' not found.)

aeisenberg · 2021-11-02T17:14:37Z

@shati-patel, it's working for me out of the box, but if I remove the codeql-go submodule from the workspace, then I get the error you mention. Is this what you are seeing?

aeisenberg · 2021-11-02T20:52:47Z

With some help from @shati-patel, I worked out what the issue was. It's a bug in the CLI and I pushed up a private PR to fix. Basically, old style package name references like codeql-go (instead of codeql/go-all) were not being translated into a correct path. The fix will go out with v2.7.1 of the CLI and I don't think it should block this PR since they are released independently.

shati-patel

Thanks for fixing that! All good now with the updated CLI 🎉

aeisenberg · 2021-11-03T15:49:04Z

Thanks for the review and trying this out!

Send a query pack

b2a6263

aeisenberg requested a review from a team as a code owner October 27, 2021 23:28

aeisenberg requested review from shati-patel, rneatherway and robertbrignull October 27, 2021 23:29

aeisenberg force-pushed the qc-packs branch from 2a8e233 to 42051f1 Compare October 28, 2021 04:09

shati-patel reviewed Oct 28, 2021

View reviewed changes

aeisenberg force-pushed the qc-packs branch from 2d01c33 to a8a4247 Compare October 28, 2021 22:04

Avoid running remote queries on v2.6.3 cli or earlier

903f5db

Also: - Fix the count of copied files - A few typos - Ensure the correct settings are applied for remote queries before running tests.

aeisenberg force-pushed the qc-packs branch from a8a4247 to 903f5db Compare October 28, 2021 23:09

Allow custom setting of codeql cli for integration tessts

48468ff

aeisenberg force-pushed the qc-packs branch 2 times, most recently from 21aebcd to 15b0d6e Compare October 29, 2021 16:08

Add more debugging logic

440044d

Help understand why tests are failing.

aeisenberg force-pushed the qc-packs branch from 15b0d6e to 440044d Compare October 29, 2021 16:18

Fix Windows path issue

56faf36

rneatherway reviewed Nov 1, 2021

View reviewed changes

Change to 0.0.0 as synthetic version

28c76be

Ensure anonymous and scope-less packs can be used as remote queries

4a65b6a

When we generate the synthetic pack, just ensure that there is a valid name.

aeisenberg force-pushed the qc-packs branch from 869b224 to 4a65b6a Compare November 1, 2021 22:18

shati-patel approved these changes Nov 3, 2021

View reviewed changes

aeisenberg merged commit e9574d3 into main Nov 3, 2021

aeisenberg deleted the qc-packs branch November 3, 2021 15:49


		// If the user has an explicit `.repositories` file, use that.

Remote Queries: Create packs for remote queries #985

Remote Queries: Create packs for remote queries #985

Uh oh!

Conversation

aeisenberg commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

shati-patel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aeisenberg commented Oct 28, 2021

Uh oh!

rneatherway commented Nov 1, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shati-patel commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aeisenberg commented Nov 1, 2021

Uh oh!

aeisenberg commented Nov 1, 2021

Uh oh!

rneatherway commented Nov 2, 2021

Uh oh!

shati-patel commented Nov 2, 2021

Uh oh!

aeisenberg commented Nov 2, 2021

Uh oh!

aeisenberg commented Nov 2, 2021

Uh oh!

shati-patel left a comment

Choose a reason for hiding this comment

Uh oh!

aeisenberg commented Nov 3, 2021

Uh oh!

Uh oh!

aeisenberg commented Oct 27, 2021 •

edited

Loading

shati-patel commented Nov 1, 2021 •

edited

Loading