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 implicit RID for Publish Properties #26028

Closed
richlander opened this issue Jun 15, 2022 · 10 comments · Fixed by #26143
Closed

Use implicit RID for Publish Properties #26028

richlander opened this issue Jun 15, 2022 · 10 comments · Fixed by #26143
Milestone

Comments

@richlander
Copy link
Member

richlander commented Jun 15, 2022

Use implicit RID for Publish Properties

Based on #23539, we have the opportunities to make additional improvements fit-and-finish improvements. The referenced change enables an implicit RID to be used when a RID is needed, specifically via the CLI like with --self-contained. There several properties where that is equally valuable.

We should extend the same change to:

  • SelfContained
  • PublishAot
  • PublishReadyToRun
  • PublishSingleFile
  • PublishTrimmed

These proposed changes make it obvious that we're missing PublishSelfContained as an option. It's odd that this has never come up. It would enable producing self contained apps, but only for the publish verb, just like the other Publish* properties.

We should add:

  • PublishSelfContained

In retrospect, I've struggled with the lack of PublishSelfContained, but it never occurred to me that we could have such a straightforward and symmetric solution to solve that.

--use-current-runtime

The new implicit RID feature replaces --use-current-runtime. The following two lines are now equivalent.

dotnet build --self-contained
dotnet build --self-contained --use-current-runtime

We should deprecate (and undocument) --use-current-runtime going foward. It no longer has a purpose.

@baronfel @agocke @nagilson @marcpopMSFT

@baronfel
Copy link
Member

baronfel commented Jun 20, 2022

We should do this! The RuntimeInference part especially should be pushed down into the RuntimeInference targets and removed from the CLI's handling - right now the CLI can sometimes implicitly assign the RID via the RuntimeIdentifier property based on user gestures (see #25062 for an example of this) and this mismatch between the CLI and the targets leads to unclear ordering.

@baronfel baronfel removed the untriaged Request triage from a team member label Jun 20, 2022
@baronfel baronfel added this to the 7.0.1xx milestone Jun 20, 2022
@richlander
Copy link
Member Author

I was thinking about --use-current-runtime in relationship to the Visual Studio guidance at #26031. Sadly, I think we may need to keep --use-current-runtime.

That guidance suggests setting RuntimeSpecific and SelfContained properties to false. That's fine.

After talking to @bradygaster, I learned that there were some scenarios that would benefit to RuntimeSpecific = true and SelfContained = false. I believe that. However, there is only one way that I know how to do that, like the following:

dotnet publish -c Release --use-current-runtime --self-contained false

That forced set of settings would be necessary to coerce a project into "Brady mode" if it was configured like this:

<PropertyGroup>
   <SelfContained>true</SelfContained>
   <RuntimeIdentifier>linux-x64</RuntimeIdentifier>
</PropertyGroup>

The key point here is whether RuntimeSpecific should overwrite a RID if one is specified.

Here are some choices:

  1. RuntimeSpecific only specifies a RID when RuntimeIdentifier isn't specified.
  2. RuntimeSpecific always overwrites RuntimeIdentifier.
  3. Specifying RuntimeSpecific and RuntimeIdentifier together is an error.

If we go with the first one, then it seems like we need to keep --use-current-runtime. If we go with the second one, we don't. The last one seems like it would cause problems w/o a lot of value.

If we go with the first one, the property would basically be RuntimeSpecificCurrentRidHighlander. Bad movie reference ("there can only be one").

Anyone have a preference for which of these we should go with?

@nagilson
Copy link
Member

We haven't forgotten this proposal! There have been some brief discussions on which choice, but nothing confirmed yet. I think some folks will reach out to discuss this with you offline when there's bandwidth.

I'm lacking experience to give a good enough final answer. But I lean towards 1. This is under the assumption that if I specify a runtime with -r, -a, or -os, I would not expect that choice to be overridden later because it's been specified very explicitly. But the property appears to more 'hint at a desired behavior' (wanting the published app to decide on a specific RID by itself if one isn't provided) It seems more intuitive if, providing --runtime osx.10.11-x64 overrides the project property, than to provide an osx runtime and then get something not published for osx.10.11-x64.

@richlander
Copy link
Member Author

richlander commented Jun 30, 2022

I also lean towards 1 so seems like we have a deal. I think it has the most intuitive behavior.

We'd need to decide what RuntimeSpecific=false means with -r present. I'd suggest that RuntimeSpecific solely sets defaults, that can be overrriden, which would allow for that case.

I'm trying to remember the case where --use-current-runtime would still be needed. Oh, I remember now. It's this case.

A project file has this:

<RuntimeIdentifier>linux-x64</RuntimeIdentifier>

Whether it also specified RuntimeSpecific doesn't matter.

Via the CLI, you want to override that to linux-arm64 and you are building within an linux-arm64 container image. In that case, docker publish -r linux-arm64 works, but you are forced to write that and have a separate Dockerfile for Linux Arm64. Instead you say, docker publish --use-current-runtime and it works perfectly. You can use that Dockerfile in whatever you environment you want.

Ideally, we could have a shorter alias for that like --ucr. That would be very nice.

Thinking more about this, RuntimeSpecific=true should be a wrapper over this:

<SelfContained>false</SelfContained>
<RuntimeIdentifier>$CurrentRuntime</RuntimeIdentifier>

That means if you specify -r linux-x64 via the CLI, you still have an FDD app, unless you specifically add --self-contained true.

Good?

@nagilson
Copy link
Member

nagilson commented Jul 1, 2022

That sounds good. Big love for simplifying the docker files. Though I'm confused with how this aligns with the other proposals. See the two questions below please, it'll help me understand the next steps.

For #23539

It seems the point of RuntimeSpecific is to opt-in to:
A) setting self contained equal to false (opt in to FDD)
B) using the implicit rid instead of automatically doing that by default

#23539 Had me thinking we were going to do that automatically when the 4 related options are provided (self-contained, publishAot, etc).

Are we doing both? Or are we not going to do that now, and instead make it only the property RuntimeSpecific?

For #26028: (This Proposal)

What is the difference between SelfContained and PublishSelfContained? Is PublishSelfContained a property (not configurable through the CLI) that:
A) sets SelfContained to true, potentially overriding the false value set by the RuntimeSpecific wrapper
B) Leverages the implicit RID unless one is specified
C) only does these things in the event that publish is used, unlike SelfContained?

So TLDR PublishSelfContained sets SelfContained to true by default when publishing under #23539? (meaning the RID is also defaulted)


In addition, we don't deprecate ----use-current-runtime anymore. And we also add --ucr.

@richlander
Copy link
Member Author

richlander commented Jul 2, 2022

You raise some good points/questions. I'll try to address them.

I'm no an expert on MSBuild or on its interactions with the CLI. I'll just described what I see as reasonable UX from a user standpoint.

Scenarios

Let's look at the scenarios (that at least I have in mind).

I want an FDD+RID-specific app all the time

Given: Project file includes <RuntimeSpecific>true</RuntimeSpecific>

RuntimeSpecific is intended for exactly this scenario. build and publish are the same, so I'll just use build.

dotnet build -> equivalent to dotnet build --use-current-runtime --self-contained false

Note: The implicit RID should always be a portable one, like linux-x64. This may differ from RID reported by dotnet --info, like ubuntu20.04-x64.

I want to take a RuntimeSpecific app and use a specific RID (still FDD)

Same as above, but with a (small) twist.

dotnet build -r linux-arm64 -> equivalent to dotnet build -r linux-x64 --self-contained false

I want to take RuntimeSpecific app and make it self contained

Given: Project file includes <RuntimeSpecific>true</RuntimeSpecific>

I really like being able to take a project file and make it do something different than what its (by default) configured for. We should be able to do that with RuntimeSpecific as well.

dotnet build --self-contained --> This should be the same as dotnet build --self-contained true --use-current-runtime -- The fact that RuntimeSpecific is specified should be immaterial at this point. This combination shouldn't be an error since it is a reasonable override. I'd expect /p:SelfContained=true to have the same behavior.

Specifying a specific RID would be a small twist on that.

Using RuntimeSpecific and Publish* properties together

There are now several Publish* properties. This issue is proposing the following:

  • Where Publish* properties require a RID, default to the SDK RID, just as if --use-current-runtime was specified.
  • Where Publish* properties support both FDD and SCD, assume neither. Use whatever is already set/defaulted.

That's mostly straightforward. From my understanding, single file supports both FDD and SCD.

Given: Project file includes <PublishSingleFile>true</PublishSingleFile>

dotnet publish -> Should produce a single file FDD app.

Adding RuntimeSpecific shouldn't change that.

Given: Project file includes <PublishSingleFile>true</PublishSingleFile> and <RuntimeSpecific>true</RuntimeSpecific> and <SelfContained>true</SelfContained>

We have two choices here:

  • Make this an error since RuntimeSpecific isn't doing anything useful.
  • Treat SelfContained as stronger, effectively overwriting RuntimeSpecific.

We can use whatever model we decide for other similar cases. PublishSelfContained is the same case.

Reasoning about PublishSelfContained vs SelfContained vs --self-contained

The difference between PublishSelfContained and SelfContained is that the former is only effective for the publish task and the later is always effective. The --self-contained CLI argument is a wrapper over SelfContained.

Here's an interesting case to reason about:

<SelfContained>false</SelfContained>
<PublishSelfContained>true</PublishSelfContained>

That seems reasonable to me.

This one is potentially more problematic:

<SelfContained>false</SelfContained>
<PublishSelfContained>true</PublishSelfContained>

and

dotnet publish --self-contained false

Perhaps in that case, the user needs to look at the project file and realize that /p:PublishSelfContained=false is required. That would be fine with me.

I'm not sure if that answers all the question, but hopefully these scenarios help.

@tannergooding
Copy link
Member

I'd find it helpful if the "implicit" RID had more detail/depth. For a given platform there is often a "general purpose" and "specific" RID available.

As an example, on Windows there is both win-x64 and win10-x64. On Linux there is linux-x64 (portable), ubuntu-x64, and ubuntu20.04-x64.

"Current runtime" might be construed as the most specific rather than as the "most portable" and likewise "most portable" may sometimes be undesirable or incorrect depending on the application or scenario.

@nagilson
Copy link
Member

nagilson commented Jul 6, 2022

That does answer all of the questions, thanks for taking the time to respond and clarify.

@richlander
Copy link
Member Author

richlander commented Jul 7, 2022

portable

My understanding is that "current runtime" is always/already a portable one and dotnet --info is the most specific one. This is addressed (mostly via links) at dotnet/source-build#2932.

I added a note in my mini spec above. Thanks for calling this out. @tmds also thanks you.

@marcpopMSFT
Copy link
Member

We believe everything but publish self contained is complete here and that is tracked by other issues

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

Successfully merging a pull request may close this issue.

6 participants