Adding OS and arch command line options #18889

sfoslund · 2021-07-13T17:24:57Z

Fixes first bullet of #18832

Description

This change adds --os and --arch options to build, publish, run, and test commands. This is a shorthand syntax for the runtime identifier, where the provided values will be combined with the default RID. For example, on a win-x64 machine, specifying --os os will result in running with the RID set to os-x64 and specifying --arch arch will result in win-arch. Full spec here: https://github.com/dotnet/designs/blob/d248ad68b41866eba17e80225a08518b8202ddf6/accepted/2021/architecture-targeting.md#enable-shorthand-rid-targeting-syntax
These changes are all additive and do not impact the existing CLI.

Regression?

No

Risk

Low, this is a new feature and won't effect existing CLI.

Verification

[X] Manual (required)
[x] Automated

dotnet-issue-labeler · 2021-07-13T17:25:01Z

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.

vitek-karas · 2021-07-15T08:32:20Z

Can you please add a short description of what the change is supposed to do? (Basically a mini-spec for the change)

src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs

sfoslund · 2021-07-15T15:46:02Z

Can you please add a short description of what the change is supposed to do? (Basically a mini-spec for the change)

Sorry, forgot to link the issue/ spec: #18832, this PR covers the first bullet in that issue.

richlander · 2021-07-15T15:54:41Z

@sfoslund -- it would be best to put that information (in response to Vitek) in the PR description. That makes it much easier for others coming to this PR.

src/Cli/dotnet/CommonOptions.cs

richlander · 2021-07-15T16:11:30Z

Modulo my feedback, this looks good. Thanks!

I'm still left wondering how specifying a RID will behave for new scenarios, like dotnet tool install. Will that be a subsequent change?

I'm more than happy to test these changes to validate that they play out correctly in an actual build. If you share a build with me, I'll test it (in a timely manner).

sfoslund · 2021-07-15T17:04:52Z

I'm still left wondering how specifying a RID will behave for new scenarios, like dotnet tool install. Will that be a subsequent change?

Can you clarify what you mean here? I included it in this change because it was in your spec.

richlander · 2021-07-15T17:12:12Z

Can you clarify what you mean here? I included it in this change because it was in your spec.

Yes, it is in the spec. What I mean is, if you type dotnet tool install dotnet-runtimeinfo -a x64, is an x64 apphost generated instead of an Arm64 one (assuming you are using the Arm64 SDK)?

sfoslund · 2021-07-15T17:25:02Z

Yes, it is in the spec. What I mean is, if you type dotnet tool install dotnet-runtimeinfo -a x64, is an x64 apphost generated instead of an Arm64 one (assuming you are using the Arm64 SDK)?

Right now these options aren't hooked up to anything because I wasn't clear based on the spec what the intended behavior was. Is that the intended behavior? If so I can implement it.

richlander · 2021-07-15T17:32:40Z

I could have been more clear/prescriptive on this point. This text (from the spec) is the closest I got to on that.

Tool installation with dotnet tool install is a particularly problematic scenario. At the time .NET 6 is released, one can expect that close to 100% of .NET tools (packaged via NuGet) will target a version prior to .NET 6 and therefore be incapable on running on Arm64 (at least on macOS). However, when installed, the SDK will generate an apphost that matches the SDK RID. As a result, the tool will consistently fail to launch (since the required runtime will be missing). The SDK needs to apply the same implicit RID rules for .NET tools that it will apply for dotnet build. In addition, dotnet tool install should enable explicit RID targeting.

Note: One can think of .NET tools as being distributed as portable apps with no apphost, with the most favorable apphost being provided just-in-time at installation. It's a pretty good model and aligns with the spirit of the overall design.

The same behavior is expected for dotnet test and dotnet run (and dotnet watch, which is a conceptual wrapper over those too).

sfoslund · 2021-07-15T19:10:46Z

The same behavior is expected for dotnet test and dotnet run

I see, dotnet run and test are already enabled by this PR but I'll integrate this into dotnet tool install.

(and dotnet watch, which is a conceptual wrapper over those too).

This PR does not cover dotnet watch as my team doesn't own that code. @captainsafia are you the right person for that?

captainsafia · 2021-07-15T19:44:44Z

This PR does not cover dotnet watch as my team doesn't own that code. @captainsafia are you the right person for that?

@pranavkm can help out with dotnet watch issues.

src/Cli/dotnet/CommonOptions.cs

vitek-karas · 2021-07-16T13:54:21Z

src/Cli/dotnet/CommonOptions.cs

+            return properties;
+        }
+
+        private static string GetCurrentRuntimeId()


I assume we have some logic in the msbuild part of the SDK which already does this. Wouldn't it be cleaner to just set properties based on the command line options and let msbuild handle the actual RID selection then?

Per the spec:

The shorthand syntax is intended as a CLI and not MSBuild concept. For scenarios that today require specifying RIDs in project file, the intent is that users will continue to specify RIDs. If we find that there is a need for a shorthand RID syntax in project files, then we can consider extending this syntax to MSBuild.

We could pass "private" properties to MSBuild if we had to, but if we can I think we should avoid it, as we'd have to prevent them from flowing across project references as global properties the same way we do with RuntimeIdentifier.

Alternatively - if we have existing C# code which does this today, we should share that between MSBuild and CLI. If we don't, maybe we should consider doing so. Otherwise this will be a prime target for unintentional breaks and inconsistencies.

For MSBuild, it's a property (NETCoreSdkRuntimeIdentifier) which we generate into the Microsoft.NETCoreSdk.BundledVersions.props file when we're building the SDK.

That sounds like something we could do for CLI as well - but it would require us to build the CLI as RID specific, which is not the case today.
Alternative idea - can we instead rely on the dotnet.dll location? It would be easier/safer than trying to compute the path. Options:

Simply look "next to dotnet.dll" - not as robust as the other options, but definitely much better than the code here

Through a post-build step bake the RID into the binary. This is technically tricky and could have problems with signing - but it's an option.

Build CLI as RID specific - the best approach I think (we plan to do that eventually anyway for size reasons)

We already also use the NETCoreSdkRuntimeIdentifierChain.txt file for the workload MSBuild SDK resolver. So I think it's fine to rely on that file. I think looking next to dotnet.dll is a great idea to simplify this code, in what ways do you think it wouldn't be robust?

src/Cli/dotnet/CommonOptions.cs

src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs

src/Cli/dotnet/CommonOptions.cs

src/Cli/dotnet/commands/dotnet-run/RunCommandParser.cs

src/Cli/dotnet/commands/dotnet-test/TestCommandParser.cs

src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallCommandParser.cs

Nirmal4G

Some suggestions.

I know that the whole point of having short forms is to write in the CLI faster but the long form should be more beginner friendly and should have a proper word along with its shorter alias. The very long form I suggested could be useful in scripts which could make them more readable and better understandable.

src/Cli/dotnet/CommonOptions.cs

sfoslund · 2021-07-19T20:04:15Z

@dsplaisted @vitek-karas I've updated based on your feedback, let me know if you have any further suggestions. @richlander I'm leaving dotnet tool install out of this PR for now as that command will be more complex. I'll make a separate PR once this is merged.

src/Cli/dotnet/CommonOptions.cs

vitek-karas · 2021-07-20T20:27:50Z

src/Cli/dotnet/CommonOptions.cs

+        {
+            var dotnetRootPath = Path.GetDirectoryName(Environment.ProcessPath);
+            // When running under test the path does not always contain "dotnet" and Product.Version is empty.
+            dotnetRootPath = dotnetRootPath.Contains("dotnet") ? dotnetRootPath : Path.Combine(dotnetRootPath, "dotnet");


I still think that doing path.Contains("dotnet") is not the best. If I clone the SDK repo to F:\dotnet\sdk (which is actually where I have it on my machine), that condition will be true for everything from the repo - including the testhost which comes from the repo-local SDK. But that doesn't mean that the path will point to dotnet.exe.

Updated to check the file name in the latest commit.

sfoslund · 2021-07-20T22:59:36Z

@dsplaisted we'd like to get this in for preview 7, can I get a review so we can merge?

dsplaisted

I think we should also have some coverage of the new options that work by actually starting a dotnet process, since the code to get the current RuntimeIdentifier works differently when running as an in-process unit test.

dsplaisted · 2021-07-22T21:00:07Z

src/Cli/dotnet/commands/dotnet-run/RunCommandParser.cs

+            command.AddOption(CommonOptions.ArchitectureOption());
+            command.AddOption(CommonOptions.OperatingSystemOption());


Does does the fact that these options have a forwarding function interfere here, since the runtime is passed to the RunCommand as a parameter instead of as additional MSBuild arguments like the other commands?

We account for these options here: https://github.com/dotnet/sdk/pull/18889/files#diff-c59ee7ae48403cce44671ddc3549eb921e7c23bda26840c883b29d9f7e7942e5R38, the forwarding function shouldn't interfere.

wli3 · 2021-07-23T19:17:34Z

I could have been more clear/prescriptive on this point. This text (from the spec) is the closest I got to on that.

Tool installation with dotnet tool install is a particularly problematic scenario. At the time .NET 6 is released, one can expect that close to 100% of .NET tools (packaged via NuGet) will target a version prior to .NET 6 and therefore be incapable on running on Arm64 (at least on macOS). However, when installed, the SDK will generate an apphost that matches the SDK RID. As a result, the tool will consistently fail to launch (since the required runtime will be missing). The SDK needs to apply the same implicit RID rules for .NET tools that it will apply for dotnet build. In addition, dotnet tool install should enable explicit RID targeting.

Note: One can think of .NET tools as being distributed as portable apps with no apphost, with the most favorable apphost being provided just-in-time at installation. It's a pretty good model and aligns with the spirit of the overall design.

The same behavior is expected for dotnet test and dotnet run (and dotnet watch, which is a conceptual wrapper over those too).

@richlander. I suggest we take a pause for now and discuss further in the meeting.

that close to 100% of .NET tools (packaged via NuGet) will target a version prior to .NET 6 and therefore be incapable on running on Arm64

That is always the case, .NET5 tools cannot run on .NET6, .NET3.1 tools cannot run on .NET5. There is nothing special on arm64. Even if we generate the right rid apphost, it will still fail since runtime does not move forward.

And if we generate different apphost on the PATH, will the shell resolve both architecture executable for the user?

And the engineering work is not trivial at all for the work. RID select involves reading the read graph which global tools does not do. We also need to consider prepackage apphost in addition at SDK provided non-signed apphost.

wli3 · 2021-07-23T19:35:27Z

Modulo my feedback, this looks good. Thanks!

I'm still left wondering how specifying a RID will behave for new scenarios, like dotnet tool install. Will that be a subsequent change?

I'm more than happy to test these changes to validate that they play out correctly in an actual build. If you share a build with me, I'll test it (in a timely manner).

And what is the benefit of allowing user to specific a rid? Tools are framework-dependent, and once it is on user's machine, all we need to do is to find a convenient way to run it. Unlike website-docker issue, SDK control both end of the deployment.

richlander · 2021-07-23T19:47:09Z

Let me turn this around to you. How would this scenario work?

Install .NET Tool (built with .NET Core 3.1) with the .NET 6 Arm64 SDK.
Run tool.

What do you expect to happen?

wli3 · 2021-07-23T19:53:08Z

@richlander

Let me turn this around to you. How would this scenario work?

Install .NET Tool (built with .NET Core 3.1) with the .NET 6 Arm64 SDK.

Run tool.

What do you expect to happen?

Regardless arm64 or not, it will not work. Basically installing .NET Tool (built with .NET Core 3.1) with the .NET 5 SDK. will not work today.

wli3 · 2021-07-23T19:56:26Z

And I talked you about making runtime move forward before, but runtime team decided not to.

Even that worked. I prefer runtime to be able to resolve different runtime architecture from any apphost to any architecture. Tools are just a symptom of not having our framework dependent scenario work for arm64 vs x64 yet

richlander · 2021-07-23T20:40:01Z

Regardless arm64 or not, it will not work. Basically installing .NET Tool (built with .NET Core 3.1) with the .NET 5 SDK. will not work today.

It works fine. Explain why it doesn't.

vitek-karas · 2021-07-23T20:44:35Z

We can't make arm64 apphost run x64 runtime - it's just not possible. On macOS there are some dirty tricks possible which might allow this, but I'm now aware of anything similar on Windows (and I don't think anything like that is planned).
It will work if you run the app via dotnet app.dll (and you pick the right dotnet, but that is another problem on its own).

I think the basic ask here is that dotnet tool install will know about the fact that .NET 3.1 doesn't have arm64 runtime and default to x64 apphost for those tools (assuming 3.1 runtime is installed).

richlander · 2021-07-23T20:45:30Z

Exactly.

wli3 · 2021-07-23T22:07:38Z

Talked to @richlander offline. The immediate issue need to address is that:

Have 6.0 arm installed on the machine. install older tool targeting 3.1. The apphost will be arm based. When the user run the app, the app will tell the user to download 3.1 intel runtime. But due to the apphost, even intel runtime is installed, the user still cannot run the app. And runtime team has not plan to create such apphost can run both arm and non arm application.

Scoped the experience to make the development cheaper:
only allow -architecture, so that the valid user input can only be win-x64, win-arm64, osx-x64, osx-arm (OS is the current machine OS). Then we don't need rid graph. We can derive the apphost package id and version by rid and version (inserted to SDK), and download using SDK nuget API directly.

if targeting < 6.0 runtime, on windows and mac, default rid is win-x64 or osx-x64. If about, the default is native rid. Allow using the override the -architecture (although we need further thinking on scenarios)

Technical note:

There is an issue for resolving "dotnet [space] blah", it would require current SDK to run first to resolve the executable and then process.start a potential different architecture binary. This likely not works. We might ask the user to run dotnet-format instead.

Figure out what the tool is targeting (if < 6.0) is another unknown. Maybe SDK could get it from asset.json by nuget restore. The worst case scenario SDK could parse and read from runtime.config.json

Be aware of the packaged apphost code path. Although first party should retarget or auto roll forward to avoid this situation.

vitek-karas · 2021-07-26T12:20:59Z

Technically there is a way to ask the tool what framework version it needs/wants - sort of. Once we extract it on disk (before the executable creation), we could use native hosting APIs to ask hostfxr to load the app and try to resolve frameworks (given some runtime location, probably the SDK-native location). This would either work (in which case it's probably OK to create executable with the same arch as the SDK) or it would fail, in which case the SDK could either guess that it's downlevel app, or somehow read it from the error reported from hostxr.

But I understand that this would be a lot of work to get right, so probably something to consider for 7 (in which case we could add a new API into hostfxr for this for example).

richlander · 2021-07-26T16:38:19Z

Is this something we could consider now? I know that hostfxr is a risky component to change. That said, we're still working on this scenario. I wonder if there is an opportunity to "do it right" from the beginnining.

vitek-karas · 2021-07-26T20:00:50Z

I think we can get the desired behavior without hostfxr change today (Since we really only have .NET 6 and newer or .NET 5 and older split - so a binary "it worked or not" is enough to decide between the two). The new API would make the code robust to future changes, but that can be done when we actually need it.

Adding a new API to hostfxr is risky and I think we're pass the point in .NET 6 where I would feel comfortable doing it - especially since we don't have a strong reason to do so.

/cc @agocke for context

agocke · 2021-07-26T20:18:34Z

Agreed, I wouldn't consider a hostfxr change at this point. .NET 7, sure, but for now parsing JSON is the best approach.

sfoslund requested review from wli3, joeloff, richlander and dsplaisted July 13, 2021 18:37

marcpopMSFT added the Urgency-Soon label Jul 14, 2021

joeloff reviewed Jul 15, 2021

View reviewed changes

src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetBuildInvocation.cs Outdated Show resolved Hide resolved

richlander reviewed Jul 15, 2021

View reviewed changes

src/Cli/dotnet/CommonOptions.cs Outdated Show resolved Hide resolved

richlander reviewed Jul 15, 2021

View reviewed changes

src/Cli/dotnet/CommonOptions.cs Show resolved Hide resolved

richlander reviewed Jul 15, 2021

View reviewed changes

src/Cli/dotnet/CommonOptions.cs Outdated Show resolved Hide resolved

vitek-karas reviewed Jul 16, 2021

View reviewed changes

dsplaisted reviewed Jul 16, 2021

View reviewed changes

Nirmal4G reviewed Jul 19, 2021

View reviewed changes

src/Cli/dotnet/CommonOptions.cs Show resolved Hide resolved

src/Cli/dotnet/CommonOptions.cs Show resolved Hide resolved

sfoslund force-pushed the OsArchOptions branch from a77b172 to cf134cf Compare July 19, 2021 23:05

vitek-karas reviewed Jul 20, 2021

View reviewed changes

src/Cli/dotnet/CommonOptions.cs Outdated Show resolved Hide resolved

sfoslund force-pushed the OsArchOptions branch 2 times, most recently from f59a080 to 785fd9a Compare July 20, 2021 20:12

vitek-karas reviewed Jul 20, 2021

View reviewed changes

sfoslund changed the base branch from main to release/6.0.1xx-preview7 July 20, 2021 21:16

Update rid file testing logic

5e88e2e

sfoslund force-pushed the OsArchOptions branch from e8920ce to 5e88e2e Compare July 21, 2021 17:37

sfoslund mentioned this pull request Jul 22, 2021

Architecture and OS targeting improvements for .NET 6 #18832

Closed

7 tasks

marcpopMSFT added the Servicing-consider label Jul 22, 2021

jamshedd added Servicing-approved and removed Servicing-consider labels Jul 22, 2021

dsplaisted approved these changes Jul 22, 2021

View reviewed changes

sfoslund merged commit 176b685 into dotnet:release/6.0.1xx-preview7 Jul 22, 2021

sfoslund deleted the OsArchOptions branch July 22, 2021 21:03

richlander mentioned this pull request Aug 12, 2021

New -a and --os are self-contained by default #19706

Closed

wli3 mentioned this pull request Sep 29, 2021

dotnet-ef needs to target 6 dotnet/efcore#27787

Closed

MarcoRossignoli mentioned this pull request Oct 25, 2021

dotnet test --arch X64 doesn't fail on Mac arm64 #21980

Open

richlander mentioned this pull request Jun 15, 2022

Enable FDD + (implicit) RID-specific apps with RuntimeSpecific #26031

Closed

marcpopMSFT mentioned this pull request Jul 27, 2022

questionable file name assumptions baked into SDK #26837

Closed

v-wuzhai pushed a commit that referenced this pull request Apr 22, 2024

Remove ArcadeBuildVertical parameter (#18889)

8c24fe4

ViktorHofer pushed a commit that referenced this pull request May 7, 2024

Remove ArcadeBuildVertical parameter (#18889)

ea25390

		command.AddOption(CommonOptions.ArchitectureOption());
		command.AddOption(CommonOptions.OperatingSystemOption());

Adding OS and arch command line options #18889

Adding OS and arch command line options #18889

Conversation

sfoslund commented Jul 13, 2021 • edited Loading

Description

Regression?

Risk

Verification

dotnet-issue-labeler bot commented Jul 13, 2021

vitek-karas commented Jul 15, 2021

sfoslund commented Jul 15, 2021

richlander commented Jul 15, 2021

richlander commented Jul 15, 2021

sfoslund commented Jul 15, 2021

richlander commented Jul 15, 2021

sfoslund commented Jul 15, 2021

richlander commented Jul 15, 2021

sfoslund commented Jul 15, 2021

captainsafia commented Jul 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nirmal4G left a comment

Choose a reason for hiding this comment

sfoslund commented Jul 19, 2021

vitek-karas Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfoslund commented Jul 20, 2021

dsplaisted left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 commented Jul 23, 2021

wli3 commented Jul 23, 2021

richlander commented Jul 23, 2021

wli3 commented Jul 23, 2021 • edited Loading

wli3 commented Jul 23, 2021

richlander commented Jul 23, 2021

vitek-karas commented Jul 23, 2021

richlander commented Jul 23, 2021

wli3 commented Jul 23, 2021 • edited Loading

vitek-karas commented Jul 26, 2021

richlander commented Jul 26, 2021

vitek-karas commented Jul 26, 2021

agocke commented Jul 26, 2021

sfoslund commented Jul 13, 2021 •

edited

Loading

vitek-karas Jul 20, 2021 •

edited

Loading

wli3 commented Jul 23, 2021 •

edited

Loading

wli3 commented Jul 23, 2021 •

edited

Loading