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

Remove dependencies on non-live illink #91468

Merged
merged 34 commits into from
Oct 5, 2023
Merged

Remove dependencies on non-live illink #91468

merged 34 commits into from
Oct 5, 2023

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Sep 1, 2023

No description provided.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 1, 2023
@ghost ghost assigned sbomer Sep 1, 2023
Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

Can you take a look in targetingpacks.targets and update the KnownILLink item changes to point to the live ILLink?

@sbomer
Copy link
Member Author

sbomer commented Sep 1, 2023

I think we might be able to remove KnownILLinkPack entirely because we shouldn't be using the illink referenced by the SDK anywhere. Going to give this a try.

Microsoft.Extensions.Options.SourceGeneration was enabling
the single-file analyzer, but did not import illink.targets
because it is not a source project.

We could fix this by importing liveILLink.targets instead, but
this introduces more problems with circular dependencies
(generator projects depending on illink depending on
generator projects).
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

First of all: Amazing change!!

Slightly unrelated to this PR but still important from my point of view: Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?

eng/liveILLink.targets Outdated Show resolved Hide resolved
eng/Analyzers.targets Outdated Show resolved Hide resolved
This more closely matches the condition on the import of
illink.targets. When EnableSingleFileAnalyzer is true, we should
also import liveILLink.targets so that we get the live analyzer
bits.

Also address feedback about path separators.
This will make it easier to use different conditions for the two.
Some libraries test projects set PublishTrimmed, so they require
the liveILLink.targets.
@sbomer
Copy link
Member Author

sbomer commented Sep 5, 2023

Is there a feature flag that we can condition the illink.targets import on so that only project that actually use the linker, import the file and with it build the P2P to ILLink.Tasks.csproj?

I'm looking for a clean way to do this, but I'm not sure of the best way yet. _RequiresILLinkPack is effectively the SDK's version of this:
https://github.com/dotnet/sdk/blob/1c9e30a466d2c1f3b4673e3b0adfb02590dc8b74/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L37-L43

liveILLink.targets sets this to false because we don't want to reference the SDK's version of the package. Maybe we should use a similar condition for the liveILLink.targets.

This should let libraries projects (whether they are source or test
projects) use the SDK's trimming properties with a live ILLink, enabling
the ProjectReference only for those projects that need it.

Because illink.targets also imports liveILLink.targets, this import is
guarded by a property to ensure liveILLink.targets is only imported
once on this path.
eng/liveILLink.targets Outdated Show resolved Hide resolved
sbomer and others added 2 commits October 3, 2023 13:44
- Fix indentation
- Only import liveILLink.targets in libraries when illink.targets is imported
- Respect MSBuildRuntimeType for live ILLinkTasksAssembly override

Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Various test configurations (wasm, nativeaot) rely on the live ILLink
when running libraries tests.
@ViktorHofer
Copy link
Member

Looks like there is a remaining failure in the NativeAOT leg .

This target isn't defined in the ILLink.Tasks.csproj ProjectReference.
Try skipping nonexistent targets to fix this.
@sbomer
Copy link
Member Author

sbomer commented Oct 5, 2023

Failures are known (it seems browser-wasm job timed out but didn't report status back to github). I couldn't have got this through without your help @ViktorHofer and @jkoritzinsky - thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants