Skip to content
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

Fix upgrades path #1138

Merged
merged 1 commit into from
Feb 11, 2022
Merged

Fix upgrades path #1138

merged 1 commit into from
Feb 11, 2022

Conversation

aeisenberg
Copy link
Contributor

Ensure that upgrades can be resolved even when the upgrades pack is not
in the workspace. This is the situation when the core libraries are
resolved from the package cache.

This change works because qlProgram.libraryPath is the resolved
search path for compiling the query. We are guaranteed that the
appropriate core libraries are included in this query.

Note that this change avoids using extra source folders from the
workspace. Previously without using packages, we assume that all
relevant query paths are already inside the workspace. With
packaging, this is no longer the case.

It is theoretically possible that there will be extra upgrade scripts
that are not on the resolved search path, but are included in the
workspace. This situation would have worked in the past.This is not a
situation that we expect to happen in practice. And if this does happen,
I believe this is an error and all upgrades should be added explicitly
to the search path.

An open question is if this will work with downgrade scripts. If it does
not, then I don't think this change makes things any worse than before.

Replace this with a description of the changes your pull request makes.

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.

@aeisenberg aeisenberg requested a review from cklin February 11, 2022 19:12
@aeisenberg aeisenberg requested a review from a team as a code owner February 11, 2022 19:12
@cklin
Copy link
Contributor

cklin commented Feb 11, 2022

It looks like the test database might need to be rebuilt for the new javascript dbscheme?

@aeisenberg
Copy link
Contributor Author

The failure is when running against an old version of the CLI. It's passing on newer versions. I'm going to see if I can reproduce locally.

@aeisenberg
Copy link
Contributor Author

Ah ha....the problem is that older codeql versions (pre-packaging) did not have the upgrades packs on the library path of the src packs. There was no dependency chain.

What this means is that for older CLIs, we need to use the old mechanism and put all workspace folders onto the search path.

It may necessary to combine the workspace folders with the resolved path, but I'd rather not go there unless I have to.

Ensure that upgrades can be resolved even when the upgrades pack is not
in the workspace. This is the situation when the core libraries are
resolved from the package cache.

This change works because `qlProgram.libraryPath` is the resolved
search path for compiling the query. We are guaranteed that the
appropriate core libraries are included in this query.

Note that this change avoids using extra source folders from the
workspace. Previously without using packages, we assume that all
relevant query paths are already inside the workspace. With
packaging, this is no longer the case.

It is theoretically possible that there will be extra upgrade scripts
that are not on the resolved search path, but are included in the
workspace. This situation would have worked in the past.This is not a
situation that we expect to happen in practice. And if this does happen,
I believe this is an error and all upgrades should be added explicitly
to the search path.

An open question is if this will work with downgrade scripts. If it does
not, then I don't think this change makes things any worse than before.
@aeisenberg aeisenberg force-pushed the aeisenberg/upgrades-path branch from 4a00ab3 to 8b5622e Compare February 11, 2022 20:13
@aeisenberg aeisenberg merged commit a2b5ad0 into main Feb 11, 2022
@aeisenberg aeisenberg deleted the aeisenberg/upgrades-path branch February 11, 2022 20:58
@aeisenberg aeisenberg mentioned this pull request Feb 11, 2022
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.

2 participants