Support cache-aware CodeQL bundles (codeql-bundle 0.5.0) - #39
Support cache-aware CodeQL bundles (codeql-bundle 0.5.0)#39nicolaswill wants to merge 4 commits into
Conversation
codeql-bundle's cache-aware bundle support reads two package data files at runtime via importlib.resources: codeql_bundle/supported-codeql-bundles.json codeql_bundle/supported-codeql-bundles.schema.json PyInstaller only freezes Python modules, so these were missing from the frozen binary QLT ships and every invocation aborted with an unhandled FileNotFoundError, breaking 'qlt codeql run install --custom-bundle' and '--quick-bundle'. Pass both files to PyInstaller with --add-data. The separator expected by --add-data is os.pathsep, which is ';' on Windows and ':' elsewhere, so it is resolved from [System.IO.Path]::PathSeparator rather than hardcoded -- this script runs on all three platforms. The Test-Path guard keeps the script working for older codeql-bundle releases that do not ship these files, and the final argument list is echoed before the build. Also bump the pinned codeql-bundle version to 0.5.0, which is the release that introduces these files.
Expose the codeql-bundle cache directory through QLT, either as a 'CacheDir' key in qlt.conf.json or as 'qlt codeql run install --cache-dir' on the command line, with the command line taking precedence. The directory holds downloaded bundles and published compilation caches, so restoring it across CI runs (e.g. with actions/cache) avoids re-downloading and lets the cache-aware bundle support reuse prebuilt compilation caches. Relative paths are resolved against the caller's working directory before being handed to the bundle tool, which runs with its working directory set to the QLT base path.
There was a problem hiding this comment.
Pull request overview
This PR updates the CodeQL Toolkit (QLT) packaging and install flow to better support codeql-bundle by bundling required package data into the frozen binary, and by exposing a cache directory knob to make bundle downloads/compilation caches reusable across runs (especially CI).
Changes:
- Add
CacheDirsupport in config and pass it through to the bundle tool via--cache-dir, with CLI override support. - Update the
codeql-bundlePyInstaller build script to include runtime package data files when present. - Bump referenced
codeql-bundleversion to0.5.0in docs/workflows and update the example config to CodeQL2.26.1.
Show a summary per file
| File | Description |
|---|---|
| src/CodeQLToolkit.Shared/Utils/QLTConfig.cs | Adds CacheDir to configuration model for bundle-tool cache pass-through. |
| src/CodeQLToolkit.Shared/CodeQL/CodeQLInstallation.cs | Threads CacheDir into installation and prepends --cache-dir to bundle tool args. |
| src/CodeQLToolkit.Features/CodeQL/Commands/Targets/InstallCommand.cs | Allows command-line --cache-dir to override config-loaded cache directory. |
| src/CodeQLToolkit.Features/CodeQL/Commands/CodeQLCommandFeature.cs | Adds --cache-dir option wiring for the install command. |
| scripts/build_codeql_bundle_dist.ps1 | Adds PyInstaller --add-data entries for codeql-bundle package data files. |
| example/qlt.conf.json | Updates example CodeQL versions/identifiers to 2.26.1. |
| developer_guide.md | Updates documented build command to use codeql-bundle 0.5.0. |
| .github/workflows/internal-build-release-win64.yml | Updates workflow to build/package codeql-bundle 0.5.0. |
| .github/workflows/internal-build-release-macos64.yml | Updates workflow to build/package codeql-bundle 0.5.0. |
| .github/workflows/internal-build-release-linux64.yml | Updates workflow to build/package codeql-bundle 0.5.0. |
| .github/actions/install-qlt-local/action.yml | Updates local install action to build/package codeql-bundle 0.5.0. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Low
117cae1 to
ae61f4d
Compare
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
src/CodeQLToolkit.Shared/CodeQL/CodeQLInstallation.cs:278
CacheDiris interpolated directly into a singleProcessStartInfo.Argumentsstring. If the value contains quotes/newlines it can break argument quoting or inject additional bundle-tool flags. Also, whitespace-only values should be treated as unset.
Consider normalizing/validating the cache directory before composing bundleArgs (or move to ArgumentList).
if (CacheDir != null && CacheDir.Length > 0)
{
Log<CodeQLInstallation>.G().LogInformation($"Note: Using bundle cache directory `{CacheDir}` ...");
bundleArgs = $"--cache-dir \"{Path.GetFullPath(CacheDir)}\" {bundleArgs}";
}
src/CodeQLToolkit.Features/CodeQL/Commands/CodeQLCommandFeature.cs:45
- This option is declared as
Option<string>(..., () => null, ...), but the handler parameter andInstallCommand.CacheDirare non-nullablestringwhile the project has<Nullable>enable</Nullable>. If the option is omitted,cacheDirwill be null at runtime and will likely produce nullable warnings.
Use Option<string?> (and string? cacheDir / string? CacheDir) or remove the explicit null default and update types accordingly.
var cacheDirOption = new Option<string>("--cache-dir", () => null, "Directory the bundle tool uses for downloaded bundles and compilation caches. Restore it across CI runs (e.g. with actions/cache) to avoid re-downloading and to reuse published compilation caches. Overrides the 'CacheDir' value in qlt.conf.json. If unset, the bundle tool's platform-specific default is used.") { IsRequired = false };
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Low
codeql-bundle only treats a library pack as a customization pack when
`Customizations.qll` lives in `<scope>/<package_name>`, with `-` replaced
by `_`. For a pack named `qlt/cpp-customizations` that means
`qlt/cpp_customizations/Customizations.qll`, but the example kept the
module at `qlt/Customizations.qll`.
The check is a plain path existence test in ResolvedCodeQLPack, so the
pack was silently classified as a plain library pack. Two consequences:
- The customization was never imported into `codeql/cpp-all`, so the
standard query packs did not see it. Only queries that explicitly
imported the module picked it up, which is why the tests passed while
the standard security queries ignored the custom flow source.
- No standard query pack was recompiled, so there was nothing for the
published compilation caches to accelerate.
Move the module to the documented location and update the post-bundle
test to the corresponding module path. codeql-bundle now reports
"Bundling the customization pack qlt/cpp-customizations" and recreates
codeql/cpp-examples and codeql/cpp-queries, and both example tests still
pass against a stock CodeQL 2.15.5.
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
src/CodeQLToolkit.Shared/CodeQL/CodeQLInstallation.cs:278
--cache-diris added by interpolating a quoted full path into a singleArgumentsstring. On Windows, if the provided cache directory ends with a directory separator (e.g.C:\cache\), the trailing\immediately before the closing quote can escape the quote under Windows command-line parsing rules, causing--cache-dirto be parsed incorrectly. Trimming the ending directory separator (afterGetFullPath) avoids this and keeps behavior stable across platforms.
if (CacheDir != null && CacheDir.Length > 0)
{
Log<CodeQLInstallation>.G().LogInformation($"Note: Using bundle cache directory `{CacheDir}` ...");
bundleArgs = $"--cache-dir \"{Path.GetFullPath(CacheDir)}\" {bundleArgs}";
}
- Files reviewed: 11/12 changed files
- Comments generated: 0 new
- Review effort level: Low
codeql-bundle ships customization packs inside the custom bundle with their dependencies stripped, and adds them as a dependency of the standard library pack. That is how it avoids a cycle between the two. The pack sources stay discoverable through the CodeQL workspace though, and workspace packs take precedence over the ones in the distribution, so the source copy shadows the bundled one and puts the cycle back. Every 'codeql pack install' that reaches the customized standard library then fails with a cyclic dependency error. This never surfaced because the example's customization pack was laid out so that it was classified as an ordinary library pack and never injected. Move the sources of packs marked 'Bundle' aside while installing query packs in bundle mode and restore them afterwards, including on the failure path. Bump the example to 2.26.1 as well. Now that its customization pack is really applied, the standard query packs are genuinely recompiled, and 2.26.1 is covered by the published compilation cache while 2.15.5 is not.
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (7)
src/CodeQLToolkit.Features/Query/Commands/Targets/InstallQueryPacksCommandTarget.cs:144
- HideBundledPackSources is not exception-safe: if File.Move throws after some files have already been moved, the method never returns and the caller’s finally block won’t restore the already-hidden packs (because hiddenPacks never got assigned). Also, File.Move will throw if a stale "*.qlt-bundled" file already exists. Consider making the method restore on failure and overwriting any existing hidden file.
private static List<string> HideBundledPackSources(string[] packFiles)
{
var hidden = new List<string>();
foreach (var packFile in packFiles)
{
var hiddenPath = packFile + BundledPackSuffix;
Log<InstallQueryPacksCommandTarget>.G().LogInformation($"Temporarily hiding {packFile} because the pack is provided by the custom bundle...");
File.Move(packFile, hiddenPath);
hidden.Add(hiddenPath);
}
src/CodeQLToolkit.Features/Query/Commands/Targets/InstallQueryPacksCommandTarget.cs:100
- The pack file path is injected into ProcessStartInfo.Arguments without quoting. If the workspace path contains spaces, this will be parsed as multiple arguments and
codeql pack installwill fail.
process.StartInfo.Arguments = $"pack install {file}";
example/qlt.conf.json:6
- The PR description states the example
qlt.conf.jsonstays pinned to CodeQL 2.15.5, but this change updates it to 2.26.1. Please either keep the example pinned as described or update the PR description/intent so they match.
"CodeQLCLI": "2.26.1",
"CodeQLStandardLibrary": "codeql-cli/v2.26.1",
"CodeQLCLIBundle": "codeql-bundle-v2.26.1",
"EnableCustomCodeQLBundles": true,
"CodeQLStandardLibraryIdent": "codeql-cli_v2.26.1",
example/cpp/stuff/src/qlpack.yml:9
- This example pins
codeql/cpp-allto "12.0.0", which is a major departure from the0.x.yversioning used elsewhere in this repo (e.g. src/CodeQLToolkit.Features/Templates/Query/cpp/qlpack-query.liquid:7 and test/CodeQLToolkit.Shared.Tests/Utils/PackReaderTests.cs:24). If this is a typo,codeql pack installwill fail to resolve dependencies. Please confirm the intendedcodeql/cpp-allversion for the chosen CodeQL CLI and update accordingly.
codeql/cpp-all: "12.0.0"
example/cpp/stuff2/src/qlpack.yml:9
- This example pins
codeql/cpp-allto "12.0.0", which is a major departure from the0.x.yversioning used elsewhere in this repo (e.g. src/CodeQLToolkit.Features/Templates/Query/cpp/qlpack-query.liquid:7 and test/CodeQLToolkit.Shared.Tests/Utils/PackReaderTests.cs:24). If this is a typo,codeql pack installwill fail to resolve dependencies. Please confirm the intendedcodeql/cpp-allversion for the chosen CodeQL CLI and update accordingly.
codeql/cpp-all: "12.0.0"
example/cpp/customizations/src/qlpack.yml:5
- This customization pack pins
codeql/cpp-allto "12.0.0", which is a major departure from the0.x.yversioning used elsewhere in this repo (e.g. src/CodeQLToolkit.Features/Templates/Query/cpp/qlpack-query.liquid:7 and test/CodeQLToolkit.Shared.Tests/Utils/PackReaderTests.cs:24). If this is a typo,codeql pack installwill fail to resolve dependencies. Please confirm the intendedcodeql/cpp-allversion for the chosen CodeQL CLI and update accordingly.
"codeql/cpp-all": "12.0.0"
src/CodeQLToolkit.Features/Query/Commands/Targets/InstallQueryPacksCommandTarget.cs:80
- bundledPackFiles resolution repeatedly parses each qlpack.yml (CodeQLPackReader.read) once per configuration entry, which can get expensive in large workspaces. Read each pack file once and compare against a precomputed set of bundled pack names.
This issue also appears in the following locations of the same file:
- line 100
- line 132
bundledPackFiles = allFiles.Where(f =>
config.CodeQLPackConfiguration.Any(p => CodeQLPackReader.read(f).Name == p.Name && p.Bundle == true)
).ToArray();
- Files reviewed: 22/23 changed files
- Comments generated: 0 new
- Review effort level: Low
Makes QLT work with
advanced-security/codeql-bundlev0.5.0's cache-aware bundles, and fixes two bugs in the custom-bundle path that turned up along the way.Bundling codeql-bundle's data files into the frozen binary
Version 0.5.0 of codeql-bundle ships two JSON files inside the package (
supported-codeql-bundles.jsonand its schema) and reads them at runtime viaimportlib.resources. PyInstaller only picks up Python modules, so the frozen binary was missing them.The build script now passes both files with
--add-data. Since the script runs on Linux, macOS and Windows, the separator comes from[System.IO.Path]::PathSeparatorinstead of being hardcoded. The files are guarded withTest-Pathso older codeql-bundle versions still build, and the argument list gets echoed to the build log for debugging.The pinned version goes from 0.4.1 to 0.5.0 everywhere it appears: the release workflows,
install-qlt-local, and the developer guide.Upstream technically degrades gracefully without these files, but the failure mode is bad: the schema is loaded from the package on every catalog load, so validation fails, the code falls back to an empty catalog, and caching silently does nothing. No error, no caches.
--cache-dirpass-throughqlt codeql run installnow accepts--cache-dirand forwards it to codeql-bundle. CI can point the large bundle and cache downloads at a cacheable location instead of the home directory. Also settable inqlt.conf.json.Example customization pack was in the wrong place
codeql-bundle only treats a pack as a customization pack if it has
<scope>/<name with dashes as underscores>/Customizations.qll. The example hadqlt/Customizations.qllfor a pack namedqlt/cpp-customizations, so it was quietly handled as a plain library pack and never injected into the standard query packs. It had been wrong since Feb 2024. Moved toqlt/cpp_customizations/Customizations.qll.Pack sources shadowing their bundled copies
Fixing the example exposed a real bug in
install-packs --use-bundle. Once a pack is correctly treated as a customization pack, codeql-bundle adds it as a dependency ofcodeql/<lang>-allinside the bundle. But the pack's source directory is still visible throughcodeql-workspace.yml, and workspace packs win over distribution packs, so the source copy shadows the bundled one. Anything depending oncodeql/cpp-allthen fails with:There's no config-only way out:
--additional-packsand--search-pathcan't override workspace precedence, and bundle mode needs the source hidden while stock mode needs it visible.The fix:
InstallQueryPacksCommandTargettemporarily renames theqlpack.ymlof eachBundle-marked pack toqlpack.yml.qlt-bundledduring the install so resolution finds the bundled copy. The files are restored in afinally. BecauseDieWithErrorcallsEnvironment.Exitand skipsfinally, failures instead record the pack, break the loop, restore, and then report. Leftovers from a hard kill get swept on the next run.Nobody hit this before because the reference example was misconfigured, and real repos mark all their packs
Bundle/ReferencesBundle, which filters everything out socodeql pack installnever runs at all.The example config moves to
codeql-bundle-v2.26.1(the published cache catalog starts at 2.18.0), with thecodeql/cpp-allpins and lock files updated to match.Verification
Reproduced the original
FileNotFoundErrorwithout--add-dataand confirmed both files land in the onefile archive after the fix (note thatstringsgives a false negative here since entries are zlib-compressed). Packaging was validated against the real published v0.5.0 artifacts with the real script andgh, no stubs.Ran a real end-to-end install against the live catalog with an empty cache dir: the catalog matched by SHA-256, the asset downloaded and installed,
--compilation-cachereachedcodeql pack create, clean exit. On paired runs differing only by the compilation cache, the cpp-queries recompile got a bit over 2x faster and the whole run roughly 1.7x faster.Also reproduced the dependency cycle against a real v0.5.0 bundle and confirmed the rename fix resolves it, with all packs installing and no leftover
.qlt-bundledfiles. The failure-path restore was verified by injecting a bad dependency.dotnet buildis clean and unit tests pass.One gotcha for anyone benchmarking:
--quick-bundlemaps to-nc, which means--no-precompile, not "disable compilation cache". Testing with-ncwill make the cache look useless.