Skip to content

move ProjectCache out of experimental #12051

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

JanProvaznik
Copy link
Member

@JanProvaznik JanProvaznik commented Jun 20, 2025

Fixes #11575

Context

ProjectCache should not be in experimental due to being used in production
but we need to maintain backward compatibility with plugins compiled against the previous namespace
internals use the new namespace

Changes Made

adapt ProjectCacheService to work with both namespaces to maintain backward compatibility, loading both kinds of plugins and converting results to the new namespace
copy existing classes to maintain binary compatibility and mark them obsolete

Testing

Tests run on 2 scenarios, an old namespace plugin loaded from assembly file. And in memory plugin with the new namespace. This tests both current behavior and binary compatibility.

Notes

Needs adjustment on VS insertion.

VS side changes: https://devdiv.visualstudio.com/DevDiv/_git/VS/commit/e3f1029a4c04344c0a18ec4b1a6674dfd96e003a
final exp insertion https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/646925

@JanProvaznik
Copy link
Member Author

JanProvaznik commented Jun 20, 2025

@rainersigwald ptal if the strategy of modifying ProjectCacheService to load both kinds of plugins and duplicating the logic with both types looks acceptable.

Locally it seems to work backwards compatibly with the sample plugin, though I'll have to adjust the test class done

@JanProvaznik JanProvaznik marked this pull request as ready for review June 23, 2025 09:43
Copy link
Contributor

@Copilot 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 moves the ProjectCache API out of the experimental namespace into the main Microsoft.Build.ProjectCache namespace while preserving backward compatibility with plugins compiled against the old experimental API.

  • Renamed namespace imports and type references from Microsoft.Build.Experimental.ProjectCache to Microsoft.Build.ProjectCache.
  • Updated ProjectCacheService to detect and interoperate with both new and experimental plugin instances via wrapper adapters.
  • Introduced obsolete wrapper classes under the Experimental folder and updated the project file and serialization contracts to include them.

Reviewed Changes

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

Show a summary per file
File Description
src/MSBuild/XMake.cs Updated catch block to handle new and experimental exceptions
src/Build/Microsoft.Build.csproj Added compilation of experimental compatibility wrappers
src/Build/CompatibilitySuppressions.xml Added suppressions for moved BuildParameters.ProjectCacheDescriptor
src/Build/BackEnd/Components/Communications/SerializationContractInitializer.cs Registered both new and experimental ProjectCacheException
src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs Extended service to load and invoke both plugin types
src/Build/BackEnd/Components/ProjectCache/* Renamed namespaces and added obsolete compatibility types
src/Build/BackEnd/BuildManager/BuildParameters.cs Updated remarks and namespace for ProjectCacheDescriptor
src/Build/BackEnd/BuildManager/BuildManager.cs Replaced experimental calls in shutdown logic
src/Build.UnitTests/ProjectCache/ProjectCacheTests.cs Switched test imports to new namespace
src/Build.UnitTests/BackEnd/TranslationHelpers.cs Handled known moved exception types
src/Build.UnitTests/BackEnd/Scheduler_Tests.cs Updated test imports for scheduler
Comments suppressed due to low confidence (3)

src/Build/CompatibilitySuppressions.xml:4

  • [nitpick] Consider consolidating repeated <Suppression> entries for the same member across frameworks, or add a comment explaining why each is necessary to improve maintainability.
  <Suppression>

src/Build.UnitTests/ProjectCache/ProjectCacheTests.cs:15

  • Tests have been updated to reference the new namespace, but there are no tests covering the backward‐compatibility path for experimental plugins. Consider adding unit tests to exercise the experimental–to–new compatibility adapters.
using Microsoft.Build.ProjectCache;

src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs:958

  • The catch clause references Experimental.ProjectCache.ProjectCacheException but there is no Experimental namespace alias or using directive. Use the full namespace (Microsoft.Build.Experimental.ProjectCache.ProjectCacheException) or add a using Experimental = Microsoft.Build.Experimental.ProjectCache; alias.
                    catch (Exception e) when (e is not ProjectCacheException && e is not Experimental.ProjectCache.ProjectCacheException)

@YuliiaKovalova YuliiaKovalova self-requested a review June 23, 2025 15:47
@JanProvaznik
Copy link
Member Author

there are 7 places in VS where this namespace is used and presumably will have to be adjusted

@YuliiaKovalova
Copy link
Member

there are 7 places in VS where this namespace is used and presumably will have to be adjusted

@JanProvaznik
Copy link
Member Author

there are 7 places in VS where this namespace is used and presumably will have to be adjusted

If I understand correctly that's a specific cache implementation which yes should migrate eventually, but if it's already compiled it needs to remain compatible. VS is different because there is extra logic around the caches as a host.

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Ok, so the VS-side changes are because there's a cache plugin there that dynamically loads another one, that will also need to handle the transition? I can live with that!

LGTM but let's double check with @dfederm -- any additional testing you'd like to see?

Copy link
Contributor

@dfederm dfederm left a comment

Choose a reason for hiding this comment

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

LGTM. The listed testing looks sufficient to me. I imagine with this sort of change things would either completely work or completely break.

@YuliiaKovalova YuliiaKovalova merged commit 4e1f2f2 into dotnet:main Jul 4, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move ProjectCache to non-Experimental
5 participants