From 20bfd6a8903e47c7cd68fd1562bfffdb6c5b76c3 Mon Sep 17 00:00:00 2001 From: Vitek Karas Date: Wed, 22 Sep 2021 10:04:00 -0700 Subject: [PATCH] Split install_location to file-per-architecture (#59404) On non-Windows we used install_location file with multiple lines which contained locations per-architecture (and the default). This changes the host to use separate file for each architecture: * `install_location_` for the architecture specific * `install_location` for the "legacy" file This is a much easier format for installers to handle (and it also simplifies the apphost code a little bit). Adapted tests to the new behavior. Removed some tests which don't make sense anymore. Some small test cleanup to deduplicate code. Changed the test-only env. variable to point to the directory with configuration files instead of to the configuration file itself. --- .../tests/HostActivation.Tests/Constants.cs | 2 +- .../InstallLocationCommandResultExtensions.cs | 9 +- .../MultiArchInstallLocation.cs | 41 ++----- .../NativeHosting/Nethost.cs | 58 ++++------ .../RegisteredInstallLocationOverride.cs | 18 +-- src/native/corehost/hostmisc/pal.unix.cpp | 108 +++++++++--------- 6 files changed, 96 insertions(+), 140 deletions(-) diff --git a/src/installer/tests/HostActivation.Tests/Constants.cs b/src/installer/tests/HostActivation.Tests/Constants.cs index f7e743aa99a..25768a56982 100644 --- a/src/installer/tests/HostActivation.Tests/Constants.cs +++ b/src/installer/tests/HostActivation.Tests/Constants.cs @@ -53,7 +53,7 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation public const string DefaultInstallPath = "_DOTNET_TEST_DEFAULT_INSTALL_PATH"; public const string RegistryPath = "_DOTNET_TEST_REGISTRY_PATH"; public const string GloballyRegisteredPath = "_DOTNET_TEST_GLOBALLY_REGISTERED_PATH"; - public const string InstallLocationFilePath = "_DOTNET_TEST_INSTALL_LOCATION_FILE_PATH"; + public const string InstallLocationPath = "_DOTNET_TEST_INSTALL_LOCATION_PATH"; } public static class RuntimeId diff --git a/src/installer/tests/HostActivation.Tests/InstallLocationCommandResultExtensions.cs b/src/installer/tests/HostActivation.Tests/InstallLocationCommandResultExtensions.cs index c85b7d3e56c..c4a8438d811 100644 --- a/src/installer/tests/HostActivation.Tests/InstallLocationCommandResultExtensions.cs +++ b/src/installer/tests/HostActivation.Tests/InstallLocationCommandResultExtensions.cs @@ -3,6 +3,7 @@ using System; +using System.IO; using System.Runtime.InteropServices; using FluentAssertions; using Microsoft.DotNet.Cli.Build.Framework; @@ -45,14 +46,14 @@ namespace HostActivation.Tests return assertion.HaveStdErrContaining($"Using global installation location [{installLocation}]"); } - public static AndConstraint HaveFoundDefaultInstallLocationInConfigFile(this CommandResultAssertions assertion, string installLocation) + public static AndConstraint HaveLookedForDefaultInstallLocation(this CommandResultAssertions assertion, string installLocationPath) { - return assertion.HaveStdErrContaining($"Found install location path '{installLocation}'."); + return assertion.HaveStdErrContaining($"Looking for install_location file in '{Path.Combine(installLocationPath, "install_location")}'."); } - public static AndConstraint HaveFoundArchSpecificInstallLocationInConfigFile(this CommandResultAssertions assertion, string installLocation, string arch) + public static AndConstraint HaveLookedForArchitectureSpecificInstallLocation(this CommandResultAssertions assertion, string installLocationPath, string architecture) { - return assertion.HaveStdErrContaining($"Found architecture-specific install location path: '{installLocation}' ('{arch}')."); + return assertion.HaveStdErrContaining($"Looking for architecture specific install_location file in '{Path.Combine(installLocationPath, "install_location_" + architecture.ToLowerInvariant())}'."); } } } diff --git a/src/installer/tests/HostActivation.Tests/MultiArchInstallLocation.cs b/src/installer/tests/HostActivation.Tests/MultiArchInstallLocation.cs index 97de5222c69..3829a30e682 100644 --- a/src/installer/tests/HostActivation.Tests/MultiArchInstallLocation.cs +++ b/src/installer/tests/HostActivation.Tests/MultiArchInstallLocation.cs @@ -174,40 +174,14 @@ namespace HostActivation.Tests if (!OperatingSystem.IsWindows()) { - result.Should().HaveFoundDefaultInstallLocationInConfigFile(path1) - .And.HaveFoundArchSpecificInstallLocationInConfigFile(path1, arch1) - .And.HaveFoundArchSpecificInstallLocationInConfigFile(path2, arch2); + result.Should() + .HaveLookedForArchitectureSpecificInstallLocation(registeredInstallLocationOverride.PathValueOverride, arch2); } result.Should().HaveUsedGlobalInstallLocation(path2); } } - [Fact] - [SkipOnPlatform(TestPlatforms.Windows, "This test targets the install_location config file which is only used on Linux and macOS.")] - public void InstallLocationFile_OnlyFirstLineMayNotSpecifyArchitecture() - { - var fixture = sharedTestState.PortableAppFixture - .Copy(); - - var appExe = fixture.TestProject.AppExe; - using (var registeredInstallLocationOverride = new RegisteredInstallLocationOverride(appExe)) - { - registeredInstallLocationOverride.SetInstallLocation(new (string, string)[] { - (string.Empty, "a/b/c"), - (string.Empty, "x/y/z"), - }); - Command.Create(appExe) - .EnableTracingAndCaptureOutputs() - .ApplyRegisteredInstallLocationOverride(registeredInstallLocationOverride) - .DotNetRoot(null) - .Execute() - .Should().HaveFoundDefaultInstallLocationInConfigFile("a/b/c") - .And.HaveStdErrContaining($"Only the first line in '{registeredInstallLocationOverride.PathValueOverride}' may not have an architecture prefix.") - .And.HaveUsedConfigFileInstallLocation("a/b/c"); - } - } - [Fact] [SkipOnPlatform(TestPlatforms.Windows, "This test targets the install_location config file which is only used on Linux and macOS.")] public void InstallLocationFile_ReallyLongInstallPathIsParsedCorrectly() @@ -229,7 +203,7 @@ namespace HostActivation.Tests .ApplyRegisteredInstallLocationOverride(registeredInstallLocationOverride) .DotNetRoot(null) .Execute() - .Should().HaveFoundDefaultInstallLocationInConfigFile(reallyLongPath) + .Should().HaveLookedForDefaultInstallLocation(registeredInstallLocationOverride.PathValueOverride) .And.HaveUsedConfigFileInstallLocation(reallyLongPath); } } @@ -247,16 +221,15 @@ namespace HostActivation.Tests { Directory.CreateDirectory(testArtifactsPath); - string directory = Path.Combine(testArtifactsPath, "installLocationOverride"); - Directory.CreateDirectory(directory); - string nonExistentLocationFile = Path.Combine(directory, "install_location"); + string installLocationDirectory = Path.Combine(testArtifactsPath, "installLocationOverride"); + Directory.CreateDirectory(installLocationDirectory); string defaultInstallLocation = Path.Combine(testArtifactsPath, "defaultInstallLocation"); Command.Create(appExe) .CaptureStdErr() .EnvironmentVariable( - Constants.TestOnlyEnvironmentVariables.InstallLocationFilePath, - nonExistentLocationFile) + Constants.TestOnlyEnvironmentVariables.InstallLocationPath, + installLocationDirectory) .EnvironmentVariable( Constants.TestOnlyEnvironmentVariables.DefaultInstallPath, defaultInstallLocation) diff --git a/src/installer/tests/HostActivation.Tests/NativeHosting/Nethost.cs b/src/installer/tests/HostActivation.Tests/NativeHosting/Nethost.cs index 0af2ce34774..9a90ad81253 100644 --- a/src/installer/tests/HostActivation.Tests/NativeHosting/Nethost.cs +++ b/src/installer/tests/HostActivation.Tests/NativeHosting/Nethost.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using HostActivation.Tests; using Microsoft.DotNet.Cli.Build.Framework; using System; using System.IO; @@ -216,12 +217,21 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.NativeHosting .DotNetRoot(null) .Execute(); - result.Should().HaveStdErrContaining($"Looking for install_location file in '{registeredInstallLocationOverride.PathValueOverride}'."); + if (shouldUseArchSpecificInstallLocation) + { + result.Should().HaveLookedForArchitectureSpecificInstallLocation( + registeredInstallLocationOverride.PathValueOverride, + sharedState.RepoDirectories.BuildArchitecture); + } + else + { + result.Should().HaveLookedForDefaultInstallLocation(registeredInstallLocationOverride.PathValueOverride); + } if (shouldPass) { result.Should().Pass() - .And.HaveStdErrContaining($"Using install location '{installLocation}'.") + .And.HaveUsedConfigFileInstallLocation(installLocation) .And.HaveStdOutContaining($"hostfxr_path: {sharedState.HostFxrPath}".ToLower()); } else @@ -234,35 +244,6 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.NativeHosting } } - [Fact] - [SkipOnPlatform(TestPlatforms.Windows, "This test targets the install_location config file which is only used on Linux and macOS.")] - public void GetHostFxrPath_GlobalInstallation_HasMoreThanOneDefaultInstallationPath() - { - string installLocation = Path.Combine(sharedState.ValidInstallRoot, "dotnet"); - using (var registeredInstallLocationOverride = new RegisteredInstallLocationOverride(sharedState.NethostPath)) - { - registeredInstallLocationOverride.SetInstallLocation(new (string, string)[] { - (string.Empty, installLocation), (string.Empty, installLocation) - }); - - CommandResult result = Command.Create(sharedState.NativeHostPath, GetHostFxrPath) - .EnableTracingAndCaptureOutputs() - .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) - .DotNetRoot(null) - .Execute(); - - result.Should().Pass() - .And.HaveStdErrContaining($"Looking for install_location file in '{registeredInstallLocationOverride.PathValueOverride}'.") - .And.HaveStdErrContaining($"Found install location path '{installLocation}'.") - .And.HaveStdErrContaining($"Only the first line in '{registeredInstallLocationOverride.PathValueOverride}' may not have an architecture prefix.") - .And.HaveStdErrContaining($"Using install location '{installLocation}'.") - .And.HaveStdOutContaining($"hostfxr_path: {sharedState.HostFxrPath}".ToLower()); - } - } - [Fact] [SkipOnPlatform(TestPlatforms.Windows, "This test targets the install_location config file which is only used on Linux and macOS.")] public void GetHostFxrPath_GlobalInstallation_HasNoDefaultInstallationPath() @@ -285,9 +266,10 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.NativeHosting .Execute(); result.Should().Pass() - .And.HaveStdErrContaining($"Looking for install_location file in '{registeredInstallLocationOverride.PathValueOverride}'.") - .And.HaveStdErrContaining($"Found architecture-specific install location path: '{installLocation}' ('{sharedState.RepoDirectories.BuildArchitecture.ToLower()}').") - .And.HaveStdErrContaining($"Using install location '{installLocation}'.") + .And.HaveLookedForArchitectureSpecificInstallLocation( + registeredInstallLocationOverride.PathValueOverride, + sharedState.RepoDirectories.BuildArchitecture) + .And.HaveUsedConfigFileInstallLocation(installLocation) .And.HaveStdOutContaining($"hostfxr_path: {sharedState.HostFxrPath}".ToLower()); } } @@ -314,10 +296,10 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.NativeHosting .Execute(); result.Should().Pass() - .And.HaveStdErrContaining($"Looking for install_location file in '{registeredInstallLocationOverride.PathValueOverride}'.") - .And.HaveStdErrContaining($"Found install location path '{installLocation}/a/b/c'.") - .And.HaveStdErrContaining($"Found architecture-specific install location path: '{installLocation}' ('{sharedState.RepoDirectories.BuildArchitecture.ToLower()}').") - .And.HaveStdErrContaining($"Using install location '{installLocation}'.") + .And.HaveLookedForArchitectureSpecificInstallLocation( + registeredInstallLocationOverride.PathValueOverride, + sharedState.RepoDirectories.BuildArchitecture) + .And.HaveUsedConfigFileInstallLocation(installLocation) .And.HaveStdOutContaining($"hostfxr_path: {sharedState.HostFxrPath}".ToLower()); } } diff --git a/src/installer/tests/HostActivation.Tests/RegisteredInstallLocationOverride.cs b/src/installer/tests/HostActivation.Tests/RegisteredInstallLocationOverride.cs index d37cfafe649..fcb29470d9e 100644 --- a/src/installer/tests/HostActivation.Tests/RegisteredInstallLocationOverride.cs +++ b/src/installer/tests/HostActivation.Tests/RegisteredInstallLocationOverride.cs @@ -56,10 +56,11 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation // On Linux/macOS the install location is registered in a file which is normally // located in /etc/dotnet/install_location // So we need to redirect it to a different place here. - string directory = Path.Combine(TestArtifact.TestArtifactsPath, "installLocationOverride"); + string directory = Path.Combine(TestArtifact.TestArtifactsPath, "installLocationOverride" + Process.GetCurrentProcess().Id.ToString()); + if (Directory.Exists(directory)) + Directory.Delete(directory, true); Directory.CreateDirectory(directory); - PathValueOverride = Path.Combine(directory, "install_location." + Process.GetCurrentProcess().Id.ToString()); - File.WriteAllText(PathValueOverride, ""); + PathValueOverride = directory; } } @@ -78,9 +79,12 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation } else { - File.WriteAllText(PathValueOverride, string.Join(Environment.NewLine, - locations.Select(l => string.Format("{0}{1}", - (!string.IsNullOrWhiteSpace(l.Architecture) ? l.Architecture + "=" : ""), l.Path)))); + foreach (var location in locations) + { + string installLocationFileName = "install_location" + + (!string.IsNullOrWhiteSpace(location.Architecture) ? ("_" + location.Architecture) : string.Empty); + File.WriteAllText(Path.Combine(PathValueOverride, installLocationFileName), location.Path); + } } } @@ -127,7 +131,7 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation else { return command.EnvironmentVariable( - Constants.TestOnlyEnvironmentVariables.InstallLocationFilePath, + Constants.TestOnlyEnvironmentVariables.InstallLocationPath, registeredInstallLocationOverride.PathValueOverride); } } diff --git a/src/native/corehost/hostmisc/pal.unix.cpp b/src/native/corehost/hostmisc/pal.unix.cpp index ef644bdf0d7..1ea7817900c 100644 --- a/src/native/corehost/hostmisc/pal.unix.cpp +++ b/src/native/corehost/hostmisc/pal.unix.cpp @@ -383,12 +383,12 @@ pal::string_t pal::get_dotnet_self_registered_config_location() { // ***Used only for testing*** pal::string_t environment_install_location_override; - if (test_only_getenv(_X("_DOTNET_TEST_INSTALL_LOCATION_FILE_PATH"), &environment_install_location_override)) + if (test_only_getenv(_X("_DOTNET_TEST_INSTALL_LOCATION_PATH"), &environment_install_location_override)) { return environment_install_location_override; } - return _X("/etc/dotnet/install_location"); + return _X("/etc/dotnet"); } namespace @@ -414,6 +414,42 @@ namespace } } +bool get_install_location_from_file(const pal::string_t& file_path, bool& file_found, pal::string_t& install_location) +{ + file_found = true; + bool install_location_found = false; + FILE* install_location_file = pal::file_open(file_path, "r"); + if (install_location_file != nullptr) + { + if (!get_line_from_file(install_location_file, install_location)) + { + trace::warning(_X("Did not find any install location in '%s'."), file_path.c_str()); + } + else + { + install_location_found = true; + } + + fclose(install_location_file); + if (install_location_found) + return true; + } + else + { + if (errno == ENOENT) + { + trace::verbose(_X("The install_location file ['%s'] does not exist - skipping."), file_path.c_str()); + file_found = false; + } + else + { + trace::error(_X("The install_location file ['%s'] failed to open: %s."), file_path.c_str(), pal::strerror(errno)); + } + } + + return false; +} + bool pal::get_dotnet_self_registered_dir(pal::string_t* recv) { recv->clear(); @@ -427,71 +463,31 @@ bool pal::get_dotnet_self_registered_dir(pal::string_t* recv) } // *************************** - pal::string_t install_location_file_path = get_dotnet_self_registered_config_location(); - 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) - { - if (errno == ENOENT) - { - trace::verbose(_X("The install_location file ['%s'] does not exist - skipping."), install_location_file_path.c_str()); - } - else - { - trace::error(_X("The install_location file ['%s'] failed to open: %s."), install_location_file_path.c_str(), pal::strerror(errno)); - } - - return false; - } + pal::string_t install_location_path = get_dotnet_self_registered_config_location(); + pal::string_t arch_specific_install_location_file_path = install_location_path; + append_path(&arch_specific_install_location_file_path, (_X("install_location_") + to_lower(get_arch())).c_str()); + trace::verbose(_X("Looking for architecture specific install_location file in '%s'."), arch_specific_install_location_file_path.c_str()); pal::string_t install_location; - int current_line = 0; - bool is_first_line = true, install_location_found = false; - - while (get_line_from_file(install_location_file, install_location)) + bool file_found = false; + if (!get_install_location_from_file(arch_specific_install_location_file_path, file_found, install_location)) { - current_line++; - size_t arch_sep = install_location.find(_X('=')); - if (arch_sep == pal::string_t::npos) + if (file_found) { - if (is_first_line) - { - recv->assign(install_location); - install_location_found = true; - trace::verbose(_X("Found install location path '%s'."), install_location.c_str()); - } - else - { - trace::warning(_X("Found unprefixed install location path '%s' on line %d."), install_location.c_str(), current_line); - trace::warning(_X("Only the first line in '%s' may not have an architecture prefix."), install_location_file_path.c_str()); - } - - is_first_line = false; - continue; + return false; } - pal::string_t arch_prefix = install_location.substr(0, arch_sep); - pal::string_t path_to_location = install_location.substr(arch_sep + 1); + pal::string_t legacy_install_location_file_path = install_location_path; + append_path(&legacy_install_location_file_path, _X("install_location")); + trace::verbose(_X("Looking for install_location file in '%s'."), legacy_install_location_file_path.c_str()); - trace::verbose(_X("Found architecture-specific install location path: '%s' ('%s')."), path_to_location.c_str(), arch_prefix.c_str()); - if (pal::strcasecmp(arch_prefix.c_str(), get_arch()) == 0) + if (!get_install_location_from_file(legacy_install_location_file_path, file_found, install_location)) { - recv->assign(path_to_location); - install_location_found = true; - trace::verbose(_X("Found architecture-specific install location path matching the current host architecture ('%s'): '%s'."), arch_prefix.c_str(), path_to_location.c_str()); - break; + return false; } - - is_first_line = false; - } - - fclose(install_location_file); - if (!install_location_found) - { - trace::warning(_X("Did not find any install location in '%s'."), install_location_file_path.c_str()); - return false; } + recv->assign(install_location); trace::verbose(_X("Using install location '%s'."), recv->c_str()); return true; } -- 2.34.1