[tools] Remove most static state from the tools.#25619
Conversation
The remaining static state is mostly caching of environment variables (which won't change during a process' lifetime). This is necessary, because soon we'll include some of this code in MSBuild tasks, which shouldn't have static state.
|
/azp run |
There was a problem hiding this comment.
Pull request overview
This PR continues the tools refactor to reduce (or eliminate) static state so the same code can be safely reused from MSBuild tasks. Most of the remaining static state becomes either immutable (readonly) or scoped to a specific tool/configuration instance (including per-link-context caches).
Changes:
- Moved a number of previously-static configuration values to
Application/IToolLog(ex: Xcode/SDK/target framework, platform, warning-level state). - Updated linker steps and helpers to use the instance-scoped
Application(App) instead ofDriver/static globals. - Reworked a few caches to avoid process-wide static dictionaries (ex: via
ConditionalWeakTable), and marked shared immutable values asreadonly.
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 |
|---|---|
| tools/mtouch/AssemblyResolver.cs | Makes Cecil ReaderParameters defaults immutable (readonly). |
| tools/linker/CoreTypeMapStep.cs | Removes static cache state; makes CI filter cache instance-scoped. |
| tools/dotnet-linker/Steps/TrimmableRegistrarStep.cs | Uses App.TargetFramework instead of Driver.TargetFramework to avoid static state. |
| tools/dotnet-linker/Steps/ConfigurationAwareStep.cs | Uses ProductException.IsError (App) to correctly classify warnings/errors per-configuration. |
| tools/dotnet-linker/SetupStep.cs | Removes static ErrorHelper.Platform initialization (platform now comes from log/app). |
| tools/dotnet-linker/LinkerConfiguration.cs | Routes configuration through Application and updates warning parsing/reporting to be app-aware; improves “no configuration available” fallback. |
| tools/dotnet-linker/BackingFieldDelayHandler.cs | Scopes cached method bodies per link context via ConditionalWeakTable. |
| tools/common/StringUtils.cs | Marks static quoting state as readonly. |
| tools/common/StaticRegistrar.cs | Switches Xcode version access to App instead of Driver. |
| tools/common/Optimizations.cs | Marks static option metadata arrays as readonly. |
| tools/common/IToolLog.cs | Adds Platform to IToolLog so error prefixing and warning escalation can be log/app scoped. |
| tools/common/Frameworks.cs | Marks sentinel version as readonly. |
| tools/common/FileCopier.cs | Removes Driver.Force dependency from up-to-date checks. |
| tools/common/ErrorHelper.tools.cs | Replaces static global warning-level storage with per-log state (ConditionalWeakTable), and derives error prefix from IToolLog.Platform. |
| tools/common/error.cs | Refactors ProductException severity calculation to be log-aware (IsError (log)), and formats prefix via ErrorHelper.GetPrefix. |
| tools/common/Driver.cs | Moves various values from static Driver into Application (SdkRoot, DeveloperDirectory, Xcode version/product version, framework dir). |
| tools/common/cache.cs | Removes Driver.Force from cache comparisons (now purely content/mtime based). |
| tools/common/Application.cs | Introduces instance properties for target framework/Xcode/SDK state and moves default hidden-warning initialization to the instance. |
| src/bgen/BindingTouch.cs | Implements IToolLog.Platform for bgen so shared error logic can select the right prefix. |
| msbuild/Xamarin.MacDev.Tasks/ErrorHelper.msbuild.cs | Aligns prefix API with tools-side GetPrefix (IToolLog?) signature for the partial ErrorHelper. |
Comments suppressed due to low confidence (1)
tools/common/ErrorHelper.tools.cs:310
- When a warning is promoted to an error (WarnAsError),
mte.IsError (log)may return true, but the printed diagnostic still usesProductException.ToString()which callsToString (null)and will always format promoted warnings aswarning ...instead oferror .... This can confuse MSBuild parsing and users; pass the log intoToStringso severity/prefix are consistent withIsError (log).
if (!error && GetWarningLevel (log, mte.Code) == WarningLevel.Disable)
return false; // This is an ignored warning.
log.LogError (mte.ToString ());
ShowInner (log, e);
| public string FrameworkCurrentDirectory { | ||
| get { | ||
| if (framework_dir is null) | ||
| throw new InvalidOperationException ($"Teh current framework directory hasn't been set."); |
|
Azure Pipelines successfully started running 3 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
✅ [PR Build #1340f20] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #1340f20] Build passed (Build packages) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [PR Build #1340f20] Build passed (Build macOS tests) ✅Pipeline on Agent |
🚀 [CI Build #1340f20] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 193 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
The remaining static state is mostly caching of environment variables (which won't
change during a process' lifetime).
This is necessary, because soon we'll include some of this code in MSBuild tasks,
which shouldn't have static state.