Skip to content

Make automodel and flow queries work without submodule.#2731

Merged
starcke merged 3 commits intomainfrom
starcke/no-submodule
Aug 29, 2023
Merged

Make automodel and flow queries work without submodule.#2731
starcke merged 3 commits intomainfrom
starcke/no-submodule

Conversation

@starcke
Copy link
Copy Markdown
Contributor

@starcke starcke commented Aug 21, 2023

Resolve automodel and flow queries without submodule.

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.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@starcke starcke force-pushed the starcke/no-submodule branch 4 times, most recently from 4505966 to c605cc9 Compare August 25, 2023 11:25
@starcke starcke changed the base branch from refactor-data-ext-queries to main August 25, 2023 11:28
@starcke starcke closed this Aug 25, 2023
@starcke starcke reopened this Aug 25, 2023
@starcke starcke force-pushed the starcke/no-submodule branch from c605cc9 to d2eb366 Compare August 25, 2023 12:58
@starcke starcke marked this pull request as ready for review August 25, 2023 14:19
@starcke starcke requested review from a team as code owners August 25, 2023 14:19
@starcke starcke force-pushed the starcke/no-submodule branch from 7311059 to 4b1e320 Compare August 28, 2023 08:46
@starcke
Copy link
Copy Markdown
Contributor Author

starcke commented Aug 28, 2023

Unfortunately it seems I had some issues with this:

  • It turned out that it only worked because I had some pre-downloaded dependencies in my CodeQL cache. Therefore we need to add an explicit pack download to make it works from clean slate.
  • The createLockFileForStandardQuery was actually not as benign as we originally thought. It turns out it actually puts a lock file inside the standard package directory (e.g. /home/starcke/.codeql/packages/codeql/java-all). What is worse is if there is a dependency problem then it might put a partial lock file in there effectively corrupting the package (with no automatic recovery).

I believe I have fixed both of these problems:

  • We explicitly download the needed pack (queries in addition to all).
  • We only call createLockFileForStandardQuery in the fetch case, as that needs it to resolve the query.

I think the solution to the second problem is not the most elegant one. I think the best would probably be to stop using createLockFileForStandardQuery completely as that is dangerous to have around. One direction we could take here is to actually move the fetch queries to the codeql repo (which we also have other reasons for doing).

@starcke starcke requested a review from charisk August 28, 2023 08:53
@starcke starcke force-pushed the starcke/no-submodule branch from 4b1e320 to 259159d Compare August 28, 2023 11:22
@aeisenberg
Copy link
Copy Markdown
Contributor

So, this is creating a lock file in the downloaded library pack? It's generally something we should be avoiding since downloaded packs should be read-only. It looks like you have a cleanup job running that deletes the lock file, so that's a bit better. One concern I have is that you're likely to get different lock files over time as we release new transitive dependencies.

Moving the queries to their own query pack would be a better approach, in my view.

@starcke
Copy link
Copy Markdown
Contributor Author

starcke commented Aug 29, 2023

So, this is creating a lock file in the downloaded library pack? It's generally something we should be avoiding since downloaded packs should be read-only. It looks like you have a cleanup job running that deletes the lock file, so that's a bit better. One concern I have is that you're likely to get different lock files over time as we release new transitive dependencies.

It was doing that for the automodel queries (after I integrated them), and that was actually what corrupted my pack directory (as per our discussion on slack) because it failed half way through creating a broken lock file. I have now removed that lock creation from the automodel and flow queries. It is now only used by the fetch queries which need it (and here the lock creation happens in a temp dir), but longer term I would prefer to remove the creation of the lock completely as it feels very risky to use.

Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Looks good and works on my machine.

Comment thread extensions/ql-vscode/src/data-extensions-editor/external-api-usage-queries.ts Outdated
…sage-queries.ts

Co-authored-by: Charis Kyriakou <charisk@users.noreply.github.com>
@starcke starcke merged commit 14b9f93 into main Aug 29, 2023
@starcke starcke deleted the starcke/no-submodule branch August 29, 2023 10:45
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