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

Allow Implicit RID Opt Out + Don't Infer RID for non EXE Type Projects #28628

Merged
merged 11 commits into from
Oct 20, 2022

Conversation

nagilson
Copy link
Member

@nagilson nagilson commented Oct 17, 2022

Description

  • Allows UseCurrentRuntimeIdentifier property to opt out of this feature (Implicit RIDS) in case someone doesn't want it
  • Resolves issue where you do the following:

Create a .NET Core project (e,g, Winforms APP)
In same solution, add a Class Library and set it as a project reference to the main project (created in step1)
Navigate to ClickOnce publish wizard, and set all as default
Navigate to Configurations wizard and set it to Framework-Dependent, SingleFile, x86
image

Publish and see failure. Only fails in VS. Also, if you set the properties anywhere besides VS, it won't fail.

Before:
Screenshot_47

After:
Screenshot_48

Why?

  1. PublishSingleFile (PSF) is not supposed to work if you don't set a RID, but it works if you set a RID in the top-level project but then don't flow the RID to the project references.

  2. AppendsRuntimeIdentifierToOutputPath is set to true even with there is no rid in a library if the top level project had a rid, so the library is ridless but tries to append it's nonexistent rid to the output path which used to be OK

  3. We tried to infer a RID because PSF generally requires a RID to exist but the check for the RID doesn't run in PrepareForPublish for Non-Exe projects.

  4. When VS Injects their own publishing properties in the publishing wizard the pathing gets mixed up.

So, we can solve this by only doing that inference for Exe OutputTypes since PSF doesn't require a RID in subprojects. Rainer confirmed with me that it fixed this scenario. We need to take it to tactics now most likely.

cc @rainersigwald @dsplaisted

Customer Impact

Will allow customers to correctly publish libraries in the VS Publishing wizard but also keep the feature of not having to specify a RID when building or publishing selfcontained and friends.

Regression

This is a new feature so not a regression.

Risk

Low because it only lowers the scope of a newer feature and allows opting out of that feature instead of doing it everytime.
But there is a chance it might break something else we haven't thought of. So far, we didn't find anything else.

Testing

See the above Before and after screenshot.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@nagilson nagilson added the Bug label Oct 17, 2022
@rainersigwald
Copy link
Member

There is another possible solution that might help some other scenarios (like the exe-depends-on-exe we talked about): stop PublishSingleFile (and related properties) from flowing to ProjectReferences. However, that's a bigger, riskier change, and I like the focused scope of this fix.

@@ -65,6 +65,7 @@ Copyright (c) .NET Foundation. All rights reserved.
(
'$(RuntimeIdentifier)' == '' and
'$(RuntimeIdentifiers)' == '' and
'$(OutputType)' == 'Exe' and
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to also add an explicit do-not-infer-RID property here? Since it's caused trouble once, it would be nice to be able to offer a solution to users who hit another related problem without requiring servicing.

Copy link
Member

Choose a reason for hiding this comment

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

You probably want to use _IsExecutable or HasRuntimeOutput instead of OutputType here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Making the change, but for my own understanding: may you please explain why either of those is better than OutputType?

Copy link
Member

Choose a reason for hiding this comment

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

The OutputType can also be WinExe, which these properties take into account.

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

Is it possible to add a test for the failing scenario?

@@ -65,6 +65,7 @@ Copyright (c) .NET Foundation. All rights reserved.
(
'$(RuntimeIdentifier)' == '' and
'$(RuntimeIdentifiers)' == '' and
'$(OutputType)' == 'Exe' and
Copy link
Member

Choose a reason for hiding this comment

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

You probably want to use _IsExecutable or HasRuntimeOutput instead of OutputType here.

@nagilson nagilson changed the title Don't Infer RID for non EXE Type Projects Allow Implicit RID Opt Out + Don't Infer RID for non EXE Type Projects Oct 18, 2022
@nagilson
Copy link
Member Author

nagilson commented Oct 18, 2022

cc @baronfel @richlander to potentially comment on how bad or not bad it would be if we removed implicit RIDs for 7.0, in case there is objection to adding this fix and tactics would rather pull the feature for now. We're bringing this to tactics because we found a scenario where this broke VS publishing; VS publishing has their own RID logic. This fixed it but can't be 100% sure there isn't any additional stuff we haven't thought of. Preferably, we keep it.

@nagilson nagilson merged commit 6942fe7 into dotnet:release/7.0.1xx Oct 20, 2022
nagilson added a commit to dotnet/docs that referenced this pull request Oct 21, 2022
Initial Feature was added in 2020 but not documented here: dotnet/sdk#10449
We've added implicit RIDs here: dotnet/sdk#26143
And we've added interaction to toggle implicit RIDs off with UCR here: dotnet/sdk#28628 
And we added the shorthand for ucr here: dotnet/sdk#27971 

Needs to be duplicated for `dotnet publish` as well.
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.

3 participants