From 034d8b7c687c05f543fa908b5d388158c9f3572f Mon Sep 17 00:00:00 2001 From: Henry Mercer Date: Mon, 1 Jun 2020 19:26:15 +0100 Subject: [PATCH 1/7] Use semver package for version comparison and precedence checking --- common/config/rush/pnpm-lock.yaml | 33 +++++-- extensions/ql-vscode/package.json | 4 +- extensions/ql-vscode/src/cli-version.ts | 85 +------------------ extensions/ql-vscode/src/distribution.ts | 56 +++--------- extensions/ql-vscode/src/extension.ts | 2 +- .../no-workspace/cli-version.test.ts | 50 ----------- .../no-workspace/distribution.test.ts | 47 ++-------- 7 files changed, 53 insertions(+), 224 deletions(-) delete mode 100644 extensions/ql-vscode/src/vscode-tests/no-workspace/cli-version.test.ts diff --git a/common/config/rush/pnpm-lock.yaml b/common/config/rush/pnpm-lock.yaml index 920259144..471ea9a76 100644 --- a/common/config/rush/pnpm-lock.yaml +++ b/common/config/rush/pnpm-lock.yaml @@ -26,6 +26,7 @@ dependencies: '@types/react': 16.9.23 '@types/react-dom': 16.9.5 '@types/sarif': 2.1.2 + '@types/semver': 7.2.0 '@types/sinon': 7.5.2 '@types/sinon-chai': 3.2.3 '@types/through2': 2.0.34 @@ -68,6 +69,7 @@ dependencies: react: 16.13.0 react-dom: 16.13.0_react@16.13.0 reflect-metadata: 0.1.13 + semver: 7.3.2 sinon: 9.0.1 sinon-chai: 3.5.0_chai@4.2.0+sinon@9.0.1 style-loader: 0.23.1 @@ -508,6 +510,12 @@ packages: dev: false resolution: integrity: sha512-TELZl5h48KaB6SFZqTuaMEw1hrGuusbBcH+yfMaaHdS2pwDr3RTH4CVN0LyY1kqSiDm9PPvAMx8FJ0LUZreOCQ== + /@types/semver/7.2.0: + dependencies: + '@types/node': 12.12.30 + dev: false + resolution: + integrity: sha512-TbB0A8ACUWZt3Y6bQPstW9QNbhNeebdgLX4T/ZfkrswAfUzRiXrgd9seol+X379Wa589Pu4UEx9Uok0D4RjRCQ== /@types/sinon-chai/3.2.3: dependencies: '@types/chai': 4.2.11 @@ -6210,6 +6218,13 @@ packages: hasBin: true resolution: integrity: sha512-b39TBaTSfV6yBrapU89p5fKekE2m/NwnDocOVruQFS1/veMgdzuPcnOM34M6CwxW8jH/lxEa5rBoDeUwu5HHTw== + /semver/7.3.2: + dev: false + engines: + node: '>=10' + hasBin: true + resolution: + integrity: sha512-OrOb32TeeambH6UrhtShmF7CRDqhL6/5XpPNp2DuRH6+9QLw/orhp72j87v8Qa1ScDkvrrBNpZcDejAirJmfXQ== /serialize-javascript/2.1.2: dev: false resolution: @@ -7836,7 +7851,7 @@ packages: peerDependencies: glob: '*' resolution: - integrity: sha512-NkoIMaJdASYX4NjcB+nsEk/8Ff/2RLvHwL0efNOny3no6aNuJ3EkpNK0ZdX7HQdmTdY3IJPmjoJ3Rn4pkbxgdA== + integrity: sha512-14DvfY6Fj3HXp2/CNJ2zNh9MA8zPw9mUcr8WqkSsYvJow7JMcIlJ//OOONwpoSWtfrk1bk6Cin7jj9H79ItHQQ== tarball: 'file:projects/build-tasks.tgz' version: 0.0.0 'file:projects/semmle-bqrs.tgz_typescript@3.8.3': @@ -7851,7 +7866,7 @@ packages: peerDependencies: typescript: '*' resolution: - integrity: sha512-lE3FBYrOVF1JH0ZqvF4YA+bed3JPWYucsnFe+XL140a/YR19XD+TTHIfov7VpR9qdyWfARgvmR+gf2qsguXTKQ== + integrity: sha512-24GdnvMbGfQIWMfgDhift+kYJDnG7dX03NrpX4ajZ2rckteysvq2/K7XI1OXGvUuqrt3m0/+GRDHpSI9XKDJJA== tarball: 'file:projects/semmle-bqrs.tgz' version: 0.0.0 'file:projects/semmle-io-node.tgz_typescript@3.8.3': @@ -7866,7 +7881,7 @@ packages: peerDependencies: typescript: '*' resolution: - integrity: sha512-MD9edC5HjrCfPmhktw6XmWotUmperj27/hDZiuMbuSlJ4jRKyiBtJ8Vk2Y4U41TrzsBlJfAwZW8tetPw5ujiLg== + integrity: sha512-Bj0ax/bASrHV7tamOuXZZdd3UOB4NBKdjdszIRaDvDRTu8RlEst+TVoUhkfy30qb2/6ePp3/juOJyyiBJN7u8Q== tarball: 'file:projects/semmle-io-node.tgz' version: 0.0.0 'file:projects/semmle-io.tgz_typescript@3.8.3': @@ -7880,7 +7895,7 @@ packages: peerDependencies: typescript: '*' resolution: - integrity: sha512-ta1lLi1COIeFwpwH523cWheWx6OE8GTqguQmOA7G6CwRF41RYbbREf/4KlOLKO/uG2akhhl+3gcWY2c5/VDC/A== + integrity: sha512-NtyviDSevxbd+hj4J66LucOzo8LU2hJ1Jh0eHw0Qu3tRZPUT8HcQlseyy29AvZR8n8eppfEZiAm/JdiHfmRPMA== tarball: 'file:projects/semmle-io.tgz' version: 0.0.0 'file:projects/semmle-vscode-utils.tgz': @@ -7892,14 +7907,14 @@ packages: dev: false name: '@rush-temp/semmle-vscode-utils' resolution: - integrity: sha512-Dbwt0/Wd0VNKkRZRjFQv3hmGy/UDt36HDtEDsNgZIcQACoY1j2+mJavpQ+ZzCg4Ftj06eHDVk+ptzUEd+8Ybzw== + integrity: sha512-5y5r8SDoN9Fp44naC9gUe8rOexeckXg2T0h9QCJAIcEgnFqOxzRc6Rv9gbMUStFKNh+rFlvmYmgPAdg5QkfgUg== tarball: 'file:projects/semmle-vscode-utils.tgz' version: 0.0.0 'file:projects/typescript-config.tgz': dev: false name: '@rush-temp/typescript-config' resolution: - integrity: sha512-qJbtY2jvt6LKkmUt/seiYyXSEB6Oip/rW+SxofQEnpyplgIQv7whTZb6g5pwlSLGl8goTaQFm4NfazKhFmxXvQ== + integrity: sha512-XuUIySaNoooIduvehnlKYaHqZJmmQoCqB1RtKhNszjCYZaSSJAnKVucViWBf5oNLKSNP7NchrD7gcoBlQ3xYvw== tarball: 'file:projects/typescript-config.tgz' version: 0.0.0 'file:projects/vscode-codeql.tgz': @@ -7921,6 +7936,7 @@ packages: '@types/react': 16.9.23 '@types/react-dom': 16.9.5 '@types/sarif': 2.1.2 + '@types/semver': 7.2.0 '@types/sinon': 7.5.2 '@types/sinon-chai': 3.2.3 '@types/tmp': 0.1.0 @@ -7955,6 +7971,7 @@ packages: proxyquire: 2.1.3 react: 16.13.0 react-dom: 16.13.0_react@16.13.0 + semver: 7.3.2 sinon: 9.0.1 sinon-chai: 3.5.0_chai@4.2.0+sinon@9.0.1 style-loader: 0.23.1 @@ -7978,7 +7995,7 @@ packages: dev: false name: '@rush-temp/vscode-codeql' resolution: - integrity: sha512-ClyrIRqnMYMmVHtHvW8MvS4GrRSt/dXY3lxBpxSv3wSJ67pEvWKea+DJyeVN2zaHz1/7gAOWQHhwBz6O3lEq6w== + integrity: sha512-bU6tGSUD6TzMa6XDiDymvfY28xtDKp6uYPVCwiy7zdsl5NYUxph5Yua0Snoam7oytdYMa2HieTn8Lh6Hkb5P/A== tarball: 'file:projects/vscode-codeql.tgz' version: 0.0.0 registry: '' @@ -8010,6 +8027,7 @@ specifiers: '@types/react': ^16.8.17 '@types/react-dom': ^16.8.4 '@types/sarif': ~2.1.2 + '@types/semver': ~7.2.0 '@types/sinon': ~7.5.2 '@types/sinon-chai': ~3.2.3 '@types/through2': ~2.0.34 @@ -8052,6 +8070,7 @@ specifiers: react: ^16.8.6 react-dom: ^16.8.6 reflect-metadata: ~0.1.13 + semver: ~7.3.2 sinon: ~9.0.0 sinon-chai: ~3.5.0 style-loader: ~0.23.1 diff --git a/extensions/ql-vscode/package.json b/extensions/ql-vscode/package.json index 20d7a0e93..91621b8e2 100644 --- a/extensions/ql-vscode/package.json +++ b/extensions/ql-vscode/package.json @@ -590,7 +590,9 @@ "vscode-languageclient": "^6.1.3", "vscode-test-adapter-api": "~1.7.0", "vscode-test-adapter-util": "~0.7.0", - "minimist": "~1.2.5" + "minimist": "~1.2.5", + "semver": "~7.3.2", + "@types/semver": "~7.2.0" }, "devDependencies": { "@types/chai": "^4.1.7", diff --git a/extensions/ql-vscode/src/cli-version.ts b/extensions/ql-vscode/src/cli-version.ts index 87a8179b9..56cbae5cb 100644 --- a/extensions/ql-vscode/src/cli-version.ts +++ b/extensions/ql-vscode/src/cli-version.ts @@ -1,10 +1,11 @@ +import * as semver from "semver"; import { runCodeQlCliCommand } from "./cli"; import { Logger } from "./logging"; /** * Get the version of a CodeQL CLI. */ -export async function getCodeQlCliVersion(codeQlPath: string, logger: Logger): Promise { +export async function getCodeQlCliVersion(codeQlPath: string, logger: Logger): Promise { const output: string = await runCodeQlCliCommand( codeQlPath, ["version"], @@ -12,85 +13,5 @@ export async function getCodeQlCliVersion(codeQlPath: string, logger: Logger): P "Checking CodeQL version", logger ); - return tryParseVersionString(output.trim()); -} - -/** - * Try to parse a version string, returning undefined if we can't parse it. - * - * Version strings must contain a major, minor, and patch version. They may optionally - * start with "v" and may optionally contain some "tail" string after the major, minor, and - * patch versions, for example as in `v2.1.0+baf5bff`. - */ -export function tryParseVersionString(versionString: string): Version | undefined { - const match = versionString.match(versionRegex); - if (match === null) { - return undefined; - } - return { - buildMetadata: match[5], - majorVersion: Number.parseInt(match[1], 10), - minorVersion: Number.parseInt(match[2], 10), - patchVersion: Number.parseInt(match[3], 10), - prereleaseVersion: match[4], - rawString: versionString, - }; -} - -/** - * Regex for parsing semantic versions - * - * From the semver spec https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string - */ -const versionRegex = new RegExp(String.raw`^v?(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)` + - String.raw`(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?` + - String.raw`(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$`); - -/** - * A version of the CodeQL CLI. - */ -export interface Version { - /** - * Build metadata - * - * For example, this will be `abcdef0` for version 2.1.0-alpha.1+abcdef0. - * Build metadata must be ignored when comparing versions. - */ - buildMetadata: string | undefined; - - /** - * Major version number - * - * For example, this will be `2` for version 2.1.0-alpha.1+abcdef0. - */ - majorVersion: number; - - /** - * Minor version number - * - * For example, this will be `1` for version 2.1.0-alpha.1+abcdef0. - */ - minorVersion: number; - - /** - * Patch version number - * - * For example, this will be `0` for version 2.1.0-alpha.1+abcdef0. - */ - patchVersion: number; - - /** - * Prerelease version - * - * For example, this will be `alpha.1` for version 2.1.0-alpha.1+abcdef0. - * The prerelease version must be considered when comparing versions. - */ - prereleaseVersion: string | undefined; - - /** - * Raw version string - * - * For example, this will be `2.1.0-alpha.1+abcdef0` for version 2.1.0-alpha.1+abcdef0. - */ - rawString: string; + return semver.parse(output.trim()) || undefined; } diff --git a/extensions/ql-vscode/src/distribution.ts b/extensions/ql-vscode/src/distribution.ts index 474c71a3f..ae4f81594 100644 --- a/extensions/ql-vscode/src/distribution.ts +++ b/extensions/ql-vscode/src/distribution.ts @@ -2,6 +2,7 @@ import * as fetch from "node-fetch"; import * as fs from "fs-extra"; import * as os from "os"; import * as path from "path"; +import * as semver from "semver"; import * as unzipper from "unzipper"; import * as url from "url"; import { ExtensionContext, Event } from "vscode"; @@ -9,7 +10,7 @@ import { DistributionConfig } from "./config"; import { InvocationRateLimiter, InvocationRateLimiterResultKind, showAndLogErrorMessage } from "./helpers"; import { logger } from "./logging"; import * as helpers from "./helpers"; -import { getCodeQlCliVersion, tryParseVersionString, Version } from "./cli-version"; +import { getCodeQlCliVersion } from "./cli-version"; /** * distribution.ts @@ -41,9 +42,7 @@ const DEFAULT_DISTRIBUTION_REPOSITORY_NAME = "codeql-cli-binaries"; */ export const DEFAULT_DISTRIBUTION_VERSION_CONSTRAINT: VersionConstraint = { description: "2.*.*", - isVersionCompatible: (v: Version) => { - return v.majorVersion === 2 && v.minorVersion >= 0; - } + isVersionCompatible: (v: semver.SemVer) => semver.satisfies(v, "2.x") }; export interface DistributionProvider { @@ -403,20 +402,16 @@ export class ReleasesApiConsumer { return false; } - const version = tryParseVersionString(release.tag_name); - if (version === undefined || !versionConstraint.isVersionCompatible(version)) { - return false; - } - - return true; + const version = semver.parse(release.tag_name); + return version !== null && versionConstraint.isVersionCompatible(version); }); - // tryParseVersionString must succeed due to the previous filtering step + // Tag names must all be parsable to semvers due to the previous filtering step. const latestRelease = compatibleReleases.sort((a, b) => { - const versionComparison = versionCompare(tryParseVersionString(b.tag_name)!, tryParseVersionString(a.tag_name)!); - if (versionComparison === 0) { - return b.created_at.localeCompare(a.created_at); + const versionComparison = semver.compare(semver.parse(b.tag_name)!, semver.parse(a.tag_name)!); + if (versionComparison !== 0) { + return versionComparison; } - return versionComparison; + return b.created_at.localeCompare(a.created_at, "en-US"); })[0]; if (latestRelease === undefined) { throw new Error("No compatible CodeQL CLI releases were found. " + @@ -516,29 +511,6 @@ export async function extractZipArchive(archivePath: string, outPath: string): P })); } -/** - * Comparison of semantic versions. - * - * Returns a positive number if a is greater than b. - * Returns 0 if a equals b. - * Returns a negative number if a is less than b. - */ -export function versionCompare(a: Version, b: Version): number { - if (a.majorVersion !== b.majorVersion) { - return a.majorVersion - b.majorVersion; - } - if (a.minorVersion !== b.minorVersion) { - return a.minorVersion - b.minorVersion; - } - if (a.patchVersion !== b.patchVersion) { - return a.patchVersion - b.patchVersion; - } - if (a.prereleaseVersion !== undefined && b.prereleaseVersion !== undefined) { - return a.prereleaseVersion.localeCompare(b.prereleaseVersion); - } - return 0; -} - function codeQlLauncherName(): string { return (os.platform() === "win32") ? "codeql.exe" : "codeql"; } @@ -571,7 +543,7 @@ export type FindDistributionResult = interface CompatibleDistributionResult { codeQlPath: string; kind: FindDistributionResultKind.CompatibleDistribution; - version: Version; + version: semver.SemVer; } interface UnknownCompatibilityDistributionResult { @@ -582,7 +554,7 @@ interface UnknownCompatibilityDistributionResult { interface IncompatibleDistributionResult { codeQlPath: string; kind: FindDistributionResultKind.IncompatibleDistribution; - version: Version; + version: semver.SemVer; } interface NoDistributionResult { @@ -722,7 +694,7 @@ export interface GithubRelease { assets: GithubReleaseAsset[]; /** - * The creation date of the release on GitHub. + * The creation date of the release on GitHub, in ISO 8601 format. */ created_at: string; @@ -769,7 +741,7 @@ export interface GithubReleaseAsset { interface VersionConstraint { description: string; - isVersionCompatible(version: Version): boolean; + isVersionCompatible(version: semver.SemVer): boolean; } export class GithubApiError extends Error { diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index f0244ec49..685a6d667 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/extensions/ql-vscode/src/extension.ts @@ -182,7 +182,7 @@ export async function activate(ctx: ExtensionContext): Promise { const result = await distributionManager.getDistribution(); switch (result.kind) { case FindDistributionResultKind.CompatibleDistribution: - logger.log(`Found compatible version of CodeQL CLI (version ${result.version.rawString})`); + logger.log(`Found compatible version of CodeQL CLI (version ${result.version.raw})`); break; case FindDistributionResultKind.IncompatibleDistribution: helpers.showAndLogWarningMessage("The current version of the CodeQL CLI is incompatible with this extension."); diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/cli-version.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/cli-version.test.ts deleted file mode 100644 index 17e2ac1c3..000000000 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/cli-version.test.ts +++ /dev/null @@ -1,50 +0,0 @@ -import { expect } from "chai"; -import "mocha"; -import { tryParseVersionString } from "../../cli-version"; - -describe("Version parsing", () => { - it("should accept version without prerelease and build metadata", () => { - const v = tryParseVersionString("3.2.4")!; - expect(v.majorVersion).to.equal(3); - expect(v.minorVersion).to.equal(2); - expect(v.patchVersion).to.equal(4); - expect(v.prereleaseVersion).to.be.undefined; - expect(v.buildMetadata).to.be.undefined; - }); - - it("should accept v at the beginning of the version", () => { - const v = tryParseVersionString("v3.2.4")!; - expect(v.majorVersion).to.equal(3); - expect(v.minorVersion).to.equal(2); - expect(v.patchVersion).to.equal(4); - expect(v.prereleaseVersion).to.be.undefined; - expect(v.buildMetadata).to.be.undefined; - }); - - it("should accept version with prerelease", () => { - const v = tryParseVersionString("v3.2.4-alpha.0")!; - expect(v.majorVersion).to.equal(3); - expect(v.minorVersion).to.equal(2); - expect(v.patchVersion).to.equal(4); - expect(v.prereleaseVersion).to.equal("alpha.0"); - expect(v.buildMetadata).to.be.undefined; - }); - - it("should accept version with prerelease and build metadata", () => { - const v = tryParseVersionString("v3.2.4-alpha.0+abcdef0")!; - expect(v.majorVersion).to.equal(3); - expect(v.minorVersion).to.equal(2); - expect(v.patchVersion).to.equal(4); - expect(v.prereleaseVersion).to.equal("alpha.0"); - expect(v.buildMetadata).to.equal("abcdef0"); - }); - - it("should accept version with build metadata", () => { - const v = tryParseVersionString("v3.2.4+abcdef0")!; - expect(v.majorVersion).to.equal(3); - expect(v.minorVersion).to.equal(2); - expect(v.patchVersion).to.equal(4); - expect(v.prereleaseVersion).to.be.undefined; - expect(v.buildMetadata).to.equal("abcdef0"); - }); -}); diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts index 38906fa53..2a54aa647 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts @@ -1,14 +1,14 @@ import * as chai from "chai"; import * as path from "path"; import * as fetch from "node-fetch"; -import 'chai/register-should'; -import * as sinonChai from 'sinon-chai'; -import * as sinon from 'sinon'; +import "chai/register-should"; +import * as semver from "semver"; +import * as sinonChai from "sinon-chai"; +import * as sinon from "sinon"; import * as pq from "proxyquire"; import "mocha"; -import { Version } from "../../cli-version"; -import { GithubRelease, GithubReleaseAsset, ReleasesApiConsumer, versionCompare } from "../../distribution"; +import { GithubRelease, GithubReleaseAsset, ReleasesApiConsumer } from "../../distribution"; const proxyquire = pq.noPreserveCache(); chai.use(sinonChai); @@ -86,7 +86,7 @@ describe("Releases API consumer", () => { const latestRelease = await consumer.getLatestRelease({ description: "2.*.*", - isVersionCompatible: version => version.majorVersion === 2 + isVersionCompatible: version => semver.satisfies(version, "2.x") }); expect(latestRelease.id).to.equal(1); }); @@ -152,41 +152,6 @@ describe("Releases API consumer", () => { }); }); -describe("Release version ordering", () => { - function createVersion(majorVersion: number, minorVersion: number, patchVersion: number, prereleaseVersion?: string, buildMetadata?: string): Version { - return { - buildMetadata, - majorVersion, - minorVersion, - patchVersion, - prereleaseVersion, - rawString: `${majorVersion}.${minorVersion}.${patchVersion}` + - prereleaseVersion ? `-${prereleaseVersion}` : "" + - buildMetadata ? `+${buildMetadata}` : "" - }; - } - - it("major versions compare correctly", () => { - expect(versionCompare(createVersion(3, 0, 0), createVersion(2, 9, 9)) > 0).to.be.true; - }); - - it("minor versions compare correctly", () => { - expect(versionCompare(createVersion(2, 1, 0), createVersion(2, 0, 9)) > 0).to.be.true; - }); - - it("patch versions compare correctly", () => { - expect(versionCompare(createVersion(2, 1, 2), createVersion(2, 1, 1)) > 0).to.be.true; - }); - - it("prerelease versions compare correctly", () => { - expect(versionCompare(createVersion(2, 1, 0, "alpha.2"), createVersion(2, 1, 0, "alpha.1")) > 0).to.true; - }); - - it("build metadata compares correctly", () => { - expect(versionCompare(createVersion(2, 1, 0, "alpha.1", "abcdef0"), createVersion(2, 1, 0, "alpha.1", "bcdef01"))).to.equal(0); - }); -}); - describe('Launcher path', () => { const pathToCmd = `abc${path.sep}codeql.cmd`; const pathToExe = `abc${path.sep}codeql.exe`; From 71d4038744b9780307753f0aac67cea499ba459d Mon Sep 17 00:00:00 2001 From: Henry Mercer Date: Mon, 1 Jun 2020 20:04:00 +0100 Subject: [PATCH 2/7] Use version ranges instead of version constraint for simplicity --- extensions/ql-vscode/src/distribution.ts | 46 ++++++++----------- extensions/ql-vscode/src/extension.ts | 4 +- .../no-workspace/distribution.test.ts | 18 +++----- 3 files changed, 27 insertions(+), 41 deletions(-) diff --git a/extensions/ql-vscode/src/distribution.ts b/extensions/ql-vscode/src/distribution.ts index ae4f81594..3de02376f 100644 --- a/extensions/ql-vscode/src/distribution.ts +++ b/extensions/ql-vscode/src/distribution.ts @@ -36,14 +36,11 @@ const DEFAULT_DISTRIBUTION_OWNER_NAME = "github"; const DEFAULT_DISTRIBUTION_REPOSITORY_NAME = "codeql-cli-binaries"; /** - * Version constraint for the CLI. + * Range of versions of the CLI that are compatible with the extension. * * This applies to both extension-managed and CLI distributions. */ -export const DEFAULT_DISTRIBUTION_VERSION_CONSTRAINT: VersionConstraint = { - description: "2.*.*", - isVersionCompatible: (v: semver.SemVer) => semver.satisfies(v, "2.x") -}; +export const DEFAULT_DISTRIBUTION_VERSION_RANGE: semver.Range = new semver.Range("2.x"); export interface DistributionProvider { getCodeQlPathWithoutVersionCheck(): Promise; @@ -51,16 +48,16 @@ export interface DistributionProvider { } export class DistributionManager implements DistributionProvider { - constructor(extensionContext: ExtensionContext, config: DistributionConfig, versionConstraint: VersionConstraint) { + constructor(extensionContext: ExtensionContext, config: DistributionConfig, versionRange: semver.Range) { this._config = config; - this._extensionSpecificDistributionManager = new ExtensionSpecificDistributionManager(extensionContext, config, versionConstraint); + this._extensionSpecificDistributionManager = new ExtensionSpecificDistributionManager(extensionContext, config, versionRange); this._onDidChangeDistribution = config.onDidChangeDistributionConfiguration; this._updateCheckRateLimiter = new InvocationRateLimiter( extensionContext, "extensionSpecificDistributionUpdateCheck", () => this._extensionSpecificDistributionManager.checkForUpdatesToDistribution() ); - this._versionConstraint = versionConstraint; + this._versionRange = versionRange; } /** @@ -74,19 +71,19 @@ export class DistributionManager implements DistributionProvider { }; } const version = await getCodeQlCliVersion(codeQlPath, logger); - if (version !== undefined && !this._versionConstraint.isVersionCompatible(version)) { - return { - codeQlPath, - kind: FindDistributionResultKind.IncompatibleDistribution, - version, - }; - } if (version === undefined) { return { codeQlPath, kind: FindDistributionResultKind.UnknownCompatibilityDistribution, }; } + if (!semver.satisfies(version, this._versionRange)) { + return { + codeQlPath, + kind: FindDistributionResultKind.IncompatibleDistribution, + version, + }; + } return { codeQlPath, kind: FindDistributionResultKind.CompatibleDistribution, @@ -197,14 +194,14 @@ export class DistributionManager implements DistributionProvider { private readonly _extensionSpecificDistributionManager: ExtensionSpecificDistributionManager; private readonly _updateCheckRateLimiter: InvocationRateLimiter; private readonly _onDidChangeDistribution: Event | undefined; - private readonly _versionConstraint: VersionConstraint; + private readonly _versionRange: semver.Range; } class ExtensionSpecificDistributionManager { - constructor(extensionContext: ExtensionContext, config: DistributionConfig, versionConstraint: VersionConstraint) { + constructor(extensionContext: ExtensionContext, config: DistributionConfig, versionRange: semver.Range) { this._extensionContext = extensionContext; this._config = config; - this._versionConstraint = versionConstraint; + this._versionRange = versionRange; } public async getCodeQlPathWithoutVersionCheck(): Promise { @@ -325,7 +322,7 @@ class ExtensionSpecificDistributionManager { } private async getLatestRelease(): Promise { - const release = await this.createReleasesApiConsumer().getLatestRelease(this._versionConstraint, this._config.includePrerelease); + const release = await this.createReleasesApiConsumer().getLatestRelease(this._versionRange, this._config.includePrerelease); // FIXME: Look for platform-specific codeql distribution if available release.assets = release.assets.filter(asset => asset.name === 'codeql.zip'); if (release.assets.length === 0) { @@ -373,7 +370,7 @@ class ExtensionSpecificDistributionManager { private readonly _config: DistributionConfig; private readonly _extensionContext: ExtensionContext; - private readonly _versionConstraint: VersionConstraint; + private readonly _versionRange: semver.Range; private static readonly _currentDistributionFolderBaseName = "distribution"; private static readonly _currentDistributionFolderIndexStateKey = "distributionFolderIndex"; @@ -394,7 +391,7 @@ export class ReleasesApiConsumer { this._repoName = repoName; } - public async getLatestRelease(versionConstraint: VersionConstraint, includePrerelease = false): Promise { + public async getLatestRelease(versionRange: semver.Range, includePrerelease = false): Promise { const apiPath = `/repos/${this._ownerName}/${this._repoName}/releases`; const allReleases: GithubRelease[] = await (await this.makeApiCall(apiPath)).json(); const compatibleReleases = allReleases.filter(release => { @@ -403,7 +400,7 @@ export class ReleasesApiConsumer { } const version = semver.parse(release.tag_name); - return version !== null && versionConstraint.isVersionCompatible(version); + return version !== null && semver.satisfies(version, versionRange); }); // Tag names must all be parsable to semvers due to the previous filtering step. const latestRelease = compatibleReleases.sort((a, b) => { @@ -739,11 +736,6 @@ export interface GithubReleaseAsset { size: number; } -interface VersionConstraint { - description: string; - isVersionCompatible(version: semver.SemVer): boolean; -} - export class GithubApiError extends Error { constructor(public status: number, public body: string) { super(`API call failed with status code ${status}, body: ${body}`); diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index 685a6d667..1a00cace6 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/extensions/ql-vscode/src/extension.ts @@ -8,7 +8,7 @@ import * as languageSupport from './languageSupport'; import { DatabaseManager } from './databases'; import { DatabaseUI } from './databases-ui'; import { TemplateQueryDefinitionProvider, TemplateQueryReferenceProvider } from './definitions'; -import { DEFAULT_DISTRIBUTION_VERSION_CONSTRAINT, DistributionManager, DistributionUpdateCheckResultKind, FindDistributionResult, FindDistributionResultKind, GithubApiError, GithubRateLimitedError } from './distribution'; +import { DEFAULT_DISTRIBUTION_VERSION_RANGE, DistributionManager, DistributionUpdateCheckResultKind, FindDistributionResult, FindDistributionResultKind, GithubApiError, GithubRateLimitedError } from './distribution'; import * as helpers from './helpers'; import { assertNever } from './helpers-pure'; import { spawnIdeServer } from './ide-server'; @@ -83,7 +83,7 @@ export async function activate(ctx: ExtensionContext): Promise { const distributionConfigListener = new DistributionConfigListener(); ctx.subscriptions.push(distributionConfigListener); - const distributionManager = new DistributionManager(ctx, distributionConfigListener, DEFAULT_DISTRIBUTION_VERSION_CONSTRAINT); + const distributionManager = new DistributionManager(ctx, distributionConfigListener, DEFAULT_DISTRIBUTION_VERSION_RANGE); const shouldUpdateOnNextActivationKey = "shouldUpdateOnNextActivation"; diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts index 2a54aa647..d7de15d14 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts @@ -51,10 +51,7 @@ describe("Releases API consumer", () => { "tag_name": "v3.1.2-pre" }, ]; - const unconstrainedVersionConstraint = { - description: "*", - isVersionCompatible: () => true - }; + const unconstrainedVersionRange = new semver.Range("*"); it("picking latest release: is based on version", async () => { class MockReleasesApiConsumer extends ReleasesApiConsumer { @@ -68,11 +65,11 @@ describe("Releases API consumer", () => { const consumer = new MockReleasesApiConsumer(owner, repo); - const latestRelease = await consumer.getLatestRelease(unconstrainedVersionConstraint); + const latestRelease = await consumer.getLatestRelease(unconstrainedVersionRange); expect(latestRelease.id).to.equal(2); }); - it("picking latest release: obeys version constraints", async () => { + it("picking latest release: version satisfies version range", async () => { class MockReleasesApiConsumer extends ReleasesApiConsumer { protected async makeApiCall(apiPath: string): Promise { if (apiPath === `/repos/${owner}/${repo}/releases`) { @@ -84,10 +81,7 @@ describe("Releases API consumer", () => { const consumer = new MockReleasesApiConsumer(owner, repo); - const latestRelease = await consumer.getLatestRelease({ - description: "2.*.*", - isVersionCompatible: version => semver.satisfies(version, "2.x") - }); + const latestRelease = await consumer.getLatestRelease(new semver.Range("2.*.*")); expect(latestRelease.id).to.equal(1); }); @@ -103,7 +97,7 @@ describe("Releases API consumer", () => { const consumer = new MockReleasesApiConsumer(owner, repo); - const latestRelease = await consumer.getLatestRelease(unconstrainedVersionConstraint, true); + const latestRelease = await consumer.getLatestRelease(unconstrainedVersionRange, true); expect(latestRelease.id).to.equal(4); }); @@ -141,7 +135,7 @@ describe("Releases API consumer", () => { const consumer = new MockReleasesApiConsumer(owner, repo); - const assets = (await consumer.getLatestRelease(unconstrainedVersionConstraint)).assets; + const assets = (await consumer.getLatestRelease(unconstrainedVersionRange)).assets; expect(assets.length).to.equal(expectedAssets.length); expectedAssets.map((expectedAsset, index) => { From 8208940532b7cc7794e04cab140189ecc7b8a7c5 Mon Sep 17 00:00:00 2001 From: Henry Mercer Date: Mon, 1 Jun 2020 20:13:33 +0100 Subject: [PATCH 3/7] Introduce release compatibility check before selecting the most recent --- extensions/ql-vscode/src/distribution.ts | 34 ++++++++++++------- .../no-workspace/distribution.test.ts | 26 +++++++++++++- 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/extensions/ql-vscode/src/distribution.ts b/extensions/ql-vscode/src/distribution.ts index 3de02376f..b2a441347 100644 --- a/extensions/ql-vscode/src/distribution.ts +++ b/extensions/ql-vscode/src/distribution.ts @@ -322,16 +322,22 @@ class ExtensionSpecificDistributionManager { } private async getLatestRelease(): Promise { - const release = await this.createReleasesApiConsumer().getLatestRelease(this._versionRange, this._config.includePrerelease); - // FIXME: Look for platform-specific codeql distribution if available - release.assets = release.assets.filter(asset => asset.name === 'codeql.zip'); - if (release.assets.length === 0) { - throw new Error("Release had no asset named codeql.zip"); - } - else if (release.assets.length > 1) { - throw new Error("Release had more than one asset named codeql.zip"); - } - return release; + return await this.createReleasesApiConsumer().getLatestRelease( + this._versionRange, + this._config.includePrerelease, + release => { + // FIXME: Look for platform-specific codeql distribution if available + // https://github.com/github/vscode-codeql/issues/417 + const matchingAssets = release.assets.filter(asset => asset.name === 'codeql.zip'); + if (matchingAssets.length !== 1) { + if (matchingAssets.length > 1) { + logger.log("WARNING: Ignoring a release with more than one asset named codeql.zip"); + } + return false; + } + return true; + } + ); } private createReleasesApiConsumer(): ReleasesApiConsumer { @@ -391,7 +397,7 @@ export class ReleasesApiConsumer { this._repoName = repoName; } - public async getLatestRelease(versionRange: semver.Range, includePrerelease = false): Promise { + public async getLatestRelease(versionRange: semver.Range, includePrerelease = false, additionalCompatibilityCheck?: (release: GithubRelease) => boolean): Promise { const apiPath = `/repos/${this._ownerName}/${this._repoName}/releases`; const allReleases: GithubRelease[] = await (await this.makeApiCall(apiPath)).json(); const compatibleReleases = allReleases.filter(release => { @@ -400,7 +406,11 @@ export class ReleasesApiConsumer { } const version = semver.parse(release.tag_name); - return version !== null && semver.satisfies(version, versionRange); + if (version === null || !semver.satisfies(version, versionRange)) { + return false; + } + + return !additionalCompatibilityCheck || additionalCompatibilityCheck(release); }); // Tag names must all be parsable to semvers due to the previous filtering step. const latestRelease = compatibleReleases.sort((a, b) => { diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts index d7de15d14..7377193e8 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts @@ -35,7 +35,11 @@ describe("Releases API consumer", () => { "tag_name": "v3.1.1" }, { - "assets": [], + "assets": [{ + id: 1, + name: "exampleAsset.txt", + size: 1 + }], "created_at": "2019-09-05T00:00:00Z", "id": 3, "name": "", @@ -85,6 +89,26 @@ describe("Releases API consumer", () => { expect(latestRelease.id).to.equal(1); }); + it("picking latest release: release passes additional compatibility test if additional compatibility test specified", async () => { + class MockReleasesApiConsumer extends ReleasesApiConsumer { + protected async makeApiCall(apiPath: string): Promise { + if (apiPath === `/repos/${owner}/${repo}/releases`) { + return Promise.resolve(new fetch.Response(JSON.stringify(sampleReleaseResponse))); + } + return Promise.reject(new Error(`Unknown API path: ${apiPath}`)); + } + } + + const consumer = new MockReleasesApiConsumer(owner, repo); + + const latestRelease = await consumer.getLatestRelease( + new semver.Range("2.*.*"), + true, + release => release.assets.some(asset => asset.name === "exampleAsset.txt") + ); + expect(latestRelease.id).to.equal(3); + }); + it("picking latest release: includes prereleases when option set", async () => { class MockReleasesApiConsumer extends ReleasesApiConsumer { protected async makeApiCall(apiPath: string): Promise { From 18a9e2794e1b157af3276076be3cceed920a1930 Mon Sep 17 00:00:00 2001 From: Henry Mercer Date: Mon, 1 Jun 2020 22:11:45 +0100 Subject: [PATCH 4/7] Update handling of prerelease versions of the CodeQL CLI. Suppose a user has the includePrereleases config option set, installs an extension-managed prerelease, then decides they no longer want prereleases and disables includePrereleases. In this case, we should prompt the user to downgrade the CLI to a non-prerelease version. However, if the user is managing their own CLI, we will allow them to use prereleases without incompatibility prompts. --- extensions/ql-vscode/src/distribution.ts | 81 +++++++++++++++++++----- extensions/ql-vscode/src/extension.ts | 32 ++++++++-- 2 files changed, 92 insertions(+), 21 deletions(-) diff --git a/extensions/ql-vscode/src/distribution.ts b/extensions/ql-vscode/src/distribution.ts index b2a441347..cef784c0e 100644 --- a/extensions/ql-vscode/src/distribution.ts +++ b/extensions/ql-vscode/src/distribution.ts @@ -64,28 +64,47 @@ export class DistributionManager implements DistributionProvider { * Look up a CodeQL launcher binary. */ public async getDistribution(): Promise { - const codeQlPath = await this.getCodeQlPathWithoutVersionCheck(); - if (codeQlPath === undefined) { + const distribution = await this.getDistributionWithoutVersionCheck(); + if (distribution === undefined) { return { kind: FindDistributionResultKind.NoDistribution, }; } - const version = await getCodeQlCliVersion(codeQlPath, logger); + const version = await getCodeQlCliVersion(distribution.codeQlPath, logger); if (version === undefined) { return { - codeQlPath, + distribution, kind: FindDistributionResultKind.UnknownCompatibilityDistribution, }; } - if (!semver.satisfies(version, this._versionRange)) { + + /** + * Specifies whether prerelease versions of the CodeQL CLI should be accepted. + * + * Suppose a user sets the includePrerelease config option, obtains a prerelease, then decides + * they no longer want a prerelease, so unsets the includePrerelease config option. + * Unsetting the includePrerelease config option should trigger an update check, and this + * update check should present them an update that returns them back to a non-prerelease + * version. + * + * Therefore, we adopt the following: + * + * - If the user is managing their own CLI, they can use a prerelease without specifying the + * includePrerelease option. + * - If the user is using an extension-managed CLI, then prereleases are only accepted when the + * includePrerelease config option is set. + */ + const includePrerelease = distribution.kind !== DistributionKind.ExtensionManaged || this._config.includePrerelease; + + if (!semver.satisfies(version, this._versionRange, { includePrerelease })) { return { - codeQlPath, + distribution, kind: FindDistributionResultKind.IncompatibleDistribution, version, }; } return { - codeQlPath, + distribution, kind: FindDistributionResultKind.CompatibleDistribution, version }; @@ -96,10 +115,18 @@ export class DistributionManager implements DistributionProvider { return result.kind !== FindDistributionResultKind.NoDistribution; } + public async getCodeQlPathWithoutVersionCheck(): Promise { + const distribution = await this.getDistributionWithoutVersionCheck(); + if (distribution === undefined) { + return undefined; + } + return distribution.codeQlPath; + } + /** * Returns the path to a possibly-compatible CodeQL launcher binary, or undefined if a binary not be found. */ - public async getCodeQlPathWithoutVersionCheck(): Promise { + async getDistributionWithoutVersionCheck(): Promise { // Check config setting, then extension specific distribution, then PATH. if (this._config.customCodeQlPath) { if (!await fs.pathExists(this._config.customCodeQlPath)) { @@ -117,19 +144,28 @@ export class DistributionManager implements DistributionProvider { ) { warnDeprecatedLauncher(); } - return this._config.customCodeQlPath; + return { + codeQlPath: this._config.customCodeQlPath, + kind: DistributionKind.CustomPathConfig + }; } const extensionSpecificCodeQlPath = await this._extensionSpecificDistributionManager.getCodeQlPathWithoutVersionCheck(); if (extensionSpecificCodeQlPath !== undefined) { - return extensionSpecificCodeQlPath; + return { + codeQlPath: extensionSpecificCodeQlPath, + kind: DistributionKind.ExtensionManaged + }; } if (process.env.PATH) { for (const searchDirectory of process.env.PATH.split(path.delimiter)) { const expectedLauncherPath = await getExecutableFromDirectory(searchDirectory); if (expectedLauncherPath) { - return expectedLauncherPath; + return { + codeQlPath: expectedLauncherPath, + kind: DistributionKind.PathEnvironmentVariable + }; } } logger.log("INFO: Could not find CodeQL on path."); @@ -146,9 +182,9 @@ export class DistributionManager implements DistributionProvider { */ public async checkForUpdatesToExtensionManagedDistribution( minSecondsSinceLastUpdateCheck: number): Promise { - const codeQlPath = await this.getCodeQlPathWithoutVersionCheck(); + const distribution = await this.getDistributionWithoutVersionCheck(); const extensionManagedCodeQlPath = await this._extensionSpecificDistributionManager.getCodeQlPathWithoutVersionCheck(); - if (codeQlPath !== undefined && codeQlPath !== extensionManagedCodeQlPath) { + if (distribution !== undefined && distribution.codeQlPath !== extensionManagedCodeQlPath) { // A distribution is present but it isn't managed by the extension. return createInvalidLocationResult(); } @@ -406,7 +442,7 @@ export class ReleasesApiConsumer { } const version = semver.parse(release.tag_name); - if (version === null || !semver.satisfies(version, versionRange)) { + if (version === null || !semver.satisfies(version, versionRange, { includePrerelease })) { return false; } @@ -534,6 +570,17 @@ function isRedirectStatusCode(statusCode: number): boolean { * Types and helper functions relating to those types. */ +export enum DistributionKind { + CustomPathConfig, + ExtensionManaged, + PathEnvironmentVariable +} + +export interface Distribution { + codeQlPath: string; + kind: DistributionKind; +} + export enum FindDistributionResultKind { CompatibleDistribution, UnknownCompatibilityDistribution, @@ -548,18 +595,18 @@ export type FindDistributionResult = | NoDistributionResult; interface CompatibleDistributionResult { - codeQlPath: string; + distribution: Distribution; kind: FindDistributionResultKind.CompatibleDistribution; version: semver.SemVer; } interface UnknownCompatibilityDistributionResult { - codeQlPath: string; + distribution: Distribution; kind: FindDistributionResultKind.UnknownCompatibilityDistribution; } interface IncompatibleDistributionResult { - codeQlPath: string; + distribution: Distribution; kind: FindDistributionResultKind.IncompatibleDistribution; version: semver.SemVer; } diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index 1a00cace6..bbd201510 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/extensions/ql-vscode/src/extension.ts @@ -8,7 +8,16 @@ import * as languageSupport from './languageSupport'; import { DatabaseManager } from './databases'; import { DatabaseUI } from './databases-ui'; import { TemplateQueryDefinitionProvider, TemplateQueryReferenceProvider } from './definitions'; -import { DEFAULT_DISTRIBUTION_VERSION_RANGE, DistributionManager, DistributionUpdateCheckResultKind, FindDistributionResult, FindDistributionResultKind, GithubApiError, GithubRateLimitedError } from './distribution'; +import { + DEFAULT_DISTRIBUTION_VERSION_RANGE, + DistributionKind, + DistributionManager, + DistributionUpdateCheckResultKind, + FindDistributionResult, + FindDistributionResultKind, + GithubApiError, + GithubRateLimitedError +} from './distribution'; import * as helpers from './helpers'; import { assertNever } from './helpers-pure'; import { spawnIdeServer } from './ide-server'; @@ -83,7 +92,8 @@ export async function activate(ctx: ExtensionContext): Promise { const distributionConfigListener = new DistributionConfigListener(); ctx.subscriptions.push(distributionConfigListener); - const distributionManager = new DistributionManager(ctx, distributionConfigListener, DEFAULT_DISTRIBUTION_VERSION_RANGE); + const codeQlVersionRange = DEFAULT_DISTRIBUTION_VERSION_RANGE; + const distributionManager = new DistributionManager(ctx, distributionConfigListener, codeQlVersionRange); const shouldUpdateOnNextActivationKey = "shouldUpdateOnNextActivation"; @@ -184,9 +194,23 @@ export async function activate(ctx: ExtensionContext): Promise { case FindDistributionResultKind.CompatibleDistribution: logger.log(`Found compatible version of CodeQL CLI (version ${result.version.raw})`); break; - case FindDistributionResultKind.IncompatibleDistribution: - helpers.showAndLogWarningMessage("The current version of the CodeQL CLI is incompatible with this extension."); + case FindDistributionResultKind.IncompatibleDistribution: { + const fixGuidanceMessage = (() => { + switch (result.distribution.kind) { + case DistributionKind.ExtensionManaged: + return "Please update the CodeQL CLI by running the \"CodeQL: Check for CLI Updates\" command."; + case DistributionKind.CustomPathConfig: + return `Please update the \"CodeQL CLI Executable Path\" setting to point to a CLI in the version range ${codeQlVersionRange}.`; + case DistributionKind.PathEnvironmentVariable: + return `Please update the CodeQL CLI on your PATH to a version in the range ${codeQlVersionRange}, or ` + + `set the \"CodeQL CLI Executable Path\" setting to the path of a CLI in the version range ${codeQlVersionRange}.`; + } + })(); + + helpers.showAndLogWarningMessage(`The current version of the CodeQL CLI (${result.version.raw}) ` + + "is incompatible with this extension. " + fixGuidanceMessage); break; + } case FindDistributionResultKind.UnknownCompatibilityDistribution: helpers.showAndLogWarningMessage("Compatibility with the configured CodeQL CLI could not be determined. " + "You may experience problems using the extension."); From b7a97d34e5c9afe7239a0d92580c5a7ba322545c Mon Sep 17 00:00:00 2001 From: Henry Mercer Date: Tue, 2 Jun 2020 10:21:16 +0100 Subject: [PATCH 5/7] Apply suggestions from code review Co-authored-by: Andrew Eisenberg --- extensions/ql-vscode/src/distribution.ts | 7 ++----- extensions/ql-vscode/src/extension.ts | 4 ++-- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/extensions/ql-vscode/src/distribution.ts b/extensions/ql-vscode/src/distribution.ts index cef784c0e..3c08c66a8 100644 --- a/extensions/ql-vscode/src/distribution.ts +++ b/extensions/ql-vscode/src/distribution.ts @@ -117,10 +117,7 @@ export class DistributionManager implements DistributionProvider { public async getCodeQlPathWithoutVersionCheck(): Promise { const distribution = await this.getDistributionWithoutVersionCheck(); - if (distribution === undefined) { - return undefined; - } - return distribution.codeQlPath; + return distribution?.codeQlPath; } /** @@ -184,7 +181,7 @@ export class DistributionManager implements DistributionProvider { minSecondsSinceLastUpdateCheck: number): Promise { const distribution = await this.getDistributionWithoutVersionCheck(); const extensionManagedCodeQlPath = await this._extensionSpecificDistributionManager.getCodeQlPathWithoutVersionCheck(); - if (distribution !== undefined && distribution.codeQlPath !== extensionManagedCodeQlPath) { + if (distribution?.codeQlPath !== extensionManagedCodeQlPath) { // A distribution is present but it isn't managed by the extension. return createInvalidLocationResult(); } diff --git a/extensions/ql-vscode/src/extension.ts b/extensions/ql-vscode/src/extension.ts index bbd201510..4b48cf09b 100644 --- a/extensions/ql-vscode/src/extension.ts +++ b/extensions/ql-vscode/src/extension.ts @@ -202,8 +202,8 @@ export async function activate(ctx: ExtensionContext): Promise { case DistributionKind.CustomPathConfig: return `Please update the \"CodeQL CLI Executable Path\" setting to point to a CLI in the version range ${codeQlVersionRange}.`; case DistributionKind.PathEnvironmentVariable: - return `Please update the CodeQL CLI on your PATH to a version in the range ${codeQlVersionRange}, or ` + - `set the \"CodeQL CLI Executable Path\" setting to the path of a CLI in the version range ${codeQlVersionRange}.`; + return `Please update the CodeQL CLI on your PATH to a version compatible with ${codeQlVersionRange}, or ` + + `set the \"CodeQL CLI Executable Path\" setting to the path of a CLI version compatible with ${codeQlVersionRange}.`; } })(); From eba67f8f4ffb89e82bfcfa41eb2c115fd17b63eb Mon Sep 17 00:00:00 2001 From: Henry Mercer Date: Tue, 2 Jun 2020 18:10:36 +0100 Subject: [PATCH 6/7] Apply suggestions from review --- extensions/ql-vscode/src/distribution.ts | 27 ++++++++++++++++-------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/extensions/ql-vscode/src/distribution.ts b/extensions/ql-vscode/src/distribution.ts index 3c08c66a8..7044b710e 100644 --- a/extensions/ql-vscode/src/distribution.ts +++ b/extensions/ql-vscode/src/distribution.ts @@ -362,10 +362,13 @@ class ExtensionSpecificDistributionManager { // FIXME: Look for platform-specific codeql distribution if available // https://github.com/github/vscode-codeql/issues/417 const matchingAssets = release.assets.filter(asset => asset.name === 'codeql.zip'); - if (matchingAssets.length !== 1) { - if (matchingAssets.length > 1) { - logger.log("WARNING: Ignoring a release with more than one asset named codeql.zip"); - } + if (matchingAssets.length === 0) { + // For example, this could be a release with only platform-specific assets. + logger.log("INFO: Ignoring a release with no assets named codeql.zip"); + return false; + } + if (matchingAssets.length > 1) { + logger.log("WARNING: Ignoring a release with more than one asset named codeql.zip"); return false; } return true; @@ -591,19 +594,25 @@ export type FindDistributionResult = | IncompatibleDistributionResult | NoDistributionResult; -interface CompatibleDistributionResult { +/** + * A result representing a distribution of the CodeQL CLI that may or may not be compatible with + * the extension. + */ +interface DistributionResult { distribution: Distribution; + kind: FindDistributionResultKind; +} + +interface CompatibleDistributionResult extends DistributionResult { kind: FindDistributionResultKind.CompatibleDistribution; version: semver.SemVer; } -interface UnknownCompatibilityDistributionResult { - distribution: Distribution; +interface UnknownCompatibilityDistributionResult extends DistributionResult { kind: FindDistributionResultKind.UnknownCompatibilityDistribution; } -interface IncompatibleDistributionResult { - distribution: Distribution; +interface IncompatibleDistributionResult extends DistributionResult { kind: FindDistributionResultKind.IncompatibleDistribution; version: semver.SemVer; } From c4766e464b19451575527d1b91112a26ac6377c0 Mon Sep 17 00:00:00 2001 From: Henry Mercer Date: Tue, 2 Jun 2020 18:28:13 +0100 Subject: [PATCH 7/7] Add additional tests for choosing the latest release of the CodeQL CLI --- .../no-workspace/distribution.test.ts | 174 +++++++++--------- 1 file changed, 89 insertions(+), 85 deletions(-) diff --git a/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts b/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts index 7377193e8..eec59fb24 100644 --- a/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts +++ b/extensions/ql-vscode/src/vscode-tests/no-workspace/distribution.test.ts @@ -17,47 +17,58 @@ const expect = chai.expect; describe("Releases API consumer", () => { const owner = "someowner"; const repo = "somerepo"; - const sampleReleaseResponse: GithubRelease[] = [ - { - "assets": [], - "created_at": "2019-09-01T00:00:00Z", - "id": 1, - "name": "", - "prerelease": false, - "tag_name": "v2.1.0" - }, - { - "assets": [], - "created_at": "2019-08-10T00:00:00Z", - "id": 2, - "name": "", - "prerelease": false, - "tag_name": "v3.1.1" - }, - { - "assets": [{ - id: 1, - name: "exampleAsset.txt", - size: 1 - }], - "created_at": "2019-09-05T00:00:00Z", - "id": 3, - "name": "", - "prerelease": false, - "tag_name": "v2.0.0" - }, - { - "assets": [], - "created_at": "2019-08-11T00:00:00Z", - "id": 4, - "name": "", - "prerelease": true, - "tag_name": "v3.1.2-pre" - }, - ]; const unconstrainedVersionRange = new semver.Range("*"); - it("picking latest release: is based on version", async () => { + describe("picking the latest release", () => { + const sampleReleaseResponse: GithubRelease[] = [ + { + "assets": [], + "created_at": "2019-09-01T00:00:00Z", + "id": 1, + "name": "", + "prerelease": false, + "tag_name": "v2.1.0" + }, + { + "assets": [], + "created_at": "2019-08-10T00:00:00Z", + "id": 2, + "name": "", + "prerelease": false, + "tag_name": "v3.1.1" + }, + { + "assets": [{ + id: 1, + name: "exampleAsset.txt", + size: 1 + }], + "created_at": "2019-09-05T00:00:00Z", + "id": 3, + "name": "", + "prerelease": false, + "tag_name": "v2.0.0" + }, + { + "assets": [], + "created_at": "2019-08-11T00:00:00Z", + "id": 4, + "name": "", + "prerelease": true, + "tag_name": "v3.1.2-pre-1.1" + }, + // Release ID 5 is older than release ID 4 but its version has a higher precedence, so release + // ID 5 should be picked over release ID 4. + { + "assets": [], + "created_at": "2019-08-09T00:00:00Z", + "id": 5, + "name": "", + "prerelease": true, + "tag_name": "v3.1.2-pre-2.0" + }, + ]; + class MockReleasesApiConsumer extends ReleasesApiConsumer { protected async makeApiCall(apiPath: string): Promise { if (apiPath === `/repos/${owner}/${repo}/releases`) { @@ -67,62 +78,55 @@ describe("Releases API consumer", () => { } } - const consumer = new MockReleasesApiConsumer(owner, repo); + it("picked release has version with the highest precedence", async () => { + const consumer = new MockReleasesApiConsumer(owner, repo); - const latestRelease = await consumer.getLatestRelease(unconstrainedVersionRange); - expect(latestRelease.id).to.equal(2); - }); + const latestRelease = await consumer.getLatestRelease(unconstrainedVersionRange); + expect(latestRelease.id).to.equal(2); + }); - it("picking latest release: version satisfies version range", async () => { - class MockReleasesApiConsumer extends ReleasesApiConsumer { - protected async makeApiCall(apiPath: string): Promise { - if (apiPath === `/repos/${owner}/${repo}/releases`) { - return Promise.resolve(new fetch.Response(JSON.stringify(sampleReleaseResponse))); - } - return Promise.reject(new Error(`Unknown API path: ${apiPath}`)); - } - } + it("version of picked release is within the version range", async () => { + const consumer = new MockReleasesApiConsumer(owner, repo); - const consumer = new MockReleasesApiConsumer(owner, repo); + const latestRelease = await consumer.getLatestRelease(new semver.Range("2.*.*")); + expect(latestRelease.id).to.equal(1); + }); - const latestRelease = await consumer.getLatestRelease(new semver.Range("2.*.*")); - expect(latestRelease.id).to.equal(1); - }); + it("fails if none of the releases are within the version range", async () => { + const consumer = new MockReleasesApiConsumer(owner, repo); - it("picking latest release: release passes additional compatibility test if additional compatibility test specified", async () => { - class MockReleasesApiConsumer extends ReleasesApiConsumer { - protected async makeApiCall(apiPath: string): Promise { - if (apiPath === `/repos/${owner}/${repo}/releases`) { - return Promise.resolve(new fetch.Response(JSON.stringify(sampleReleaseResponse))); - } - return Promise.reject(new Error(`Unknown API path: ${apiPath}`)); - } - } + await chai.expect( + consumer.getLatestRelease(new semver.Range("5.*.*")) + ).to.be.rejectedWith(Error); + }); - const consumer = new MockReleasesApiConsumer(owner, repo); + it("picked release passes additional compatibility test if an additional compatibility test is specified", async () => { + const consumer = new MockReleasesApiConsumer(owner, repo); - const latestRelease = await consumer.getLatestRelease( - new semver.Range("2.*.*"), - true, - release => release.assets.some(asset => asset.name === "exampleAsset.txt") - ); - expect(latestRelease.id).to.equal(3); - }); + const latestRelease = await consumer.getLatestRelease( + new semver.Range("2.*.*"), + true, + release => release.assets.some(asset => asset.name === "exampleAsset.txt") + ); + expect(latestRelease.id).to.equal(3); + }); - it("picking latest release: includes prereleases when option set", async () => { - class MockReleasesApiConsumer extends ReleasesApiConsumer { - protected async makeApiCall(apiPath: string): Promise { - if (apiPath === `/repos/${owner}/${repo}/releases`) { - return Promise.resolve(new fetch.Response(JSON.stringify(sampleReleaseResponse))); - } - return Promise.reject(new Error(`Unknown API path: ${apiPath}`)); - } - } + it("fails if none of the releases pass the additional compatibility test", async () => { + const consumer = new MockReleasesApiConsumer(owner, repo); - const consumer = new MockReleasesApiConsumer(owner, repo); + await chai.expect(consumer.getLatestRelease( + new semver.Range("2.*.*"), + true, + release => release.assets.some(asset => asset.name === "otherExampleAsset.txt") + )).to.be.rejectedWith(Error); + }); - const latestRelease = await consumer.getLatestRelease(unconstrainedVersionRange, true); - expect(latestRelease.id).to.equal(4); + it("picked release is the most recent prerelease when includePrereleases is set", async () => { + const consumer = new MockReleasesApiConsumer(owner, repo); + + const latestRelease = await consumer.getLatestRelease(unconstrainedVersionRange, true); + expect(latestRelease.id).to.equal(5); + }); }); it("gets correct assets for a release", async () => {