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 the Implicit SDK RID for RID-Specific Apps by Default #26143

Merged
merged 12 commits into from
Sep 13, 2022

Conversation

nagilson
Copy link
Member

@nagilson nagilson commented Jun 21, 2022

Part 1 of 4 for #26503
Resolves #23539
Resolves #26028

The implicit RID is now inferred under the following options:

SelfContained=true,
PublishReadyToRun=true,
PublishSingleFile=true
PublishAot=true
PublishSelfContained=true.

PublishSelfContained can be added to make a project publish self-contained by default.

Removed code from before where RID is injected in the CLI and moved it into the MS Build target.
Some changes to how self-contained is inferred as well.

To observe changes:

The following worked before w/ Sarah's change but doesn't require extra computation in the CLI anymore:

  •   dotnet publish --self-contained
    
  • dotnet build --self-contained

The following would cause an error before:

  •   Also observe the property if set in .csproj. 
    
  • dotnet publish -p:PublishSingleFile=true

  • dotnet publish -p:PublishReadyToRun=true

  • dotnet publish -p:SelfContained=true

  • dotnet publish -p:PublishAot=true

  • dotnet build -p:SelfContained=true (Note this also works if set in csproj unlike before, like others)

The following did not mean anything before and now causes the correct behavior, validating it works it on publish scenarios through the CLI (and the CLI only ftr):

  • Also check in the .csproj:
    - dotnet publish -p:PublishSelfContained=true

Broken by this PR Until Daniel's PR is merged into MSBuild fixing the bug which causes this to break:

Publishing a rid agnostic library.

What didn't change:

  • dotnet publish -p:PublishTrimmed

TODO:

  • Verify PublishReadyToRun and such actually use implicit RID, improve tests.
  • Handle PublishAot
  • Fix the NetSDK error in case something goes wrong
  • verify publish self contained

@nagilson
Copy link
Member Author

So Rid Agnostic projects are a whole other thing to consider. Working on that right now

@nagilson nagilson changed the title Use the Implicit SDK RID for RID-Specific Apps by Default Use the Implicit SDK RID for RID-Specific Apps by Default + Add PublishSelfContained Jul 20, 2022
@nagilson nagilson marked this pull request as ready for review July 20, 2022 23:45
@nagilson
Copy link
Member Author

Note on that one failing test: SelfContained=false is not supposed to be used with PublishTrimmed and it becomes a bit circular. Already having a discussion about that one.

@richlander
Copy link
Member

Can you document a set of CLI commands that would have had an error previously that now work because of this work? That would help a lot in reviewing your work. I'm not really the one to review all the code.

@nagilson
Copy link
Member Author

nagilson commented Aug 4, 2022

Can you document a set of CLI commands that would have had an error previously that now work because of this work? That would help a lot in reviewing your work. I'm not really the one to review all the code.

EDIT 2: This info is stale now, check the top comment.

EDIT: Copied this to the top for better visibility, should make reviewing easier.
@richlander Note this was tested exclusively in windows environments.

The following worked before w/ Sarah's change but doesn't require extra computation in the CLI anymore:

  •   dotnet publish --self-contained
    
  • dotnet build --self-contained

The following would cause an error before:

  •   Also observe the property if set in .csproj. 
    
  • dotnet publish -p:PublishSingleFile=true

  • dotnet publish -p:PublishReadyToRun=true

  • dotnet publish -p:SelfContained=true

  • dotnet publish -p:PublishAot=true

  • dotnet build -p:SelfContained=true (Note this also works if set in csproj unlike before, like others)

The following did not mean anything before and now causes the correct behavior, validating it works it on publish scenarios through the CLI (and the CLI only ftr):

  • Also check in the .csproj:
  • dotnet publish -p:PublishSelfContained=true

Broken by this PR Until Daniel's PR is merged into MSBuild fixing the bug which causes this to break:

~~- Publishing a rid agnostic library. ~~

What didn't change:

  • dotnet publish -p:PublishTrimmed

@nagilson
Copy link
Member Author

Retargeting to 7.0.100.

@nagilson nagilson changed the base branch from main to release/7.0.1xx August 18, 2022 19:25
@nagilson
Copy link
Member Author

nagilson commented Sep 1, 2022

Marking as draft again as Daniel's change has flown into MSBuild and the SDK. Once I verify this change works with that and agnostic libraries aren't broken anymore, I will remark as open and ping.

@nagilson nagilson marked this pull request as draft September 1, 2022 19:52
@nagilson
Copy link
Member Author

nagilson commented Sep 8, 2022

I am going to try to get the implicit RID in for RC2 by removing PublishSelfContained. For PublishSelfContained, I don't think is going to happen for RC2 as it looks like its going to require another change to msbuild to not break rid agnostic libraries. Removing it from this PR and putting it elsewhere once it's ready.

@nagilson nagilson changed the title Use the Implicit SDK RID for RID-Specific Apps by Default + Add PublishSelfContained Use the Implicit SDK RID for RID-Specific Apps by Default Sep 9, 2022
@nagilson nagilson marked this pull request as ready for review September 9, 2022 16:59
@nagilson
Copy link
Member Author

nagilson commented Sep 9, 2022

@dotnet/domestic-cat May someone please take a quick look at this? Hoping to get it in for RC2.
Pinging domestic cat because I don't seem to be able to ping the sdk team

Copy link
Member

@joeloff joeloff left a comment

Choose a reason for hiding this comment

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

I'm happy with the change.

My only concern would be to maybe reword some of the errors. Right now it reads as A runtime identifier for the property 'PublishAot' couldn't be inferred. and I wonder whether 'PublishAot' property reads better.

@nagilson
Copy link
Member Author

Awesome, thanks @joeloff for your review. Remaining work is looking into one test which appears to fail flakily on helix.

@nagilson nagilson merged commit 281a049 into dotnet:release/7.0.1xx Sep 13, 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
3 participants