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.

Sign up for GitHub

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

Remove support for CodeQL CLI versions older than v2.16.6 #3728

Merged
merged 3 commits into from
Sep 25, 2024
Merged

Remove support for CodeQL CLI versions older than v2.16.6 #3728

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
515132f
Remove support for CodeQL CLI versions older than v2.16.6
norascheuch Sep 25, 2024
12e06b7
Fix lint
norascheuch Sep 25, 2024
181e28c
Fix test
norascheuch Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 1 addition & 27 deletions extensions/ql-vscode/src/codeql-cli/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1909,38 +1909,12 @@ function shouldDebugCliServer() {
export class CliVersionConstraint {
// The oldest version of the CLI that we support. This is used to determine
// whether to show a warning about the CLI being too old on startup.
public static OLDEST_SUPPORTED_CLI_VERSION = new SemVer("v2.15.5");

public static CLI_VERSION_WITHOUT_MRVA_EXTENSIBLE_PREDICATE_HACK = new SemVer(
"2.16.1",
);

/**
* CLI version where there is support for multiple queries on the pack create command.
*/
public static CLI_VERSION_WITH_MULTI_QUERY_PACK_CREATE = new SemVer("2.16.1");
public static OLDEST_SUPPORTED_CLI_VERSION = new SemVer("2.16.6");
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 a bit confused about the version number syntax. We use version numbers with v in the supported_cli_versions.json, but here we have historically used no v. In the code I removed we used SemVer("2.16.1"), so without the v. Which one is correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://www.npmjs.com/package/semver says "A leading "=" or "v" character is stripped off and ignored." so either is fine. Personally I'd go with no "v" 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.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

To explain more...

From a behaviour point of view I don't think it makes any difference. From a code readability point of view it makes sense to remove unnecessary boilerplate. From a consistency point of view I suppose you might want them to match the json file, but I think any human who reads them would easily correlate the numbers and not get confused.


constructor(private readonly cli: CodeQLCliServer) {
/**/
}

private async isVersionAtLeast(v: SemVer) {
return (await this.cli.getVersion()).compare(v) >= 0;
}

async preservesExtensiblePredicatesInMrvaPack() {
// Negated, because we _stopped_ preserving these in 2.16.1.
return !(await this.isVersionAtLeast(
CliVersionConstraint.CLI_VERSION_WITHOUT_MRVA_EXTENSIBLE_PREDICATE_HACK,
));
}

async supportsPackCreateWithMultipleQueries() {
return this.isVersionAtLeast(
CliVersionConstraint.CLI_VERSION_WITH_MULTI_QUERY_PACK_CREATE,
);
}

async supportsMrvaPackCreate(): Promise<boolean> {
return (await this.cli.getFeatures()).mrvaPackCreate === true;
}
Expand Down
9 changes: 0 additions & 9 deletions extensions/ql-vscode/src/variant-analysis/run-remote-query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,15 +116,6 @@ async function generateQueryPack(

let precompilationOpts: string[];
if (cliSupportsMrvaPackCreate) {
if (
qlPackDetails.queryFiles.length > 1 &&
!(await cliServer.cliConstraints.supportsPackCreateWithMultipleQueries())
) {
throw new Error(
`Installed CLI version does not allow creating a MRVA pack with multiple queries`,
);
}

const queryOpts = qlPackDetails.queryFiles.flatMap((q) => [
"--query",
join(targetPackPath, relative(qlPackDetails.qlPackRootPath, q)),
Expand Down
1 change: 0 additions & 1 deletion extensions/ql-vscode/supported_cli_versions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,5 @@
"v2.18.4",
"v2.17.6",
"v2.16.6",
"v2.15.5",
"nightly"
]
16 changes: 1 addition & 15 deletions ...scode/test/vscode-tests/cli-integration/variant-analysis/variant-analysis-manager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,13 +347,6 @@ describe("Variant Analysis Manager", () => {
const queryToRun =
"Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql";

// Recent versions of the CLI don't preserve queries with extensible predicates in MRVA packs,
// because all the necessary info is in the `.packinfo` file.
const extraQueries =
(await cli.cliConstraints.preservesExtensiblePredicatesInMrvaPack())
? ["Telemetry/ExtractorInformation.ql"]
: [];

const qlPackRootPath = join(process.env.TEST_CODEQL_PATH, "java/ql/src");
const queryPath = join(qlPackRootPath, queryToRun);
const qlPackFilePath = join(qlPackRootPath, "qlpack.yml");
Expand All @@ -362,7 +355,7 @@ describe("Variant Analysis Manager", () => {
qlPackRootPath,
qlPackFilePath,
expectedPackName: "codeql/java-queries",
filesThatExist: [queryToRun, ...extraQueries],
filesThatExist: [queryToRun, ...["Telemetry/ExtractorInformation.ql"]],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
filesThatExist: [queryToRun, ...["Telemetry/ExtractorInformation.ql"]],
filesThatExist: [queryToRun],

We actually don't want to check for this file anymore.

Recent versions of the CLI don't preserve queries with extensible predicates in MRVA packs,

The preservesExtensiblePredicatesInMrvaPack method returns true when the CodeQL CLI version is older than 2.16.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♀️ thank's for spotting that!

filesThatDoNotExist: [],
qlxFilesThatExist: [],
dependenciesToCheck: ["codeql/java-all"],
Expand All @@ -372,13 +365,6 @@ describe("Variant Analysis Manager", () => {
});

it("should run multiple queries that are part of the same pack", async () => {
if (!(await cli.cliConstraints.supportsPackCreateWithMultipleQueries())) {
console.log(
`Skipping test because MRVA with multiple queries is only suppported in CLI version ${CliVersionConstraint.CLI_VERSION_WITH_MULTI_QUERY_PACK_CREATE} or later.`,
);
return;
}

await doVariantAnalysisTest({
queryPaths: [
"data-qlpack-multiple-queries/query1.ql",
Expand Down
Loading