-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Adding OS and arch command line options #18889
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,10 @@ | |
| using System.CommandLine; | ||
| using System.IO; | ||
| using Microsoft.DotNet.Tools.Common; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using Microsoft.DotNet.Cli.Utils; | ||
| using System.CommandLine.Parsing; | ||
|
|
||
| namespace Microsoft.DotNet.Cli | ||
| { | ||
|
|
@@ -35,7 +39,7 @@ public static Option FrameworkOption(string description) => | |
| description) | ||
| { | ||
| ArgumentHelpName = CommonLocalizableStrings.FrameworkArgumentName | ||
|
|
||
| }.ForwardAsSingle(o => $"-property:TargetFramework={o}") | ||
| .AddSuggestions(Suggest.TargetFrameworksFromProjectFile()); | ||
|
|
||
|
|
@@ -91,6 +95,18 @@ public static Option InteractiveOption() => | |
| "--interactive", | ||
| CommonLocalizableStrings.CommandInteractiveOptionDescription); | ||
|
|
||
| public static Option ArchitectureOption(bool includeShortVersion = true) => | ||
| new ForwardedOption<string>( | ||
| includeShortVersion ? new string[] { "--arch", "-a" } : new string[] { "--arch" }, | ||
| CommonLocalizableStrings.ArchitectureOptionDescription) | ||
| .SetForwardingFunction(ResolveArchOptionToRuntimeIdentifier); | ||
|
|
||
| public static Option OperatingSystemOption() => | ||
| new ForwardedOption<string>( | ||
| "--os", | ||
| CommonLocalizableStrings.OperatingSystemOptionDescription) | ||
sfoslund marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| .SetForwardingFunction(ResolveOsOptionToRuntimeIdentifier); | ||
|
|
||
| public static Option DebugOption() => new Option<bool>("--debug"); | ||
|
|
||
| public static bool VerbosityIsDetailedOrDiagnostic(this VerbosityOptions verbosity) | ||
|
|
@@ -100,6 +116,74 @@ public static bool VerbosityIsDetailedOrDiagnostic(this VerbosityOptions verbosi | |
| verbosity.Equals(VerbosityOptions.d) || | ||
| verbosity.Equals(VerbosityOptions.detailed); | ||
| } | ||
|
|
||
| internal static IEnumerable<string> ResolveArchOptionToRuntimeIdentifier(string arg, ParseResult parseResult) | ||
| { | ||
| if (parseResult.HasOption(RuntimeOption(string.Empty).Aliases.First())) | ||
| { | ||
| throw new GracefulException(CommonLocalizableStrings.CannotSpecifyBothRuntimeAndArchOptions); | ||
| } | ||
|
|
||
| if (parseResult.BothArchAndOsOptionsSpecified()) | ||
| { | ||
| // ResolveOsOptionToRuntimeIdentifier handles resolving the RID when both arch and os are specified | ||
| return Array.Empty<string>(); | ||
sfoslund marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| return ResolveRidShorthandOptions(null, arg); | ||
| } | ||
|
|
||
| internal static IEnumerable<string> ResolveOsOptionToRuntimeIdentifier(string arg, ParseResult parseResult) | ||
| { | ||
| if (parseResult.HasOption(RuntimeOption(string.Empty).Aliases.First())) | ||
| { | ||
| throw new GracefulException(CommonLocalizableStrings.CannotSpecifyBothRuntimeAndOsOptions); | ||
| } | ||
|
|
||
| if (parseResult.BothArchAndOsOptionsSpecified()) | ||
| { | ||
| return ResolveRidShorthandOptions(arg, parseResult.ValueForOption<string>(CommonOptions.ArchitectureOption().Aliases.First())); | ||
| } | ||
|
|
||
| return ResolveRidShorthandOptions(arg, null); | ||
| } | ||
|
|
||
| private static IEnumerable<string> ResolveRidShorthandOptions(string os, string arch) | ||
| { | ||
| var properties = new string[] { $"-property:RuntimeIdentifier={ResolveRidShorthandOptionsToRuntimeIdentifier(os, arch)}" }; | ||
| return properties; | ||
| } | ||
|
|
||
| internal static string ResolveRidShorthandOptionsToRuntimeIdentifier(string os, string arch) | ||
| { | ||
| var currentRid = GetCurrentRuntimeId(); | ||
| os = string.IsNullOrEmpty(os) ? GetOsFromRid(currentRid) : os; | ||
| arch = string.IsNullOrEmpty(arch) ? GetArchFromRid(currentRid) : arch; | ||
| return $"{os}-{arch}"; | ||
| } | ||
|
|
||
| private static string GetCurrentRuntimeId() | ||
sfoslund marked this conversation as resolved.
Show resolved
Hide resolved
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per the spec:
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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For MSBuild, it's a property (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already also use the |
||
| { | ||
| var dotnetRootPath = Path.GetDirectoryName(Environment.ProcessPath); | ||
| // When running under test the path does not always contain "dotnet" and Product.Version is empty. | ||
| dotnetRootPath = Path.GetFileName(dotnetRootPath).Contains("dotnet") ? dotnetRootPath : Path.Combine(dotnetRootPath, "dotnet"); | ||
sfoslund marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| var ridFileName = "NETCoreSdkRuntimeIdentifierChain.txt"; | ||
| string runtimeIdentifierChainPath = string.IsNullOrEmpty(Product.Version) ? | ||
sfoslund marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Path.Combine(Directory.GetDirectories(Path.Combine(dotnetRootPath, "sdk"))[0], ridFileName) : | ||
| Path.Combine(dotnetRootPath, "sdk", Product.Version, ridFileName); | ||
| string[] currentRuntimeIdentifiers = File.Exists(runtimeIdentifierChainPath) ? | ||
| File.ReadAllLines(runtimeIdentifierChainPath).Where(l => !string.IsNullOrEmpty(l)).ToArray() : | ||
| new string[] { }; | ||
| if (currentRuntimeIdentifiers == null || !currentRuntimeIdentifiers.Any() || !currentRuntimeIdentifiers[0].Contains("-")) | ||
| { | ||
| throw new GracefulException(CommonLocalizableStrings.CannotResolveRuntimeIdentifier); | ||
| } | ||
| return currentRuntimeIdentifiers[0]; // First rid is the most specific (ex win-x64) | ||
| } | ||
|
|
||
| private static string GetOsFromRid(string rid) => rid.Substring(0, rid.LastIndexOf("-")); | ||
|
|
||
| private static string GetArchFromRid(string rid) => rid.Substring(rid.LastIndexOf("-") + 1, rid.Length - rid.LastIndexOf("-") - 1); | ||
| } | ||
|
|
||
| public enum VerbosityOptions | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,8 @@ public static Command GetCommand() | |
| command.AddOption(InteractiveOption); | ||
| command.AddOption(NoRestoreOption); | ||
| command.AddOption(CommonOptions.VerbosityOption()); | ||
| command.AddOption(CommonOptions.ArchitectureOption()); | ||
| command.AddOption(CommonOptions.OperatingSystemOption()); | ||
sfoslund marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+50
to
+51
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We account for these options here: https://github.com/dotnet/sdk/pull/18889/files#diff-c59ee7ae48403cce44671ddc3549eb921e7c23bda26840c883b29d9f7e7942e5R38, the forwarding function shouldn't interfere. |
||
| command.TreatUnmatchedTokensAsErrors = false; | ||
|
|
||
| return command; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.