Skip to content

Fix MRVA integration test failure#3263

Merged
dbartol merged 4 commits intomainfrom
dbartol/extensible-failure
Jan 22, 2024
Merged

Fix MRVA integration test failure#3263
dbartol merged 4 commits intomainfrom
dbartol/extensible-failure

Conversation

@dbartol
Copy link
Copy Markdown
Contributor

@dbartol dbartol commented Jan 22, 2024

Newer CLI versions preserve the metadata for extensible predicates in the compiled pack, so they no longer preserve queries that contain extensible predicates when creating a MRVA pack.

Fixes https://github.com/github/semmle-code/issues/48686

@dbartol dbartol force-pushed the dbartol/extensible-failure branch from 1febe44 to d7f01dc Compare January 22, 2024 16:52
@dbartol dbartol added the Complexity: Low A good task for newcomers to learn, or experienced team members to complete quickly. label Jan 22, 2024
@dbartol dbartol marked this pull request as ready for review January 22, 2024 16:53
@dbartol dbartol requested review from a team as code owners January 22, 2024 16:53
@dbartol dbartol requested a review from cklin January 22, 2024 16:54
public static CLI_VERSION_WITH_TRIM_CACHE = new SemVer("2.15.1");

public static CLI_VERSION_WITH_EXTENSIBLE_PREDICATE_METADATA_IN_QLX =
new SemVer("2.16.1");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This constant is a bit misleading:

  • Extensible predicate metadata is stored in a created qlpack (in .packinfo files), not in QLX
  • The CLI version that starts generating extensible predicate metadata is 2.16.0, not 2.16.1

Perhaps the constant name we want is something like CLI_VERSION_WITHOUT_MRVA_EXTENSIBLE_PREDICATE_HACK?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've renamed the constant and the function to better reflect the details, plus added a few comments.

Comment thread extensions/ql-vscode/src/codeql-cli/cli.ts Fixed
Copy link
Copy Markdown
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.

Any problem merging this now so we can fix our integration tests?

@dbartol dbartol merged commit 445aa81 into main Jan 22, 2024
@dbartol dbartol deleted the dbartol/extensible-failure branch January 22, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complexity: Low A good task for newcomers to learn, or experienced team members to complete quickly.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants