Skip to content

Conversation

@mbg
Copy link
Member

@mbg mbg commented Nov 13, 2025

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.

@mbg mbg self-assigned this Nov 13, 2025
Copilot AI review requested due to automatic review settings November 13, 2025 15:17
@mbg mbg requested a review from a team as a code owner November 13, 2025 15:17
@mbg mbg requested a review from michaelnebel November 13, 2025 15:17
@github-actions github-actions bot added the size/M Should be of average difficulty to review label Nov 13, 2025
Copilot finished reviewing on behalf of mbg November 13, 2025 15:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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

Comment on lines +81 to +87
/**
* 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.
*/
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

[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.

Copilot uses AI. Check for mistakes.
@michaelnebel
Copy link

@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
Copy link
Member Author

mbg commented Nov 14, 2025

@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
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Should be of average difficulty to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants