-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
move ProjectCache out of experimental #12051
Conversation
@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, |
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.
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
toMicrosoft.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 noExperimental
namespace alias or using directive. Use the full namespace (Microsoft.Build.Experimental.ProjectCache.ProjectCacheException
) or add ausing Experimental = Microsoft.Build.Experimental.ProjectCache;
alias.
catch (Exception e) when (e is not ProjectCacheException && e is not Experimental.ProjectCache.ProjectCacheException)
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. |
src/Build/BackEnd/Components/Communications/SerializationContractInitializer.cs
Show resolved
Hide resolved
src/Build/BackEnd/Components/ProjectCache/Experimental/ProjectCacheDescriptor.cs
Outdated
Show resolved
Hide resolved
…m/JanProvaznik/msbuild into dev/janpro/projectcache-attempt-4
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.
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?
src/Build/BackEnd/Components/Communications/SerializationContractInitializer.cs
Show resolved
Hide resolved
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.
LGTM. The listed testing looks sufficient to me. I imagine with this sort of change things would either completely work or completely break.
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