Skip to content
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

Pre-Trimmer custom step host initial steps #21623

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

jtschuster
Copy link

Start of dotnet/runtime#107211.

This PR adds a custom step runner that runs custom steps before the trimmer. Though much code will likely be shared between here and android custom step runners, it's planned that each runner will be fully contained in their respective repository to allow for customizations and allow each to separately transition from Cecil to System.Reflection.Metadata if desired.

The tools/PreTrimmer/PreTrimmer/CecilExtensions and tools/PreTrimmer/PreTrimmer/ILLink directories include files copied from ILLink with small modifications to the backing collections and unrelated trimmer-specific code. These files provide the same APIs that are present in the ILLink package for the custom steps.

The output of the custom step host is ILLink XML files that can be passed to the trimmer. Currently, a descriptor file is created to pass along marking and preserved type/method information to the trimmer.

PreserveBlockCodeHandler was the first step copied and modified from a MarkHandler to a regular step that can run outside of MarkStep. All other steps are included in the compilation and build without errors, but do are not tested. The sources required for these steps are also included in the PreTrim project, which required creating the PRETRIM compilation symbol and using #ifdefs in the same place as MTOUCH, MMP, and BUNDLER.

Some unit tests have been created for a basic step that marks all metadata, and some for the PreserveBlockCodeHandler. Integration with ILLink hasn't been created or tested.

This can go cleanly in main right now, but I'm not sure if it makes more sense to create a feature branch instead?

Copy link
Contributor

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Windows Integration Tests passed 💻

All Windows Integration Tests passed.

Pipeline on Agent
Hash: d1f1fcba53985c186182b818d19832224fbd56c9 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Tests on macOS M1 - Mac Monterey (12) passed 💻

All tests on macOS M1 - Mac Monterey (12) passed.

Pipeline on Agent
Hash: d1f1fcba53985c186182b818d19832224fbd56c9 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Tests on macOS X64 - Mac Sonoma (14) passed 💻

All tests on macOS X64 - Mac Sonoma (14) passed.

Pipeline on Agent
Hash: d1f1fcba53985c186182b818d19832224fbd56c9 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Tests on macOS M1 - Mac Ventura (13) passed 💻

All tests on macOS M1 - Mac Ventura (13) passed.

Pipeline on Agent
Hash: d1f1fcba53985c186182b818d19832224fbd56c9 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ API diff for current PR / commit

.NET (No breaking changes)

✅ API diff vs stable

.NET (No breaking changes)

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: d1f1fcba53985c186182b818d19832224fbd56c9 [PR build]

@vs-mobiletools-engineering-service2

This comment has been minimized.


<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net8.0</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<TargetFramework>net8.0</TargetFramework>
<TargetFramework>net$(BundledNETCoreAppTargetFrameworkVersion)</TargetFramework>


<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net8.0</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<TargetFramework>net8.0</TargetFramework>
<TargetFramework>net$(BundledNETCoreAppTargetFrameworkVersion)</TargetFramework>

@@ -0,0 +1,5 @@
# PreTrimmer

This tool is meant to replace for illink custom steps. This tool will make modifications to assemblies, generate illink descriptors, and perform any other actions required to prepare assemblies for illink.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This tool is meant to replace for illink custom steps. This tool will make modifications to assemblies, generate illink descriptors, and perform any other actions required to prepare assemblies for illink.
This tool is meant to be a replacement for ILLink custom steps. This tool will make modifications to assemblies, generate trimming descriptors, and perform any other actions required to prepare assemblies for trimming.

#nullable enable

namespace Xamarin.Linker.Steps {
public class PreserveBlockCodeHandler : ConfigurationAwareStep {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of copying and modifying tools/dotnet-linker/Steps/PreserveBlockCodeHandler.cs, would it be possible to modify the existing file, and just use the PRETRIM conditional compilation symbol to modify it?

That makes it easier to see what's the difference is between the two implementations (and also avoids code duplication).

@@ -73,7 +73,7 @@
<ItemGroup>
<ProjectCapability Include="Apple" />
<ProjectCapability Include="Mobile" />

Copy link
Member

Choose a reason for hiding this comment

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

There are numerous unrelated whitespace changes in this file.

@@ -713,11 +713,11 @@
<!-- TODO: these steps should probably run after mark. -->
<_TrimmerCustomSteps Include="$(_AdditionalTaskAssembly)" BeforeStep="MarkStep" Condition="'$(_AreAnyAssembliesTrimmed)' == 'true'" Type="Xamarin.Linker.Steps.PreMarkDispatcher" />
<_TrimmerCustomSteps Include="$(_AdditionalTaskAssembly)" BeforeStep="MarkStep" Type="Xamarin.Linker.ManagedRegistrarStep" Condition="'$(Registrar)' == 'managed-static'" />
<_TrimmerCustomSteps Include="$(_AdditionalTaskAssembly)" BeforeSteps="MarkStep" Condition="'$(_AreAnyAssembliesTrimmed)' == 'true'" Type="Xamarin.Linker.Steps.PreserveBlockCodeHandler" />
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this change, the only difference is the addition of BeforeSteps="MarkStep", it'll still use the same PreserveBlockCodeHandler implementation, not the new one you added.

It seems to be this line should be entirely removed, and then a new MSBuild target that runs before ILLink is needed, which runs the pre-trimmer steps.

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: test results.

🎉 All 101 tests passed 🎉

Tests counts

✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (iOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (MacCatalyst): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (macOS): All 1 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ dotnettests (Multiple platforms): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (tvOS): All 1 tests passed. Html Report (VSDrops) Download
✅ framework: All 2 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 4 tests passed. Html Report (VSDrops) Download
✅ generator: All 3 tests passed. Html Report (VSDrops) Download
✅ interdependent-binding-projects: All 4 tests passed. Html Report (VSDrops) Download
✅ introspection: All 4 tests passed. Html Report (VSDrops) Download
✅ linker: All 40 tests passed. Html Report (VSDrops) Download
✅ monotouch (iOS): All 7 tests passed. Html Report (VSDrops) Download
✅ monotouch (MacCatalyst): All 8 tests passed. Html Report (VSDrops) Download
✅ monotouch (macOS): All 9 tests passed. Html Report (VSDrops) Download
✅ monotouch (tvOS): All 7 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 4 tests passed. Html Report (VSDrops) Download
✅ xtro: All 1 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: d1f1fcba53985c186182b818d19832224fbd56c9 [PR build]

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.

3 participants