C#: Cache temporary dependency directory for BMN by mbg · Pull Request #3286 · github/codeql-action

mbg · 2025-11-13T15:17:45Z

Follow-up from #3117. This PR extends the paths we cache for C# BMN with a similar solution as for Java BMN by configuring a particular, stable path for the extractor to store dependencies in.

There are four key parts to this:

If the FF for this feature is enabled, we include an extra path of our choosing in the cache for C#. Note that Actions considers these paths when restoring caches, so no existing caches are compatible with the feature enabled.
If the FF is enabled, we include it in the feature hash that is included in the cache key.
If we are analysing C# in BMN and the FF is enabled, we set a suitable environment variable for the C# extractor. Setting the environment variable for versions of the extractor which do not support it has no effect, so we don't need to check the CLI features / version.
We clean up the directory at the end of an analysis, after we may have stored it in a cache.

This change depends on support for the CODEQL_EXTRACTOR_CSHARP_OPTION_BUILDLESS_DEPENDENCY_DIR environment variable in the C# extractor, but is designed to be safe to merge as-is before that is available.

Risk assessment

For internal use only. Please select the risk level of this change:

Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

Advanced setup - Impacts users who have custom workflows.
Default setup - Impacts users who use default setup.
Code Scanning - Impacts Code Scanning (i.e. analysis-kinds: code-scanning).
Code Quality - Impacts Code Quality (i.e. analysis-kinds: code-quality).
GHES - Impacts GitHub Enterprise Server.

How did/will you validate this change?

Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).
End-to-end tests - I am depending on PR checks (i.e. tests in pr-checks).
Other - Please provide details.

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

Feature flags - All new or changed code paths can be fully disabled with corresponding feature flags.

How will you know if something goes wrong after this change is released?

Telemetry - I rely on existing telemetry or have made changes to the telemetry.
- Dashboards - I will watch relevant dashboards for issues after the release. Consider whether this requires this change to be released at a particular time rather than as part of a regular release.
- Alerts - New or existing monitors will trip if something goes wrong with this change.

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Consider adding a changelog entry for this change.
Confirm the readme and docs have been updated if necessary.

And also clean it up.

Copilot

Pull Request Overview

This PR extends dependency caching for C# build-mode: none analysis by configuring a stable temporary directory path for the C# extractor, similar to the existing Java implementation. The feature is controlled by the CsharpCacheBuildModeNone feature flag and is designed to be safe to merge before the corresponding C# extractor support is available.

Key changes:

Added new feature flag CsharpCacheBuildModeNone with environment variable CODEQL_ACTION_CSHARP_CACHE_BMN
Implemented C# dependency directory caching with conditional inclusion based on feature flag
Extended cleanup logic to remove C# temporary dependency directories alongside Java directories

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/feature-flags.ts	Adds the `CsharpCacheBuildModeNone` feature flag configuration
src/dependency-caching.ts	Implements `getCsharpTempDependencyDir()` and `getCsharpDependencyDirs()` functions, updates `getDependencyPaths` signature to be async, and includes the feature in the cache key hash
src/dependency-caching.test.ts	Adds tests verifying C# dependency directory inclusion based on feature flag state, and tests for feature prefix generation
src/analyze.ts	Sets `CODEQL_EXTRACTOR_CSHARP_OPTION_BUILDLESS_DEPENDENCY_DIR` environment variable when feature is enabled for C# build-mode: none, and threads `features` parameter through extraction functions
src/analyze-action.ts	Passes `features` parameter to `runFinalize()`
src/analyze-action-post.ts	Extends cleanup logic to remove both Java and C# temporary dependency directories
src/analyze-action-input.test.ts	Updates test assertions to account for new `features` parameter in `runFinalize()`
src/analyze-action-env.test.ts	Updates test assertions to account for new `features` parameter in `runFinalize()`
lib/*.js	Generated JavaScript files reflecting TypeScript changes

Copilot · 2025-11-13T15:23:35Z

src/dependency-caching.ts

+/**
+ * Returns an array of paths of directories on the runner that should be included in a dependency cache
+ * for a C# analysis.
+ *
+ * @returns The paths of directories on the runner that should be included in a dependency cache
+ * for a C# analysis.
+ */


[nitpick] Consider adding a note similar to the one in getJavaDependencyDirs documentation (lines 54-56) explaining why this needs to be a function: "It is important that this is a function, because we call getTemporaryDirectory which would otherwise fail in tests if we haven't had a chance to initialise RUNNER_TEMP." This would improve consistency and help future maintainers understand the design decision.

michaelnebel · 2025-11-14T13:47:27Z

@mbg : If the action only requires being able to communicate the dependency directory via an environment variable - then maybe we shouldn't expose the functionality as an extractor option?

mbg · 2025-11-14T13:55:56Z

@mbg : If the action only requires being able to communicate the dependency directory via an environment variable - then maybe we shouldn't expose the functionality as an extractor option?

I have no strong feelings about this and just mirrored what we did for the Java extractor. IIRC the team there explicitly suggested making it an extractor option.

Personally, I see no harm in it being an extractor option, but I am happy for you to make that decision and just let me know what environment variable we should set here

michaelnebel · 2025-11-14T14:20:01Z

@mbg : If the action only requires being able to communicate the dependency directory via an environment variable - then maybe we shouldn't expose the functionality as an extractor option?

I have no strong feelings about this and just mirrored what we did for the Java extractor. IIRC the team there explicitly suggested making it an extractor option.

Personally, I see no harm in it being an extractor option, but I am happy for you to make that decision and just let me know what environment variable we should set here

Let's do the same as for Java for consistency.

michaelnebel

Looks plausible to me, but it is probably a good idea to a get a review from someone that knows more about the actions code.

henrymercer · 2025-11-19T11:21:23Z

src/analyze-action-env.test.ts

+    t.assert(
+      runFinalizeStub.calledOnceWith(
+        sinon.match.any,
+        sinon.match.any,
+        "--threads=-1",
+        "--ram=4992",
+      ),
+    );
+    t.assert(
+      runQueriesStub.calledOnceWith(
+        sinon.match.any,
+        "--ram=4992",
+        "--threads=-1",
+      ),
+    );


Nice! Is it worth also worth putting the RAM and threads arguments in the same order?

Probably -- I would lie if I said I didn't get this wrong after initially copying it from one to the other. Not sure if this PR is the best place to do it though.

henrymercer · 2025-11-19T11:22:53Z

src/dependency-caching.test.ts

+  const features = createFeatures([Feature.CsharpCacheBuildModeNone]);
+
+  const result = await getFeaturePrefix(codeql, features, KnownLanguage.csharp);
+  t.notDeepEqual(result, "");


Minor: why deep equality here?

IIRC I just followed what other tests did for asserting string equality. I didn't check/try whether is would be enough.

henrymercer · 2025-11-19T11:22:59Z

src/dependency-caching.test.ts

+      continue;
+    }
+    const result = await getFeaturePrefix(codeql, features, knownLanguage);
+    t.deepEqual(result, "", `Expected no feature prefix for ${knownLanguage}`);


henrymercer · 2025-11-19T11:24:22Z

src/dependency-caching.ts

+ * Returns an array of paths of directories on the runner that should be included in a dependency cache
+ * for a C# analysis.
+ *
+ * @returns The paths of directories on the runner that should be included in a dependency cache
+ * for a C# analysis.


Nit, here and elsewhere: avoid duplicating doc comments by just keeping the @returns paragraph.

Probably not worth the effort of changing here, but happy to reduce duplication going forward.

michaelnebel · 2025-11-26T10:07:37Z

@mbg : The extractor part has been merged. I suppose it needs to be released before being testable in conjunction with this change?

mbg · 2025-11-26T10:12:19Z

I suppose it needs to be released before being testable in conjunction with this change?

@michaelnebel We can test this with a nightly (or a custom CLI build) in a test repo.

To actually be more widely usable, we do have to release the CLI and this change.

henrymercer

Reapproving after main was merged.

mbg added 5 commits November 13, 2025 13:40

Refactor C# cache content paths into a function

cf8b7a6

Pass FeatureEnablement to getDependencyPaths

d854ba6

Add FF for extra C# cache contents

a47d04c

Include getCsharpTempDependencyDir in C# caches if FF is enabled

ecaa6db

Configure temp dependency dir for C# extractor when FF is enabled

f5f9571

And also clean it up.

mbg self-assigned this Nov 13, 2025

Copilot AI review requested due to automatic review settings November 13, 2025 15:17

mbg requested a review from a team as a code owner November 13, 2025 15:17

mbg requested a review from michaelnebel November 13, 2025 15:17

github-actions bot added the size/M Should be of average difficulty to review label Nov 13, 2025

Copilot started reviewing on behalf of mbg November 13, 2025 15:18 View session

Copilot finished reviewing on behalf of mbg November 13, 2025 15:22

Copilot AI reviewed Nov 13, 2025

View reviewed changes

michaelnebel previously approved these changes Nov 17, 2025

View reviewed changes

henrymercer previously approved these changes Nov 19, 2025

View reviewed changes

Merge branch 'main' into mbg/csharp/more-cache-locations

0155561

mbg dismissed stale reviews from henrymercer and michaelnebel via 0155561 November 26, 2025 10:03

Merge branch 'main' into mbg/csharp/more-cache-locations

3e93966

henrymercer approved these changes Nov 26, 2025

View reviewed changes

mbg merged commit 59ce4c1 into main Nov 26, 2025
241 checks passed

mbg deleted the mbg/csharp/more-cache-locations branch November 26, 2025 14:37

This was referenced Dec 1, 2025

Merge main into releases/v4 #3336

Merged

Merge releases/v4 into releases/v3 #3338

Merged

Conversation

mbg commented Nov 13, 2025

Risk assessment

Which use cases does this change impact?

How did/will you validate this change?

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

How will you know if something goes wrong after this change is released?

Merge / deployment checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

michaelnebel commented Nov 14, 2025

Uh oh!

mbg commented Nov 14, 2025

Uh oh!

michaelnebel commented Nov 14, 2025

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

henrymercer Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

mbg Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

henrymercer Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

mbg Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

henrymercer Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

henrymercer Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

mbg Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

michaelnebel commented Nov 26, 2025

Uh oh!

mbg commented Nov 26, 2025

Uh oh!

henrymercer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants