Add support for multi-arch install locations #53763

mateoatr · 2021-06-05T05:43:35Z

This PR implements the accepted design for multi-arch install locations. It adds the ability for Unix users to specify architecture-specific install locations in the install location config file (located in /etc/dotnet/install_location); this file currently contains a single line pointing to the registered install location.

Changes in this PR evolve the format of the aforementioned config file so that users are now able to specify multiple architecture-specific install locations and, optionally, keep the first line as it currently is (i.e., without any architecture prefix) -- this will be needed for backcompat with previous apphosts and will also work as the fallback install location if none of the arch install locations can be resolved.

This PR also gives users the ability to set architecture-specific environment variables for specifying the location of the .NET runtime by extending the current format to DOTNET_ROOT_ARCH. The current format, DOTNET_ROOT (and DOTNET_ROOT(x86) on Windows), will continue to be supported and will work as a fallback when no arch-specific env vars are used.

Currently on Windows x86, the only environment variable that is looked for finding the install location is DOTNET_ROOT(x86), changes in this PR will cause to fallback to DOTNET_ROOT whenever DOTNET_ROOT(x86) is not set in win-x86.

ghost · 2021-06-05T05:43:43Z

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

Author:	mateoatr
Assignees:	-
Labels:	`area-Host`
Milestone:	-

src/installer/tests/HostActivation.Tests/CommandExtensions.cs

src/installer/tests/HostActivation.Tests/MultiArchInstallLocation.cs

src/native/corehost/hostmisc/pal.unix.cpp

src/native/corehost/hostmisc/utils.cpp

src/native/corehost/hostmisc/pal.unix.cpp

src/native/corehost/hostmisc/utils.cpp

src/installer/tests/HostActivation.Tests/MultiArchInstallLocation.cs

src/native/corehost/hostmisc/pal.unix.cpp

vitek-karas · 2021-06-08T09:03:37Z

src/native/corehost/hostmisc/pal.unix.cpp

+        is_first_line = false;
+    }
+
+    if (architecture_install_location != "")


If the config file looks like this on x64 machine:

/default/path x64=

The result will be to use /default/path. I'm not sure I like that. The user did specify arch specific path, it's just invalid. Basically I think this should be a very similar behavior to something like:

/default/path x64=////nonsense///

Both cases should fail with error saying that the root path is invalid (doesn't exist).

There's also this case:

x64=/dotnet/x64

(the first line is empty). If we're running on arm64 - it should fail, the only question is if it should fail with the "Did not find any install location" or "Install location '' doesn't exist".
(Note that existing apphost will fail with the latter).

src/native/corehost/hostmisc/utils.cpp

src/native/corehost/hostmisc/pal.unix.cpp

src/native/corehost/hostmisc/utils.cpp

src/native/corehost/fxr_resolver.cpp

vitek-karas

Looks good. One final ask - try to test this in the real world Apple M1 scenario:

Install 6.0 arm64 preview into /usr/local/share/dotnet
Install 5.0 x64 preview into /usr/local/share/dotnet/x64

Write /etc/dotnet/install_location with content:

/usr/local/share/dotnet/x64
arm64=/usr/local/share/dotnet

Try to run osx-arm64 net6.0 apphost app (should need this change to work)
Try to run osx-x64 net5.0 apphost app (should work as shipped)

src/native/corehost/hostmisc/pal.unix.cpp

src/native/corehost/hostmisc/pal.h

src/native/corehost/hostmisc/utils.cpp

src/installer/tests/HostActivation.Tests/MultiArchInstallLocation.cs

src/native/corehost/hostmisc/pal.unix.cpp

src/installer/tests/HostActivation.Tests/NativeHosting/Nethost.cs

src/installer/tests/HostActivation.Tests/MultiArchInstallLocation.cs

elinor-fung

Looks good to me.

Last question - this PR doesn't mention the multi-arch support using the registry on Windows. While looking at this PR, I noticed that it is currently only for x86 and x64:

runtime/src/native/corehost/hostmisc/pal.windows.cpp

Lines 347 to 351 in ca3c11d

    
           bool pal::get_dotnet_self_registered_dir(pal::string_t* recv) 
        
           { 
        
           #if !defined(TARGET_AMD64) && !defined(TARGET_X86) 
        
               //  Self-registered SDK installation directory is only supported for x64 and x86 architectures. 
        
               return false;

runtime/src/native/corehost/hostmisc/pal.windows.cpp

Lines 332 to 335 in ca3c11d

    
           bool pal::get_dotnet_self_registered_config_location(pal::string_t* recv) 
        
           { 
        
           #if !defined(TARGET_AMD64) && !defined(TARGET_X86) 
        
               return false;

Is there separate work/testing for that?

vitek-karas · 2021-06-21T08:23:25Z

Thanks a lot @elinor-fung for finding the Windows issue. That must be fixed (and tests added) as well. But feel free to do that in a separate PR>

Move to_lower, to_upper to utils Add MultiArchInstallLocation tests

Modify trace error message

Warn instead of error Change from vector to fgets

Move get_line_from_file to unix pal

Check the rid of the launched process

mateoatr · 2021-06-23T07:18:16Z

Tested the above scenarios locally on an M1 -- everything working. CI failures here seem unrelated; for instance, retriggering the Build Android x64 Release AllSubsets_Mono_RuntimeTests pipeline resulted in no failures.

mateoatr · 2021-06-23T07:32:22Z

Is there separate work/testing for that?

I'm taking a look at this. I added this item in the tracking issue that we have for the multi-arch installer support (#50798).

…nitial_config * origin/main: (362 commits) [wasm][debugger] Reuse debugger-agent on wasm debugger (dotnet#52300) Put Crossgen2 in sync with dotnet#54235 (dotnet#54438) Move System.Object serialization to ObjectConverter (dotnet#54436) Move setting fHasVirtualStaticMethods out of sanity check section (dotnet#54574) [wasm] Compile .bc->.o in parallel, before passing to the linker (dotnet#54053) Change PathInternal.IsCaseSensitive to a constant (dotnet#54340) Make mono_polling_required a public symbol (dotnet#54592) Respect EventSource::IsSupported setting in more codepaths (dotnet#51977) Root ComActivator for hosting (dotnet#54524) Add ILLink annotations to S.D.Common related to DbConnectionStringBuilder (dotnet#54280) Fix finalizer issue with regions (dotnet#54550) Add support for multi-arch install locations (dotnet#53763) Update library testing docs page to reduce confusion (dotnet#54324) [FileStream] handle UNC and device paths (dotnet#54483) Update NetAnalyzers version (dotnet#54511) Added runtime dependency to fix the intermittent test failures (dotnet#54587) Disable failing System.Reflection.Tests.ModuleTests.GetMethods (dotnet#54564) [wasm] Move AOT builds from `runtime-staging` to `runtime` (dotnet#54577) Keep obj node for ArrayIndex. (dotnet#54584) Disable another failing MemoryCache test (dotnet#54578) ...

dotnet-issue-labeler bot added the area-Host label Jun 5, 2021

vitek-karas reviewed Jun 7, 2021

View reviewed changes

src/native/corehost/hostmisc/pal.unix.cpp Outdated Show resolved Hide resolved

src/native/corehost/hostmisc/utils.cpp Show resolved Hide resolved

elinor-fung reviewed Jun 7, 2021

View reviewed changes

src/native/corehost/hostmisc/utils.cpp Outdated Show resolved Hide resolved

src/native/corehost/hostmisc/utils.cpp Outdated Show resolved Hide resolved

src/installer/tests/HostActivation.Tests/MultiArchInstallLocation.cs Outdated Show resolved Hide resolved

vitek-karas reviewed Jun 8, 2021

View reviewed changes

src/native/corehost/hostmisc/utils.cpp Outdated Show resolved Hide resolved

This was referenced Jun 9, 2021

Pipeline dependency issue: Artifact CoreCLRProduct bits missing. #53948

Closed

Pipeline dependency issue: Artifact CoreCLRProduct bits missing. #53842

Closed

elinor-fung reviewed Jun 10, 2021

View reviewed changes

src/native/corehost/hostmisc/utils.cpp Outdated Show resolved Hide resolved

src/native/corehost/hostmisc/pal.unix.cpp Show resolved Hide resolved

src/native/corehost/hostmisc/utils.cpp Outdated Show resolved Hide resolved

vitek-karas reviewed Jun 11, 2021

View reviewed changes

src/native/corehost/fxr_resolver.cpp Outdated Show resolved Hide resolved

mateoatr marked this pull request as ready for review June 14, 2021 19:19

vitek-karas approved these changes Jun 15, 2021

View reviewed changes

src/native/corehost/hostmisc/pal.unix.cpp Outdated Show resolved Hide resolved

elinor-fung reviewed Jun 15, 2021

View reviewed changes

mateoatr force-pushed the multiArch branch from 1f110d4 to 91118ca Compare June 18, 2021 22:09

mateoatr requested a review from elinor-fung June 18, 2021 22:14

elinor-fung approved these changes Jun 19, 2021

View reviewed changes

Mateo Torres Ruiz added 12 commits June 22, 2021 14:23

Add support for multiple architectures inside install_locations

89c6a26

Fix traces

fd767f1

Add install_location tests

1de63dc

Message printed to stderr

48be319

Set arch specific DOTNET_ROOT env var

f1a2b7d

Inline pal::to_lower/upper

7fe0238

Update tests

e40c618

Move to_lower, to_upper to utils Add MultiArchInstallLocation tests

Update tests

98814ea

Sort usings

3a40dfd

PR Feedback

e1d55c9

Fix trace

9602d92

Fail if DOTNET_ROOT_ARCH is defined but path doesn't exist

c1b7af2

Modify trace error message

Mateo Torres Ruiz added 11 commits June 22, 2021 14:23

PR feedback

104357c

Fallback to DOTNET_ROOT on win32

3280fff

Warn instead of error Change from vector to fgets

Add tests with really long install location path

8236390

Move get_line_from_file to unix pal

Fix tests

190bc43

Make consistent use of str assign

bbc05c8

PR feedback

e5b18e5

Fix unix pal

53edd07

Test

c9ad207

Remove nonsense string

6ae64aa

Put optional arguments last

a04b846

Check the rid of the launched process

Pass RID

2402b61

mateoatr force-pushed the multiArch branch from 91118ca to 2402b61 Compare June 22, 2021 21:23

mateoatr merged commit 97de5c5 into dotnet:main Jun 23, 2021

mateoatr mentioned this pull request Jun 24, 2021

Enable win registry install location for all architectures #54698

Merged

ghost locked as resolved and limited conversation to collaborators Jul 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multi-arch install locations #53763

Add support for multi-arch install locations #53763

mateoatr commented Jun 5, 2021 •

edited

Loading

ghost commented Jun 5, 2021

vitek-karas Jun 8, 2021

vitek-karas left a comment

elinor-fung left a comment •

edited

Loading

vitek-karas commented Jun 21, 2021

mateoatr commented Jun 23, 2021

mateoatr commented Jun 23, 2021

	bool pal::get_dotnet_self_registered_dir(pal::string_t* recv)
	{
	#if !defined(TARGET_AMD64) && !defined(TARGET_X86)
	// Self-registered SDK installation directory is only supported for x64 and x86 architectures.
	return false;

	bool pal::get_dotnet_self_registered_config_location(pal::string_t* recv)
	{
	#if !defined(TARGET_AMD64) && !defined(TARGET_X86)
	return false;

Add support for multi-arch install locations #53763

Add support for multi-arch install locations #53763

Conversation

mateoatr commented Jun 5, 2021 • edited Loading

ghost commented Jun 5, 2021

vitek-karas Jun 8, 2021

Choose a reason for hiding this comment

vitek-karas left a comment

Choose a reason for hiding this comment

elinor-fung left a comment • edited Loading

Choose a reason for hiding this comment

vitek-karas commented Jun 21, 2021

mateoatr commented Jun 23, 2021

mateoatr commented Jun 23, 2021

mateoatr commented Jun 5, 2021 •

edited

Loading

elinor-fung left a comment •

edited

Loading