-
Notifications
You must be signed in to change notification settings - Fork 5k
Add --arch
option for --list-runtimes
and --list-sdks
in host
#116078
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
Conversation
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov |
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.
Pull Request Overview
This PR adds a --arch <arch>
option to the --list-runtimes
and --list-sdks
host commands, allowing users to target a specific architecture’s install location when listing available runtimes or SDKs.
- Introduces
get_default_installation_dir_for_arch
andinstall_info::try_get_install_location
to resolve architecture-specific install paths. - Updates the FX muxer (
fx_muxer.cpp
) and command-line help (command_line.cpp
) to parse and document the new--arch
flag. - Changes
print_all_sdks
andprint_all_frameworks
signatures to acceptconst pal::char_t*
for leading whitespace, and reorganizes tests to focus on the--arch
behavior.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/native/corehost/hostmisc/pal.windows.cpp | Forward get_default_installation_dir to the new ARCH overload |
src/native/corehost/hostmisc/pal.unix.cpp | Same change on Unix and rename test ENV var to environment_override |
src/native/corehost/fxr/sdk_info.h & sdk_info.cpp | Change print_all_sdks to take const pal::char_t* |
src/native/corehost/fxr/install_info.h & install_info.cpp | Add try_get_install_location and unify install-location logic |
src/native/corehost/fxr/fx_muxer.cpp | Add get_requested_architecture , handle --arch in list commands |
src/native/corehost/fxr/framework_info.h & framework_info.cpp | Change print_all_frameworks to take const pal::char_t* |
src/native/corehost/fxr/command_line.cpp | Document --arch <arch> in usage output and adjust padding |
src/installer/tests/HostActivation.Tests/... | Remove outdated multilevel-lookup tests; add new HostCommands suite |
Comments suppressed due to low confidence (2)
src/native/corehost/hostmisc/pal.windows.cpp:392
- [nitpick] The local variable name
environmentOverride
uses camelCase, whereas the Unix counterpart usesenvironment_override
. Consider renaming toenvironment_override
for consistency.
pal::string_t environmentOverride;
src/native/corehost/fxr/fx_muxer.cpp:1030
- There are no tests covering the error paths when
--arch
is provided without a value or with an unrecognized value. Consider adding unit or integration tests that verify anInvalidArgFailure
exit code and the corresponding error message.
if (argc < 4)
trace::println(_X(" --list-runtimes [--arch <arch>] Display the installed runtimes.")); | ||
trace::println(_X(" --list-sdks [--arch <arch>] Display the installed SDKs.")); |
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.
trace::println(_X(" --list-runtimes [--arch <arch>] Display the installed runtimes.")); | |
trace::println(_X(" --list-sdks [--arch <arch>] Display the installed SDKs.")); | |
trace::println(_X(" --list-runtimes [--arch <arch>] Display the installed runtimes matching the host or specified architecture. Valid options are x64 and arm64.")); | |
trace::println(_X(" --list-sdks [--arch <arch>] Display the installed SDKs matching the host or specified architecture. Valid options are x64 and arm64.")); |
Would it be helpful to include this information? Or, should we include this elsewhere?
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 started with listing all valid arch values, but there are a decent number and listing them all didn't seem that helpful:
runtime/src/native/corehost/hostmisc/utils.cpp
Lines 194 to 205 in 1713d65
const pal::char_t* s_all_architectures[] = | |
{ | |
_X("arm"), | |
_X("arm64"), | |
_X("armv6"), | |
_X("loongarch64"), | |
_X("ppc64le"), | |
_X("riscv64"), | |
_X("s390x"), | |
_X("x64"), | |
_X("x86") | |
}; |
Maybe just listing common ones as examples?
Display the installed runtimes matching the host or specified architecture. Example architectures: arm64, x64, x86
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 like this suggestion! To me it would be valuable to document the values it accepts somewhere and what string format they are in because different ecosystems can describe the same architecture in different ways. I never would have expected 'rscv64' to be a valid value, though I don't work in anything related to that space. If I was a dev outside of dotnet and wanted to use this, there's a chance I'd end up at this comment thread to resolve that question 😁
@elinor-fung Hm, I just realized this might be tricky for us to adopt and know whether |
Yeah, I was thinking that could be a concern. Two (not great, but functional) ways I see:
Both involve an extra call to --list-runtimes/sdks, but 1 is probably a bit better, since the extra call doesn't actually have to go try to list the runtimes/sdks. |
Is this restriction necessary? It violates the GNU getopt spec. |
Technically, we could probably make that work. I had started with that, but went back to the specific order because:
I mentally went with |
Hmm, doesn't this still match up with getopts semantics? That is, my understanding the host options are basically
In that sense we would expect that I think there are other ways for a getopts parser to interpret that command line, but it seems like this is one valid way, and if we do interpret it that way, all of our existing restrictions are getopts-compatible. This would be the only one that isn't. |
I was seeing it as I think maybe they should be separated out from the options in the help usage so it is: +Usage: dotnet [host-commands]
Usage: dotnet [host-options] [path-to-application] |
This doesn't strike me as very different from
My interpretation is: options can either be flags to other commands, or they can be commands to the host to do things like print data. They can be combined with other flags, but the semantics can be that other options may be ignored, or other options may be incompatible. And it doesn't seem like ordering matters. |
Our existing |
I should say: Thank you for this! I am excited to see what kind of perf improvement we get in C# load times in VS Code with this change; I expect it will be quite good. |
Hm ok, then maybe we can improve this later. Is there anything stopping us from allowing reordering the architecture flag in the future? |
dotnet/runtime#116078 --arch was not added until .NET 10 to allow us to skip calling dotnet --info because that is slow, as it is not native code. // However, --arch gets ignored if the host does not support it. The output is also identical with or without --arch. // After discussion with the runtime team, the best way to determine if the host supports --arch is to call it with an invalid arch to see if it fails, because that only happens when --arch is supported. // The --arch flag was added in the middle of .NET 10, so we can assume it is supported if the version is 10.0 or later. // We don't want to slow down the current common case for people without .NET 10 by adding another process spawn check. // We don't check that the version is 10.0 or later after 2026 when .NET 11 starts rolling out, as It will be slower to check all of the numbers in the output for versions >= 10.
There would be nothing stopping us from allowing reordering (relaxing a requirement, so existing scenarios would continue to work normally and I don't thing there's anything added here that would complicate allowing reordering in the future). I opened: #116225 |
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.
LGTM
#2296) * Consider if host has --arch * Update check to whether --arch is supported dotnet/runtime#116078 --arch was not added until .NET 10 to allow us to skip calling dotnet --info because that is slow, as it is not native code. // However, --arch gets ignored if the host does not support it. The output is also identical with or without --arch. // After discussion with the runtime team, the best way to determine if the host supports --arch is to call it with an invalid arch to see if it fails, because that only happens when --arch is supported. // The --arch flag was added in the middle of .NET 10, so we can assume it is supported if the version is 10.0 or later. // We don't want to slow down the current common case for people without .NET 10 by adding another process spawn check. // We don't check that the version is 10.0 or later after 2026 when .NET 11 starts rolling out, as It will be slower to check all of the numbers in the output for versions >= 10. * Add a test Also support .NET 11 behavior while its in pre-release * Fix Bug Where HostArch is set to '' * increase test timeout they take longer now that they need to execute commands * increase test timeout time again
Allow specifying
--arch <arch>
for--list-runtimes
and--list-sdks
commands in host. Must be in the order--list-runtimes/sdks --arch <arch>
.dotnet
(for SDKs, any paths specified any found global.json are respected).dotnet
.Example - x64 host, registered x86 install, no arm64 install
New tests are in a
HostCommands
test class. This also moves some existing tests for host commands into the new class (bdcc21a).Info
coversDotnetArgValidation.DotNetInfo_WithSDK
,MultipleHives.DotNetInfo
,MultilevelSDKLookup.DotNetInfo
Info_NoSDK
coversDotnetArgValidation.DotNetInfo_NoSDK
Info_Utf8Path
coversDotnetArgValidation.DotNetInfo_Utf8Path
ListRuntimes
coversMultipleHives.ListRuntimes
ListSdks
coversMultilevelSDKLookup.ListSdks
ListRuntimes_OtherArchitecture*
andListSdks_OtherArchitecture*
cover the functionality added in this PRThis does remove tests running the host commands with the multi-level lookup environment variable set, but I don't think those are interesting anymore - it has been disabled since 7.0 and should always be disabled (unlike for actually running an app, where it can be based on the tfm)
Resolves #108503
cc @dotnet/appmodel @AaronRobinsonMSFT @nagilson