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

Add RidSpecific, Make Apps RID Specific By Default & RID Now FDD By Default in 8.0+ #28412

Closed
wants to merge 12 commits into from

Conversation

nagilson
Copy link
Member

@nagilson nagilson commented Oct 7, 2022

For #26031
7.0 backport #28376

Adds RuntimeSpecific:

  1. Makes TFM >= 8.0 Exe apps Infer RID automatically (FDD)
  2. RIDS no longer imply Self-Contained for TFM >= 8.0
  3. Allows opt out of the inferred RID behavior (FDDefault) by setting RuntimeSpecific to false, but no opt-out to not implying SC with RIDs in TFM 8.0
  4. Warns for apps that didn't set SC but set RIDs in TFM < 8 that they need to set SC explicitly to get the same behavior with TFM 8.0. Warning will go away after SC is set, either to true or false.

Based on .NET 8.0 Breaking Changes Notes:

  • Make RID Specific Apps FDD (NOT SC) by default instead of SC
  • Make EXE Apps Rid Specific By Default

Tested Behavior:

  • RidNoLongerImpliesSelfContainedForTFMOverSeven
  • RidImpliesSelfContainedButWarnsIfNotSetForTFMUnderEight
  • RidSpecificMakesExecutableAppsAboveTFMSevenRidSpecificByDefault
  • RidSpecificAllowsOptOutRIDlessAppsOnExecutableAppsForTFMAboveSeven
  • SelfContainedPlusRuntimeIdentifierDoesNotTriggerEightTFMFDDBreakingChangeWarning
  • RuntimeIdentifiersDisablesRuntimeSpecificFDDBehavior

Questions:

  • Why does TargetFrameworkInference sometimes not run? I accounted for that, but don't understand the contexts.
  • Do we care to set a rid with RidSpecific by default if RuntimeIdentifiers is set? ATM, I made it so RuntimeSpecific doesn't do anything if RuntimeIdentifiers is set, nor will apps with RuntimeIdentifiers be FDD.
  • Could we use a more specific RID?

Verify:
Make sure you can still specify -r runtime without it being overriden.
Make sure you can still specify self-contained to true even if you do /p:SelfContained=true.

@build-analysis build-analysis bot mentioned this pull request Oct 8, 2022
2 tasks
@nagilson nagilson changed the title [ForwardPort] 8.0 Breaking Changes 8.0 Breaking Changes Oct 10, 2022
@nagilson nagilson changed the title 8.0 Breaking Changes Make Apps RID Specific By Default & RID Now FDD By Default in 8.0+ Oct 10, 2022
@nagilson
Copy link
Member Author

Still blocked by #28563

@nagilson nagilson changed the title Make Apps RID Specific By Default & RID Now FDD By Default in 8.0+ Add RidSpecific, Make Apps RID Specific By Default & RID Now FDD By Default in 8.0+ Oct 25, 2022
@nagilson
Copy link
Member Author

This is no longer blocked.

@@ -61,6 +61,14 @@ Copyright (c) .NET Foundation. All rights reserved.
<RuntimeIdentifier Condition="'$(PlatformTarget)' == 'x86' or '$(PlatformTarget)' == ''">win7-x86</RuntimeIdentifier>
</PropertyGroup>

<PropertyGroup Condition="'$(RuntimeSpecific)' == '' and '$(_TargetFrameworkVersionWithoutV)' != '' and '$(_TargetFrameworkVersionWithoutV)' >= '8.0' and '$(OutputType)' == 'Exe'">

Choose a reason for hiding this comment

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

Suggested change
<PropertyGroup Condition="'$(RuntimeSpecific)' == '' and '$(_TargetFrameworkVersionWithoutV)' != '' and '$(_TargetFrameworkVersionWithoutV)' >= '8.0' and '$(OutputType)' == 'Exe'">
<PropertyGroup Condition="'$(RuntimeSpecific)' == '' and '$(_TargetFrameworkVersionWithoutV)' != '' and '$(_TargetFrameworkVersionWithoutV)' >= '8.0' and ('$(OutputType)' == 'Exe' or '$(OutputType)' == 'WinExe')">

@nagilson
Copy link
Member Author

We aren't doing this proposal anymore, there is a new proposal

@nagilson nagilson closed this Jan 18, 2023
@teo-tsirpanis
Copy link

Where is that new proposal?

@nagilson
Copy link
Member Author

#29950 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants