[assembly-preparer] Create a new tool to replace pre-mark custom linker steps.#25652
[assembly-preparer] Create a new tool to replace pre-mark custom linker steps.#25652rolfbjarne wants to merge 13 commits into
Conversation
Extract the static optimization methods from OptimizeGeneratedCodeHandler into a new standalone OptimizeGeneratedCode class. The handler class now only contains the linker-specific scaffolding and delegates to the new class. This makes the optimization logic reusable without depending on the linker's ExceptionalMarkHandler base class. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Refactor the big switch statement in the LinkerConfiguration constructor into a GetConfigurator() method that returns a dictionary of (Load, Save) delegate pairs for each configuration key. This enables both reading configuration from file and writing/serializing the current configuration state. Also add a Save() method that uses the configurator to write out the current configuration state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… AppBundleRewriter. Move FindNSObjectConstructor, FindINativeObjectConstructor, ImplementConstructNSObjectFactoryMethod, ImplementConstructINativeObjectFactoryMethod, and AddTypeInterfaceImplementation from ManagedRegistrarLookupTablesStep to AppBundleRewriter, and update all call sites accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…er steps. Create an 'assembly-preparer' tool that performs assembly pre-processing (currently done by custom linker steps) as a standalone build step, gated behind the opt-in `PrepareAssemblies=true` MSBuild property. Key changes: * New `tools/assembly-preparer/` library with scaffolding that emulates a subset of ILLink's API (this makes it easy to reuse the code for existing custom trimmer step by just running the same steps in the new assembly-preparer tool instead). * New `PrepareAssemblies` MSBuild task and `_PrepareAssemblies` target in Xamarin.Shared.targets. * When `PrepareAssemblies=true`, most custom linker steps/mark handlers are disabled in the trimmer pipeline (conditions added to Xamarin.Shared.Sdk.targets). * Refactor `LinkerConfiguration.cs` with `#if ASSEMBLY_PREPARER` to support both linker and assembly-preparer contexts. * New test project `tests/assembly-preparer/` with tests for the assembly preparation steps. * Add xharness test variation for running with assembly-preparer enabled. * New `tools/ap-launcher/` to invoke the tool from the command line; this is useful during debugging. Contributes towards #17693.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/review |
|
✅ .NET for Apple Platforms PR Reviewer completed successfully! |
There was a problem hiding this comment.
Pull request overview
This PR introduces an opt-in “assembly-preparer” pipeline that moves several pre-mark custom linker behaviors into a standalone assembly pre-processing step, enabled via PrepareAssemblies=true, and wires it into the build/test infrastructure so it can be validated independently of ILLink.
Changes:
- Added a new
tools/assembly-preparer/library (plustools/ap-launcher/) to run and debug assembly preparation outside the trimmer. - Added an MSBuild
PrepareAssembliestask +_PrepareAssembliestarget, and updated the trimmer pipeline to disable/shift several custom steps whenPrepareAssemblies=true. - Added
tests/assembly-preparer/plus new xharness labels/variations and supporting test infrastructure updates.
Reviewed changes
Copilot reviewed 121 out of 122 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/tools.slnx | Add assembly-preparer project |
| tools/mtouch/mtouch.csproj | Link shared tooling sources |
| tools/mtouch/mtouch.cs | Switch to new Application() ctor |
| tools/mtouch/Errors.resx | Add MX1504 message |
| tools/mtouch/Errors.designer.cs | Resource accessor for MX1504 |
| tools/Makefile | Build assembly-preparer |
| tools/linker/RegistrarRemovalTrackingStep.cs | Use OptimizeGeneratedCode helper |
| tools/linker/ObjCExtensions.cs | ASSEMBLY_PREPARER resolver path |
| tools/linker/MonoTouch.Tuner/Extensions.cs | LinkContext alias guards |
| tools/linker/MobileExtensions.cs | Use Any() for attribute check |
| tools/linker/MarkNSObjects.cs | Exclude class in ASSEMBLY_PREPARER |
| tools/linker/CoreTypeMapStep.cs | Nullability + resolver tweaks |
| tools/dotnet-linker/Steps/TrimmableRegistrarStep.cs | Track added assemblies + paths |
| tools/dotnet-linker/Steps/SetBeforeFieldInitStep.cs | Dual-mode step implementation |
| tools/dotnet-linker/Steps/PreserveBlockCodeHandler.cs | Add Cecil IL/Rocks usings |
| tools/dotnet-linker/Steps/ManagedRegistrarStep.cs | Persist/collect UCO trampoline map |
| tools/dotnet-linker/Steps/ManagedRegistrarLookupTablesStep.cs | Collect registrar type post-prepare |
| tools/dotnet-linker/Steps/InlineDlfcnMethodsStep.cs | Namespace selection tweak |
| tools/dotnet-linker/Steps/ExceptionalMarkHandler.cs | Share abr + prep guard |
| tools/dotnet-linker/OptimizeGeneratedCodeStep.cs | Move to OptimizeGeneratedCode |
| tools/dotnet-linker/MarkIProtocolHandler.cs | Use DynamicDependency for protocols |
| tools/dotnet-linker/DotNetResolver.cs | Cache lookup behavior for preparer |
| tools/dotnet-linker/DotNetGlobals.cs | Add System.Linq global using |
| tools/dotnet-linker/dotnet-linker.csproj | Share comparer/registrar + OptimizeGeneratedCode |
| tools/dotnet-linker/Compat.cs | Nullable assembly return type |
| tools/dotnet-linker/BackingFieldDelayHandler.cs | Use OptimizeGeneratedCode helper |
| tools/dotnet-linker/ApplyPreserveAttributeStep.cs | ASSEMBLY_PREPARER behaviors |
| tools/dotnet-linker/ApplyPreserveAttributeBase.cs | Exclude substep in preparer |
| tools/dotnet-linker/AppBundleRewriter.cs | Add helpers + KeepAlive workaround |
| tools/devops/automation/templates/common/configure.yml | Add assembly-processing label config |
| tools/create-dotnet-linker-launch-json/Program.cs | Emit ap-launcher launch.json support |
| tools/common/StaticRegistrar.cs | Disable some logic under preparer |
| tools/common/RegistrarMode.cs | New shared RegistrarMode enum |
| tools/common/Optimizations.cs | Fix variable used in NativeAOT warning |
| tools/common/NullableAttributes.cs | Add NotNull/MaybeNullWhen for !NET |
| tools/common/NormalizedStringComparer.cs | New shared comparer type |
| tools/common/Makefile | New make entrypoint for common |
| tools/common/Make.common | Generate SdkVersions/ProductConstants |
| tools/common/IToolLog.cs | Switch to ProductException diagnostics |
| tools/common/ErrorHelper.tools.cs | Expose warning levels lookup |
| tools/common/error.cs | Add GetWarningLevel helper |
| tools/common/Driver.cs | Make verbosity file tool-specific |
| tools/common/DerivedLinkContext.cs | Add Registrar + GetProductAssembly |
| tools/common/CoreResolver.cs | Add Xamarin.Utils using |
| tools/common/cache.cs | Add ASSEMBLY_PREPARER cache name |
| tools/common/Assembly.cs | Move comparer out + symbols guard |
| tools/common/Application.cs | PrepareAssemblies flags + logging routing |
| tools/assembly-preparer/System_Range.cs | Range polyfill for netstandard2.0 |
| tools/assembly-preparer/System_Index.cs | Index polyfill for netstandard2.0 |
| tools/assembly-preparer/System_Diagnostics_UnreachableException.cs | UnreachableException polyfill |
| tools/assembly-preparer/Scaffolding/Target.cs | Stub Target type |
| tools/assembly-preparer/Scaffolding/LinkContext.cs | LinkContext scaffolding |
| tools/assembly-preparer/Scaffolding/IStep.cs | Step interface scaffolding |
| tools/assembly-preparer/Scaffolding/BaseStep.cs | BaseStep scaffolding |
| tools/assembly-preparer/Scaffolding/AssemblyAction.cs | AssemblyAction scaffolding |
| tools/assembly-preparer/Scaffolding/AnnotationStore.cs | Minimal annotation/override map |
| tools/assembly-preparer/README.md | New tool documentation |
| tools/assembly-preparer/NetStandardExtensions.cs | netstandard polyfill extensions |
| tools/assembly-preparer/Makefile | Build/test targets |
| tools/assembly-preparer/IAssemblyPreparerLog.cs | Assembly-preparer logging interface |
| tools/assembly-preparer/GlobalUsings.cs | Global usings + placeholder namespaces |
| tools/assembly-preparer/DynamicallyAccessedMemberTypes.cs | netstandard DAMT polyfill |
| tools/assembly-preparer/AssemblyPreparer.cs | Core assembly preparation pipeline |
| tools/assembly-preparer/assembly-preparer.slnx | Tool solution |
| tools/assembly-preparer/.vscode/tasks.json | VS Code build task |
| tools/assembly-preparer/.gitignore | Ignore generated csproj.inc |
| tools/ap-launcher/README.md | ap-launcher purpose |
| tools/ap-launcher/Program.cs | Command-line launcher for debugging |
| tools/ap-launcher/Makefile | Build target |
| tools/ap-launcher/ap-launcher.slnx | Launcher solution |
| tools/ap-launcher/ap-launcher.csproj | Launcher project |
| tools/ap-launcher/.vscode/tasks.json | VS Code build task |
| tools/ap-launcher/.gitignore | Ignore build stamp |
| tools/.vscode/tasks.json | Add tools-level make task |
| tests/xharness/TestLabel.cs | Add AssemblyProcessing label |
| tests/xharness/Jenkins/TestVariationsFactory.cs | Add prepare-assemblies variations |
| tests/xharness/Harness.cs | Add assembly-preparer unit test project |
| tests/mtouch/MLaunchTool.cs | Use iOS-specific SDK version |
| tests/msbuild/Xamarin.MacDev.Tasks.Tests/TestHelpers/TestBase.cs | Switch to XcodeLocation |
| tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/MergeAppBundleTaskTest.cs | Switch to XcodeLocation |
| tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/IBToolTaskTests.cs | Switch to XcodeLocation |
| tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/GetAvailableDevicesTest.cs | Switch to XcodeLocation |
| tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/ACToolTaskTest.cs | Switch to XcodeLocation |
| tests/msbuild/Xamarin.MacDev.Tasks.Tests/AssemblySetup.cs | DEVELOPER_DIR from XcodeLocation |
| tests/Makefile | Emit SDK_VERSION vars per platform |
| tests/linker/link all/dotnet/shared.csproj | Add nowarn for MT2003 |
| tests/dotnet/UnitTests/TestBaseClass.cs | Special-case monotouch-test path |
| tests/dotnet/UnitTests/PerformanceTests.cs | Add PrepareAssemblies perf test harness |
| tests/dotnet/UnitTests/AssetsTest.cs | Use ios_sdk_version |
| tests/common/test-variations.csproj | Add prepare-assemblies variation |
| tests/common/DotNet.cs | Track duration in ExecutionResult |
| tests/common/Configuration.cs | SDK version refactor + assembly lists helpers |
| tests/assembly-preparer/ReproTest.cs | Repro roundtrip + binlog debug test |
| tests/assembly-preparer/PreserveSmartEnumConversionsTest.cs | New step test coverage |
| tests/assembly-preparer/PreserveBlockCodeHandlerTests.cs | New step test coverage |
| tests/assembly-preparer/OptimizeGeneratedCodeHandlerTests.cs | New optimization tests |
| tests/assembly-preparer/MarkIProtocolHandlerTests.cs | Protocol preservation tests |
| tests/assembly-preparer/Makefile | Build/test targets |
| tests/assembly-preparer/InlineDlfcnMethodsStepTests.cs | InlineDlfcn test coverage |
| tests/assembly-preparer/GlobalUsings.cs | Test global usings |
| tests/assembly-preparer/BaseClass.cs | Shared test harness |
| tests/assembly-preparer/assembly-preparer.slnx | Test solution |
| tests/assembly-preparer/assembly-preparer-tests.csproj | New test project |
| src/ObjCRuntime/Registrar.cs | Add non-NET null guards |
| src/ObjCRuntime/Registrar.core.cs | Add non-NET null guards |
| src/bgen/BindingTouch.cs | Add warning/error ProductException logging |
| msbuild/Xamarin.Shared/Xamarin.Shared.targets | Add PrepareAssemblies task/target wiring |
| msbuild/Xamarin.MacDev.Tasks/Xamarin.MacDev.Tasks.csproj | Reference assembly-preparer + TFM tweak |
| msbuild/Xamarin.MacDev.Tasks/Tasks/XamarinTask.cs | Route ProductException diagnostics correctly |
| msbuild/Xamarin.MacDev.Tasks/Tasks/PrepareAssemblies.cs | New MSBuild task implementation |
| msbuild/Xamarin.MacDev.Tasks/LoggingExtensions.cs | Add line-numbered log helpers |
| msbuild/Xamarin.MacDev.Tasks/ErrorHelper.msbuild.cs | Remove duplicate msbuild ErrorHelper impl |
| msbuild/Xamarin.MacDev.Tasks/ConsoleToTaskWriter.cs | Detect/flag Console output in tasks |
| msbuild/Xamarin.MacDev.Tasks.slnx | Add assembly-preparer to solution |
| msbuild/Xamarin.Localization.MSBuild/MSBStrings.resx | Add E7178 text |
| msbuild/ILMerge.targets | Merge assembly-preparer into tasks assembly |
| msbuild/.vscode/tasks.json | Add msbuild-level make task |
| dotnet/targets/Xamarin.Shared.Sdk.targets | Integrate PrepareAssemblies into trimmer flow |
Files not reviewed (1)
- tools/mtouch/Errors.designer.cs: Language not supported
| assemblyDefinition.Write (assembly.OutputPath, writerParameters); | ||
| ModuleAttributes m = assemblyDefinition.MainModule.Attributes; |
| try { | ||
| var infos = InputAssemblies.Select (GetAssemblyInfo).ToArray (); | ||
| using var preparer = new AssemblyPreparer (this, infos, OptionsFile?.ItemSpec ?? ""); | ||
| preparer.MakeReproPath = MakeReproPath; | ||
| var rv = preparer.Prepare (out var exceptions); |
| <data name="MX1504" xml:space="preserve"> | ||
| <value>Can not find the product assembly '{0}' in the list of loaded assemblies.</value> | ||
| </data> |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR introduces a new assembly-preparer tool to move assembly pre-processing out of the linker pipeline, which is a significant architectural improvement. The implementation is generally well-structured with good test coverage. However, there are critical issues that must be fixed before merge.
❌ Errors (must fix)
- Null-forgiving operator (
!): Used twice inAssemblyPreparer.cs(lines 173, 190). This operator is banned in this repository and must be replaced with proper null checks.
⚠️ Warnings (should fix)
string.Empty: Use""instead (line 28 in AssemblyPreparer.cs)Array.Empty<T>(): Use[]instead (line 112 in AssemblyPreparer.cs)- Unconditional FileWrites:
$(_UnmanagedCallersOnlyMapPath)added to FileWrites but may not always be created - Missing SessionId: Consider whether PrepareAssemblies task needs
SessionIdandIsMacEnabledfor remote execution from Windows
💡 Suggestions
- ConsoleToTaskWriter is a useful utility — consider making it
internalbut documenting it for potential reuse
✅ Positive aspects
- Well-organized scaffolding layer that emulates ILLink API for code reuse
- Comprehensive test suite with repro support
- Proper MSBuild integration with incremental build support (Inputs/Outputs)
- Good separation of concerns between tool and MSBuild task
- Conditional disabling of linker steps when PrepareAssemblies=true
Verdict: ❌ Request Changes — Fix the null-forgiving operator usage and address the other warnings before merging.
Generated by .NET for Apple Platforms PR Reviewer for issue #25652 · sonnet45 1.6M
Comment /review to run again
| // save assemblies | ||
|
|
||
| foreach (var assembly in Assemblies) { | ||
| var assemblyDefinition = assembly.Assembly!; |
There was a problem hiding this comment.
🤖 ❌ Nullable · YAGNI — The null-forgiving operator (!) is banned in this repository. This is one of the most critical rules.
Replace with a proper null check:
var assemblyDefinition = assembly.Assembly;
if (assemblyDefinition is null) {
exceptions.Add (ErrorHelper.CreateError (99, $"Assembly definition is null for {assembly.InputPath}"));
return false;
}The same issue exists on line 190 with Path.GetDirectoryName (assembly.OutputPath)! — add a null check there too.
Rule: Never use ! (null-forgiving operator)
| <ResolvedFileToPublish Remove="@(_AssembliesToPrepare)" /> | ||
| <ResolvedFileToPublish Include="@(_PreparedAssemblies)" /> | ||
| <FileWrites Include="@(_PreparedAssemblies)" /> | ||
| <FileWrites Include="$(_UnmanagedCallersOnlyMapPath)" /> |
There was a problem hiding this comment.
🤖 $(_UnmanagedCallersOnlyMapPath) to FileWrites unconditionally may cause issues if the file is not always created.
If this file is only created conditionally (e.g., when certain registrar modes are active or optimizations are enabled), the FileWrites inclusion should be conditional too, or the file should always be created (even if empty).
Verify whether _UnmanagedCallersOnlyMapPath is always created during PrepareAssemblies execution.
Rule: FileWrites for intermediate files
| var file = Path.Combine (reproPath, "arguments.txt"); | ||
| if (!File.Exists (file)) | ||
| throw new FileNotFoundException ($"Repro arguments file not found: {file}"); | ||
| return new AssemblyPreparer (ConsoleLog.Instance, Array.Empty<AssemblyPreparerInfo> (), file); |
There was a problem hiding this comment.
🤖 [] instead of Array.Empty<AssemblyPreparerInfo>() per repository conventions.
return new AssemblyPreparer (ConsoleLog.Instance, [], file);Rule: Use [] for empty arrays and not Array.Empty<T>()
|
|
||
| public LinkerConfiguration Configuration => configuration; | ||
|
|
||
| public string MakeReproPath { get; set; } = string.Empty; |
There was a problem hiding this comment.
🤖 "" instead of string.Empty per repository conventions.
public string MakeReproPath { get; set; } = "";Rule: Use "" for empty string and not string.Empty
| #nullable enable | ||
|
|
||
| namespace Xamarin.MacDev.Tasks { | ||
| public class PrepareAssemblies : XamarinTask { |
There was a problem hiding this comment.
🤖 💡 MSBuild tasks — Consider whether this task needs SessionId and IsMacEnabled properties for remote execution support when building from Windows.
Most MSBuild tasks in this repository that perform Mac-specific operations include:
SessionId="$(BuildSessionId)"
Condition="'$(IsMacEnabled)' == 'true'"Since this task processes assemblies and may eventually need to run on the Mac (or already does for certain operations), verify whether remote execution support is needed. If assembly preparation must always happen on the Mac, add these properties and conditions.
Rule: SessionId for remote execution
✅ [PR Build #2fbe855] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #2fbe855] 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 #2fbe855] Build passed (Build macOS tests) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🔥 [CI Build #2fbe855] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 17 tests failed, 203 tests passed. Failures❌ dotnettests tests (MacCatalyst) [attempt 2]1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ linker tests (tvOS) [attempt 2]15 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download ❌ monotouch tests (macOS) [attempt 2]1 tests failed, 23 tests passed.Failed tests
Html Report (VSDrops) Download Successes✅ assembly-processing: 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 |
Create an 'assembly-preparer' tool that performs assembly pre-processing (currently done by custom linker steps) as a standalone build step, gated behind the opt-in
PrepareAssemblies=trueMSBuild property.Key changes:
tools/assembly-preparer/library with scaffolding that emulates a subset of ILLink's API (this makes it easy to reuse the code for existing custom trimmer step by just running the same steps in the new assembly-preparer tool instead).PrepareAssembliesMSBuild task and_PrepareAssembliestarget in Xamarin.Shared.targets.PrepareAssemblies=true, most custom linker steps/mark handlers are disabled in the trimmer pipeline (conditions added to Xamarin.Shared.Sdk.targets).LinkerConfiguration.cswith#if ASSEMBLY_PREPARERto support both linker and assembly-preparer contexts.tests/assembly-preparer/with tests for the assembly preparation steps.tools/ap-launcher/to invoke the tool from the command line; this is useful during debugging.Contributes towards #17693.