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 net8.0 TFM when building with source-build #2150

Merged
merged 2 commits into from
Apr 19, 2023

Conversation

mthalman
Copy link
Member

@mthalman mthalman commented Apr 6, 2023

When attempting to enable PVP flow for the command-line-api, a prebuilt was detected for Microsoft.NET.ILLink.Tasks.7.0.100-1.23062.2. This occurs as a result of the enablement of trimming for the System.CommandLine project.

This is resolved by configuring the repo to target the net8.0 TFM instead of net7.0 when building with source-build.

Let's use this PR to iterate on any requested changes. Once we finalize the changes, I'll create a patch for source-build to use in addition to merging this PR.

cc @mmitche @MichaelSimons @jonsequitur

@@ -11,11 +11,11 @@
<LangVersion>10.0</LangVersion>
</PropertyGroup>

<PropertyGroup Condition="'$(DisableArcade)' == '1'">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this being removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it had no impact. The property being set was identical between the two conditions. If you want it back, can you suggest how it should be defined given these current changes?

Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

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

These changes LGTM. I do have a related question though. I am concerned that repo level prebuilt detection is not working as expected here. Why wasn't the net7.0 shared framework being flagged as a prebuilt prior to this change? Can you follow up on this or log an issue to track this down?

@mmitche
Copy link
Member

mmitche commented Apr 10, 2023

These changes LGTM. I do have a related question though. I am concerned that repo level prebuilt detection is not working as expected here. Why wasn't the net7.0 shared framework being flagged as a prebuilt prior to this change? Can you follow up on this or log an issue to track this down?

Probably because it's using 7.0 SDK: https://github.com/dotnet/command-line-api/blob/main/global.json#L3

@MichaelSimons
Copy link
Member

Probably because it's using 7.0 SDK: https://github.com/dotnet/command-line-api/blob/main/global.json#L3

Of course, that makes sense.

@mthalman
Copy link
Member Author

@jonsequitur - I've reverted the changes to the sample projects so that they use a hard-coded net7.0 TFM. That makes more sense to allow them to be standalone projects that can be copied/reused by customers.

@mthalman
Copy link
Member Author

@jonsequitur - Can you review my latest update? Go ahead and merge if it looks good.

@jonsequitur jonsequitur merged commit 87704ce into dotnet:main Apr 19, 2023
@jonsequitur
Copy link
Contributor

Thanks, @mthalman!

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.

None yet

4 participants