-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 --use-current-runtime option #14093
Add --use-current-runtime option #14093
Conversation
It has been just over a year since the last comment on #10449, so perhaps worth double checking that this is still valid in 6.0, but it does seem useful to me. I think @wli3 and @dsplaisted would be the best reviewers for this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
- I would prefer calling the switch something like
--use-current-runtime
over--infer-runtime
. @KathleenDollard, thoughts? - I would also prefer having this implemented in MSBuild logic. IE setting
UseCurrentRuntimeIdentifier
to true would set theRuntimeIdentifier
property to the current RID (possibly via theNETCoreSdkRuntimeIdentifier
property). Then the--use-current-runtme
command-line option would setUseCurrentRuntimeIdentifier
to true, but the property could also be used outside of thedotnet
command line.
95a7343
to
ebc5ea2
Compare
@dsplaisted, thanks. I have updated the PR with |
ebc5ea2
to
52dfbb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the piecemeal feedback, but thanks again for your contribution.
src/Tests/Microsoft.NET.Publish.Tests/RuntimeIdentifiersTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Publish.Tests/RuntimeIdentifiersTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Publish.Tests/RuntimeIdentifiersTests.cs
Outdated
Show resolved
Hide resolved
@@ -141,6 +141,9 @@ | |||
<data name="CmdRuntimeOptionDescription" xml:space="preserve"> | |||
<value>The target runtime to restore packages for.</value> | |||
</data> | |||
<data name="CmdCurrentRuntimeOptionDescription" xml:space="preserve"> | |||
<value>Use host runtime to build the target.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<value>Use host runtime to build the target.</value> | |
<value>Use current runtime as the target runtime.</value> |
@KathleenDollard Any further suggestions for this text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in all resx/xlf files. More feedback from @KathleenDollard is welcome. :)
One thing to be aware of here is that NETCoreSdkRuntimeIdentifier will be the "portable" RIDs like:
Is that what we want for
|
@eerhardt I would prefer the portable rids. That's what I pass in to dotnet publish all of the time. |
For source-build, the actual/non-portable RID would make sense here. This lets source-build users use the built-from-source Either option would make sense to me as the meaning of the flag as it's currently named. I think the ambiguity might be a problem in itself. 🤔 +cc @omajid |
In source-build this is actually the non-portable RID, because the SDK is built for a non-portable runtime. For source-build, either portable or non-portable might be desired. Someone could intentionally download the portable runtime/apphost packs to build an app that will run portable. But it's interesting that the behavior of this PR as-is depends on whether the SDK is built portable or non-portable. |
Personally, my main use-case was to simplify deployment scripts and instructions (e.g. https://github.com/nst/JSONTestSuite/blob/master/parsers/test_dotnet_system_text_json/README.md), where portable RID would suffice. An idea: why not both? 😎 |
portable rid would be a meaningful default when publishing a single file.
This doesn't work yet. source-build doesn't produce the artifacts to do a single-file publish for the machine rid. |
|
I meant that maybe we can use portable rid as the default for PublishSingleFile, then we don't need to add a flag and come up with a name. |
IMO, explicit option is better, as it is also used for restore and other commands (beyond |
We need to set the
|
If running a pre-evaluation custom task is an option, we could do something like: using System;
using System.Runtime.InteropServices;
using Microsoft.Build.Framework;
using Microsoft.Build.Utilities;
public class UseCurrentMachineRuntimeIdentifierTask : Task
{
public override bool Execute() => true;
[Output]
public string Value => RuntimeInformation.RuntimeIdentifier;
} <UseCurrentMachineRuntimeIdentifierTask>
<Output TaskParameter="Value" PropertyName="UseCurrentMachineRuntimeIdentifier"/>
</UseCurrentMachineRuntimeIdentifierTask> |
It's not possible. We would have to add something to MSBuild itself (ie a new intrinsic function). |
MSBuild allows for calls to <HostRid Condition="'$(HostRid)' == '' and '$(MSBuildRuntimeType)' == 'core'">$([System.Runtime.InteropServices.RuntimeInformation]::RuntimeIdentifier)</HostRid>
<HostRid Condition="'$(HostRid)' == '' and '$(MSBuildRuntimeType)' != 'core'">win-$([System.Runtime.InteropServices.RuntimeInformation]::OSArchitecture.ToString().ToLowerInvariant)</HostRid> Note that |
'$(OS)' == 'Windows_NT' and | ||
'$(RuntimeIdentifier)' == ''"> | ||
<_UsingDefaultRuntimeIdentifier>true</_UsingDefaultRuntimeIdentifier> | ||
<RuntimeIdentifier Condition="'$(PlatformTarget)' == 'x64'">win7-x64</RuntimeIdentifier> | ||
<RuntimeIdentifier Condition="'$(PlatformTarget)' == 'x86' or '$(PlatformTarget)' == ''">win7-x86</RuntimeIdentifier> | ||
</PropertyGroup> | ||
|
||
<PropertyGroup Condition="'$(UseCurrentRuntimeIdentifier)' == 'true'"> | ||
<RuntimeIdentifier>$(NETCoreSdkRuntimeIdentifier)</RuntimeIdentifier> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a Microsoft SDK, UseCurrentRuntimeIdentifier
is the same as portable rid. So --use-current-runtime
means publish portable.
On a source-build SDK, this is the same as the machine rid. So the result may be a more specific publish that doesn't work portable.
I think this difference will bite source-build SDK users, and it is preferable to have a flag that means portable-rid consistently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know the full background why source-build chose to override it, but.. if the value of NETCoreSdkRuntimeIdentifier
is overridden in source-build repo for set of reasons, do those same reasons not apply to RuntimeIdentifier
?
In other words, --use-current-runtime
's docs are stating "Use current runtime as the target runtime.", the one SDK is executing on. This is what we have implemented. It is not strictly calling out portable-rid
, that's an internal detail of general (non-overridden, non-source-build) case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know the full background why source-build chose to override it, but.. if the value of NETCoreSdkRuntimeIdentifier is overridden in source-build repo for set of reasons, do those same reasons not apply to RuntimeIdentifier?
source-build does it to indicate the resulting artifacts are not portable.
In other words, --use-current-runtime's docs are stating "Use current runtime as the target runtime.", the one SDK is executing on. This is what we have implemented. It is not strictly calling out portable-rid, that's an internal detail of general (non-overridden, non-source-build) case.
The main use-case is to enable easy publish for the portable rid.
If this flag enables that on Microsoft sdk, that is what it will be used for.
So I'd like this to have the same behavior on source-build sdk, and we can update the docs/implementation accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my perspective, the source-build scenario was being discussed earlier, and this proposal came up that I think was very nice (except maybe the word "machine" in particular, since it doesn't line up with any other concept I'm aware of):
--use-current-runtime
,UseCurrentRuntimeIdentifier
for portable RID.
--use-current-machine-runtime
,UseCurrentMachineRuntimeIdentifier
for non-portable RID.
Docs could then consistently use --use-current-runtime
, and source-build users can elect to use the non-portable RID if they so choose.
But then this PR was merged without resolving that discussion, abruptly--it seems to me. 😕 (There were also no approvals.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except maybe the word "machine"
It was adapted from @eerhardt's question #14093 (comment) (and maybe derived from the fact that 'machine config' is a thing in .NET Framework) 😉
@am11 Did we never document this feature publically? Like https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-build, and the publish page, or somewhere like that? |
FYI dotnet/docs#31970 adding it here. @am11. We also changed it to become an opt out for implicit rids. |
Thanks.
PR added |
We forget to document stuff all the time. Don't sweat it, I agree it would be better if we had something in place for this. |
It is a common use-case to use the same runtime during the build and publish. During the development of .NET apps, it is painful to remember the entire - ever growing -
--runtime <baseos>-<arch>
matrix.--use-current-runtime
, backed by MSBuild propertyUseCurrentRuntimeIdentifier=True/False
, will allow users to keep things neutral in their documentation and deployment scripts.Fixes #10449
cc @eerhardt @tmds, @dagood