Support non-lock files for C# cache key computation #3117
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,6 +87,54 @@ async function makePatternCheck(patterns: string[]): Promise<string[]> { | |
| return patterns; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the list of glob patterns that should be used to calculate the cache key hash | ||
| * for a C# dependency cache. | ||
| * | ||
| * @param codeql The CodeQL instance to use. | ||
| * @param features Information about which FFs are enabled. | ||
| * @returns A list of glob patterns to use for hashing. | ||
| */ | ||
| async function getCsharpHashPatterns( | ||
| codeql: CodeQL, | ||
| features: Features, | ||
| ): Promise<string[]> { | ||
| // These files contain accurate information about dependencies, including the exact versions | ||
| // that the relevant package manager has determined for the project. Using these gives us | ||
| // stable hashes unless the dependencies change. | ||
| const basePatterns = [ | ||
| // NuGet | ||
| "**/packages.lock.json", | ||
| // Paket | ||
| "**/paket.lock", | ||
| ]; | ||
| const globber = await makeGlobber(basePatterns); | ||
|
|
||
| if ((await globber.glob()).length > 0) { | ||
| return basePatterns; | ||
| } | ||
|
|
||
| if (await features.getValue(Feature.CsharpNewCacheKey, codeql)) { | ||
| // These are less accurate for use in cache key calculations, because they: | ||
| // | ||
| // - Don't contain the exact versions used. They may only contain version ranges or none at all. | ||
| // - They contain information unrelated to dependencies, which we don't care about. | ||
| // | ||
| // As a result, the hash we compute from these files may change, even if | ||
| // the dependencies haven't changed. | ||
| return makePatternCheck([ | ||
| "**/*.csproj", | ||
| "**/packages.config", | ||
| "**/nuget.config", | ||
| ]); | ||
| } | ||
|
Comment on lines
+117
to
+130
There was a problem hiding this comment. [nitpick] The comment explains the limitations of using non-lock files but doesn't mention the cache key compatibility concern raised in the PR description. Consider adding a comment about cache invalidation when this feature is enabled/disabled to ensure the behavior is documented in the code. Copilot uses AI. Check for mistakes. |
||
|
|
||
| // If we get to this point, the `basePatterns` didn't find any files, | ||
| // and `Feature.CsharpNewCacheKey` is either not enabled or we didn't | ||
| // find any files using those patterns either. | ||
| throw new NoMatchingFilesError(); | ||
|
There was a problem hiding this comment. I don't have all the context here, but perhaps it would be clearer to return |
||
| } | ||
|
|
||
| /** | ||
| * Default caching configurations per language. | ||
| */ | ||
|
|
@@ -108,13 +156,7 @@ const defaultCacheConfigs: { [language: string]: CacheConfig } = { | |
| }, | ||
| csharp: { | ||
| getDependencyPaths: () => [join(os.homedir(), ".nuget", "packages")], | ||
| getHashPatterns: getCsharpHashPatterns, | ||
| }, | ||
| go: { | ||
| getDependencyPaths: () => [join(os.homedir(), "go", "pkg", "mod")], | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hvitved : Can you think of other files that are relevant to include i cache-key computations?
It is probably worth noting that we might risk using "bad" caches when version (or wildcard) ranges are used (in case the available version(s) of a dependency changes in a NuGet feed). Is it correctly understood that the only impact of this is that the "right" package version is just not available in the cache (and will be downloaded)?
Related question: For how long are caches stored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps
**/global.jsonand**/*.sln?Actually, as far as I can tell, our dependency fetching logic will look in all small non-binary files for lines like
<PackageReference Include="..." />, and then try to restore those dependencies. Obviously, we cannot meaningfully include all files in the hash, so perhaps we will just have to ignore this case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there is a balance here between capturing all changes to dependencies and the ideal case, where the hash only changes if there are dependencies added or removed, or the exact versions that are in use change. If we hashed all "small non-binary" files, for example, we'd probably be uploading a new cache for every run. The main problem with that is how much of the cache quota we'd end up using.
The main goal here is to produce dependency caches for more C# projects in (primarily) Default Setup, where it's not possible to add custom caching steps.
We could port the logic to check all "small non-binary" files for lines that look like they include dependency information into the Action to determine which files to hash for this, but I wouldn't be too keen on that duplication and it's probably beyond the scope of this change. For more advanced logic like that, it would probably make sense to revisit the parts of the original EDR for this that talk about having the CLI communicate the caching configuration to the Action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with all of the above, I just wanted to point out this corner case in the dependency fetching logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was helpful! Thank you ❤️
I only wanted to add the extra context for everyone's benefit.
For
**/global.jsonand**/*.sln, do you know off the top of your head how common it is for them to contain dependency information?For
global.jsonfiles, I have only seen them used to indicate the .NET version - which we wouldn't want to cache (that should ideally be handled by the toolcache) and which I don't think affects the dependencies that are downloaded? Although, I imagine it would be possible to conditionally include dependencies based on the .NET version in use.For
*.slnfiles, I have only seen those include other project files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right; let's skip
global.jsonfiles and*.slnfiles.