-
Notifications
You must be signed in to change notification settings - Fork 216
Add global registered install location to Linux and macOS #6124
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 |
|---|---|---|
|
|
@@ -301,12 +301,73 @@ bool pal::get_global_dotnet_dirs(std::vector<pal::string_t>* recv) | |
|
|
||
| bool pal::get_dotnet_self_registered_dir(pal::string_t* recv) | ||
| { | ||
| // No support for global directories in Unix. | ||
| return false; | ||
| recv->clear(); | ||
|
|
||
| // ***Used only for testing*** | ||
| pal::string_t environment_override; | ||
| if (pal::getenv(_X("_DOTNET_TEST_GLOBALLY_REGISTERED_PATH"), &environment_override)) | ||
|
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 thought you were worried about a secutiry hole with this.
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. Yes - but I didn't want to have one PR solving two separate issues... and I need a test-only thing for this feature. So I kept doing it the "old" way... will fix this in a separate PR for all cases where we do this. |
||
| { | ||
| recv->assign(environment_override); | ||
| return true; | ||
| } | ||
| // *************************** | ||
|
|
||
| pal::string_t install_location_file_path = _X("/etc/dotnet/install_location"); | ||
|
|
||
| // ***Used only for testing*** | ||
| pal::string_t environment_install_location_override; | ||
| if (pal::getenv(_X("_DOTNET_TEST_INSTALL_LOCATION_FILE_PATH"), &environment_install_location_override)) | ||
| { | ||
| install_location_file_path = environment_install_location_override; | ||
| } | ||
| // *************************** | ||
|
|
||
| trace::verbose(_X("Looking for install_location file in '%s'."), install_location_file_path.c_str()); | ||
| FILE* install_location_file = pal::file_open(install_location_file_path, "r"); | ||
| if (install_location_file == nullptr) | ||
| { | ||
| trace::verbose(_X("The install_location file failed to open.")); | ||
| return false; | ||
| } | ||
|
|
||
| bool result = false; | ||
|
|
||
| char buf[PATH_MAX]; | ||
| char* install_location = fgets(buf, sizeof(buf), install_location_file); | ||
| if (install_location != nullptr) | ||
| { | ||
| size_t len = pal::strlen(install_location); | ||
|
|
||
| // fgets includes the newline character in the string - so remove it. | ||
| if (len > 0 && len < PATH_MAX && install_location[len - 1] == '\n') | ||
| { | ||
| install_location[len - 1] = '\0'; | ||
| } | ||
|
|
||
| trace::verbose(_X("Using install location '%s'."), install_location); | ||
| *recv = install_location; | ||
| result = true; | ||
| } | ||
| else | ||
| { | ||
| trace::verbose(_X("The install_location file first line could not be read.")); | ||
| } | ||
|
|
||
| fclose(install_location_file); | ||
| return result; | ||
| } | ||
|
|
||
| bool pal::get_default_installation_dir(pal::string_t* recv) | ||
| { | ||
| // ***Used only for testing*** | ||
| pal::string_t environmentOverride; | ||
| if (pal::getenv(_X("_DOTNET_TEST_DEFAULT_INSTALL_PATH"), &environmentOverride)) | ||
| { | ||
| recv->assign(environmentOverride); | ||
| return true; | ||
| } | ||
| // *************************** | ||
|
|
||
| #if defined(__APPLE__) | ||
| recv->assign(_X("/usr/local/share/dotnet")); | ||
| #else | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,32 +65,27 @@ public void GetHostFxrPath_DotNetRootEnvironment(bool useAssemblyPath, bool isVa | |
| [InlineData(true, false, true)] | ||
| public void GetHostFxrPath_GlobalInstallation(bool useAssemblyPath, bool useRegisteredLocation, bool isValid) | ||
| { | ||
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| { | ||
| // We don't have a good way of hooking into how the product looks for global installations yet. | ||
| return; | ||
| } | ||
|
|
||
| // Overide the registry key for self-registered global installs. | ||
| // If using the registered location, set the install location value to the valid/invalid root. | ||
| // If not using the registered location, do not set the value. When the value does not exist, | ||
| // the product falls back to the default install location. | ||
| CommandResult result; | ||
| string installRoot = Path.Combine(isValid ? sharedState.ValidInstallRoot : sharedState.InvalidInstallRoot); | ||
| using (var regKeyOverride = new RegisteredInstallKeyOverride()) | ||
| string installLocation = Path.Combine(isValid ? sharedState.ValidInstallRoot : sharedState.InvalidInstallRoot, "dotnet"); | ||
| using (var registeredInstallLocationOverride = new RegisteredInstallLocationOverride()) | ||
| { | ||
| if (useRegisteredLocation) | ||
| { | ||
| regKeyOverride.SetInstallLocation(Path.Combine(installRoot, "dotnet"), sharedState.RepoDirectories.BuildArchitecture); | ||
| registeredInstallLocationOverride.SetInstallLocation(installLocation, sharedState.RepoDirectories.BuildArchitecture); | ||
| } | ||
|
|
||
| string programFilesOverride = useRegisteredLocation ? sharedState.InvalidInstallRoot : installRoot; | ||
| result = Command.Create(sharedState.NativeHostPath, $"{GetHostFxrPath} {(useAssemblyPath ? sharedState.TestAssemblyPath : string.Empty)}") | ||
| .CaptureStdErr() | ||
| .CaptureStdOut() | ||
| .EnvironmentVariable("COREHOST_TRACE", "1") | ||
| .EnvironmentVariable(Constants.TestOnlyEnvironmentVariables.RegistryPath, regKeyOverride.KeyPath) | ||
| .EnvironmentVariable("TEST_OVERRIDE_PROGRAMFILES", programFilesOverride) | ||
| .ApplyRegisteredInstallLocationOverride(registeredInstallLocationOverride) | ||
| .EnvironmentVariable( // Redirect the default install location to a test directory | ||
| Constants.TestOnlyEnvironmentVariables.DefaultInstallPath, | ||
| useRegisteredLocation ? sharedState.InvalidInstallRoot : installLocation) | ||
| .Execute(); | ||
| } | ||
|
|
||
|
|
@@ -142,6 +137,57 @@ public void GetHostFxrPath_HostFxrAlreadyLoaded() | |
| .And.HaveStdErrContaining($"Found previously loaded library {HostFxrName}"); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("{0}", true)] | ||
| [InlineData("{0}\n", true)] | ||
| [InlineData("{0}\nSome other text", true)] | ||
|
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. It is not clear why we should allow this case to be supported.
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. This is for extensibility. If in the future we needed to add more stuff into the file, the "old" apphosts should not fail on it. So we intentionally only read the first line and ignore the rest. |
||
| [InlineData("", false)] | ||
| [InlineData("\n{0}", false)] | ||
| [InlineData(" {0}", false)] | ||
| [InlineData("{0} \n", false)] | ||
| [InlineData("{0} ", false)] | ||
| public void GetHostFxrPath_InstallLocationFile(string value, bool shouldPass) | ||
| { | ||
| if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| { | ||
| // This test targets the install_location config file which is only used on Linux and macOS. | ||
| return; | ||
| } | ||
|
|
||
| string installLocation = Path.Combine(sharedState.ValidInstallRoot, "dotnet"); | ||
|
|
||
| using (RegisteredInstallLocationOverride registeredInstallLocationOverride = new RegisteredInstallLocationOverride()) | ||
| { | ||
| File.WriteAllText(registeredInstallLocationOverride.PathValueOverride, string.Format(value, installLocation)); | ||
|
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.
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. That would probably work, but I intentionally used the explicit file write here because this is testing values which are not just locations. So basically this should be the only test which does not use the |
||
|
|
||
| CommandResult result = Command.Create(sharedState.NativeHostPath, GetHostFxrPath) | ||
| .CaptureStdErr() | ||
| .CaptureStdOut() | ||
| .EnvironmentVariable("COREHOST_TRACE", "1") | ||
| .ApplyRegisteredInstallLocationOverride(registeredInstallLocationOverride) | ||
| .EnvironmentVariable( // Redirect the default install location to an invalid location so that it doesn't cause the test to pass | ||
| Constants.TestOnlyEnvironmentVariables.DefaultInstallPath, | ||
| sharedState.InvalidInstallRoot) | ||
| .Execute(); | ||
|
|
||
| result.Should().HaveStdErrContaining($"Looking for install_location file in '{registeredInstallLocationOverride.PathValueOverride}'."); | ||
|
|
||
| if (shouldPass) | ||
| { | ||
| result.Should().Pass() | ||
| .And.HaveStdErrContaining($"Using install location '{installLocation}'.") | ||
| .And.HaveStdOutContaining($"hostfxr_path: {sharedState.HostFxrPath}".ToLower()); | ||
| } | ||
| else | ||
| { | ||
| result.Should().Fail() | ||
| .And.ExitWith(1) | ||
| .And.HaveStdOutContaining($"{GetHostFxrPath} failed: 0x{CoreHostLibMissingFailure.ToString("x")}") | ||
| .And.HaveStdErrContaining($"The required library {HostFxrName} could not be found"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public class SharedTestState : SharedTestStateBase | ||
| { | ||
| public string HostFxrPath { get; } | ||
|
|
||
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.
This is still true, no?
Is it worth calling out the distinction between global / registered / default and how they are expected to be used for fxr / framework and sdk / shared store paths?
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.
Even before this change Linux does have a global location, and now it also has the registered location.
I'm not sure we should have the full description in the header file - it's relatively complex as it stands right now...
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.
Oh, I read this comment as referring to
get_global_dotnet_dirs, which still does not return any directories on Linux. That seemed wrong to me (in my mind global would include the default and registered location), but then I realized that it was not used for fxr lookup and only multi-level framework/sdk/store - hence my thinking the distinction was unclear.I guess I just found that name (
get_global_dotnet_dirs) confusing, since it doesn't quite match what I think of as 'global'.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 added comments to the three functions which are confusing... The names are actually technically correct, but I understand the confusion. Maybe I'm just too "in this" to come up with better names... If you have better ideas please share...