sdk icon indicating copy to clipboard operation
sdk copied to clipboard

Use the Implicit SDK RID for RID-Specific Apps by Default + Add PublishSelfContained

Open nagilson opened this issue 3 years ago • 4 comments

Part 1 of 4 for https://github.com/dotnet/sdk/issues/26503 Resolves https://github.com/dotnet/sdk/issues/23539 Resolves https://github.com/dotnet/sdk/issues/26028 Resolves https://github.com/dotnet/sdk/issues/26445

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.

NOTE:

This breaks rid agnostic libraries until another fix from https://github.com/dotnet/msbuild/pull/6924 is merged in. Do NOT merge this until then.

~~Regarding PublishTrimmed, I think we should change that to only affect net7.0+ projects, discussing that.~~


TODO:

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

nagilson avatar Jun 21 '22 22:06 nagilson

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

nagilson avatar Jul 18 '22 22:07 nagilson

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.

nagilson avatar Jul 21 '22 19:07 nagilson

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.

richlander avatar Jul 30 '22 00:07 richlander

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 avatar Aug 04 '22 17:08 nagilson

Retargeting to 7.0.100.

nagilson avatar Aug 18 '22 19:08 nagilson

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 avatar Sep 01 '22 19:09 nagilson

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 avatar Sep 08 '22 22:09 nagilson

@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

nagilson avatar Sep 09 '22 17:09 nagilson

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

nagilson avatar Sep 12 '22 16:09 nagilson