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

Use live illink to trim framework #91233

Merged
merged 11 commits into from
Aug 31, 2023
Merged

Use live illink to trim framework #91233

merged 11 commits into from
Aug 31, 2023

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Aug 28, 2023

#90517 added a workaround to trim the framework using the SDK's version of ILLink.Tasks (see #90517 (comment) for context). This removes the workaround and replaces it with logic to use the live build of ILLink.Tasks.

The dependency on ILLink.Tasks is expressed via a magical ProjectReference that @ViktorHofer kindly helped me construct.

Unfortunately I had to add a small extension point to Microsoft.NET.ILLink.Tasks.props, to allow overriding the analyzer props path. This lets it be imported right out of the source tree, instead of from the build output of ILLink.Tasks.

@ghost
Copy link

ghost commented Aug 28, 2023

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: sbomer
Assignees: sbomer
Labels:

area-Infrastructure-libraries

Milestone: -

Also prevent build-native.proj from importing illink.targets.
eng/Subsets.props Outdated Show resolved Hide resolved
eng/illink.targets Outdated Show resolved Hide resolved
- Don't reference ILLink.Tasks from subsets
- Use PackageReference from illink.tasks
- Import props from source directory

  Add extensibility to shipping targets to make this possible.
- Set IsSourceProject false for .proj files
eng/illink.targets Outdated Show resolved Hide resolved
src/libraries/oob.proj Outdated Show resolved Hide resolved
eng/illink.targets Outdated Show resolved Hide resolved
sbomer and others added 2 commits August 30, 2023 20:56
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@sbomer sbomer marked this pull request as ready for review August 30, 2023 23:40
@ViktorHofer
Copy link
Member

ViktorHofer commented Aug 31, 2023

@sbomer I pushed one more commit to your branch to make sure that the tools build with the correct configuration when building from root.

EDIT: Pushed one more as paths weird displayed weirdly in binlogs:

image

We agreed on (at least under src/libraries) to use the Windows directory separator char `\` instead of a forward slash. This helps when looking at binlogs as those then correctly display a path.
@ViktorHofer
Copy link
Member

Built your changes locally with a mixed set of configurations: .\build.cmd clr+tools+libs -c Release -lc Debug /bl. Looked at the binlog and didn't see any double writes introduced by this change.

@ViktorHofer ViktorHofer merged commit faf883d into dotnet:main Aug 31, 2023
184 of 186 checks passed
@lewing
Copy link
Member

lewing commented Sep 1, 2023

This looks like it may be breaking the official build https://dev.azure.com/dnceng/internal/_build/results?buildId=2257152&view=results

@ViktorHofer
Copy link
Member

Seems to be NativeAOT legs only.

@ViktorHofer
Copy link
Member

Fix is here: #91454

@ghost ghost locked as resolved and limited conversation to collaborators Oct 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants