commit e813c49edb029494bac9a91a8c52ec1620378c92 parent 7650384a3f457532aa7df8ea3455280cb93b8c13 Author: Ryan <69221034+ryfu-msft@users.noreply.github.com> Date: Thu, 19 Sep 2024 21:35:09 -0400 Fix PATH issue for archive containing nested portables with binary dependencies (#4816) Diffstat:
16 files changed, 133 insertions(+), 31 deletions(-)
diff --git a/schemas/JSON/manifests/v1.9.0/manifest.installer.1.9.0.json b/schemas/JSON/manifests/v1.9.0/manifest.installer.1.9.0.json @@ -188,7 +188,7 @@ "maxLength": 2048, "description": "Custom switches will be passed directly to the installer by winget" }, - "Repair" : { + "Repair": { "type": [ "string", "null" ], "minLength": 1, "maxLength": 512, @@ -587,7 +587,7 @@ "type": [ "boolean", "null" ], "description": "Indicates whether the installer is prohibited from being downloaded for offline installation." }, - "RepairBehavior": { + "RepairBehavior": { "type": [ "string", "null" ], "enum": [ "modify", @@ -596,6 +596,10 @@ ], "description": "The repair method" }, + "ArchiveBinariesDependOnPath": { + "type": [ "boolean", "null" ], + "description": "Indicates whether the install location should be added directly to the PATH environment variable. Only applies to an archive containing portable packages." + }, "Installer": { "type": "object", "properties": { @@ -716,6 +720,9 @@ }, "RepairBehavior": { "$ref": "#/definitions/RepairBehavior" + }, + "ArchiveBinariesDependOnPath": { + "$ref": "#/definitions/ArchiveBinariesDependOnPath" } }, "required": [ @@ -835,6 +842,9 @@ "RepairBehavior": { "$ref": "#/definitions/RepairBehavior" }, + "ArchiveBinariesDependOnPath": { + "$ref": "#/definitions/ArchiveBinariesDependOnPath" + }, "Installers": { "type": "array", "items": { @@ -863,4 +873,4 @@ "ManifestType", "ManifestVersion" ] -}- \ No newline at end of file +} diff --git a/schemas/JSON/manifests/v1.9.0/manifest.singleton.1.9.0.json b/schemas/JSON/manifests/v1.9.0/manifest.singleton.1.9.0.json @@ -695,6 +695,10 @@ ], "description": "The repair method" }, + "ArchiveBinariesDependOnPath": { + "type": [ "boolean", "null" ], + "description": "Indicates whether the install location should be added directly to the PATH environment variable. Only applies to an archive containing portable packages." + }, "Installer": { "type": "object", "properties": { @@ -815,6 +819,9 @@ }, "RepairBehavior": { "$ref": "#/definitions/RepairBehavior" + }, + "ArchiveBinariesDependOnPath": { + "$ref": "#/definitions/ArchiveBinariesDependOnPath" } }, "required": [ @@ -1057,6 +1064,9 @@ "RepairBehavior": { "$ref": "#/definitions/RepairBehavior" }, + "ArchiveBinariesDependOnPath": { + "$ref": "#/definitions/ArchiveBinariesDependOnPath" + }, "Installers": { "type": "array", "items": { @@ -1090,4 +1100,4 @@ "ManifestType", "ManifestVersion" ] -}- \ No newline at end of file +} diff --git a/src/AppInstallerCLICore/PortableInstaller.cpp b/src/AppInstallerCLICore/PortableInstaller.cpp @@ -121,7 +121,16 @@ namespace AppInstaller::CLI::Portable } else if (entry.FileType == PortableFileType::Symlink) { - if (!InstallDirectoryAddedToPath) + if (BinariesDependOnPath && !InstallDirectoryAddedToPath) + { + // Scenario indicated by 'ArchiveBinariesDependOnPath' manifest entry. + // Skip symlink creation for portables dependent on binaries that require the install directory to be added to PATH. + std::filesystem::path installDirectory = std::filesystem::path(entry.SymlinkTarget).parent_path(); + AddToPathVariable(installDirectory); + AICLI_LOG(Core, Info, << "Install directory added to PATH: " << installDirectory); + CommitToARPEntry(PortableValueName::InstallDirectoryAddedToPath, InstallDirectoryAddedToPath = true); + } + else if (!InstallDirectoryAddedToPath) { std::filesystem::file_status status = std::filesystem::status(filePath); if (std::filesystem::is_directory(status)) diff --git a/src/AppInstallerCLICore/PortableInstaller.h b/src/AppInstallerCLICore/PortableInstaller.h @@ -32,6 +32,7 @@ namespace AppInstaller::CLI::Portable std::string WinGetPackageIdentifier; std::string WinGetSourceIdentifier; bool InstallDirectoryCreated = false; + bool BinariesDependOnPath = false; // If we fail to create a symlink, add install directory to PATH variable bool InstallDirectoryAddedToPath = false; @@ -112,4 +113,4 @@ namespace AppInstaller::CLI::Portable void AddToPathVariable(const std::filesystem::path& value); void RemoveFromPathVariable(const std::filesystem::path& value); }; -}- \ No newline at end of file +} diff --git a/src/AppInstallerCLICore/Workflows/PortableFlow.cpp b/src/AppInstallerCLICore/Workflows/PortableFlow.cpp @@ -135,12 +135,18 @@ namespace AppInstaller::CLI::Workflow } } - Utility::Architecture arch = context.Get<Execution::Data::Installer>()->Arch; + const auto& installer = context.Get<Execution::Data::Installer>().value(); + Utility::Architecture arch = installer.Arch; const std::string& productCode = GetPortableProductCode(context); PortableInstaller portableInstaller = PortableInstaller(scope, arch, productCode); portableInstaller.IsUpdate = isUpdate; + if (IsArchiveType(installer.BaseInstallerType) && installer.ArchiveBinariesDependOnPath) + { + portableInstaller.BinariesDependOnPath = true; + } + // Set target install directory std::string_view locationArg = context.Args.GetArg(Execution::Args::Type::InstallLocation); std::filesystem::path targetInstallDirectory; @@ -239,7 +245,6 @@ namespace AppInstaller::CLI::Workflow void PortableInstallImpl(Execution::Context& context) { PortableInstaller& portableInstaller = context.Get<Execution::Data::PortableInstaller>(); - try { context.Reporter.Info() << Resource::String::InstallFlowStartingPackageInstall << std::endl; @@ -324,4 +329,4 @@ namespace AppInstaller::CLI::Workflow EnsureVolumeSupportsReparsePoints; } } -}- \ No newline at end of file +} diff --git a/src/AppInstallerCLIE2ETests/Helpers/TestCommon.cs b/src/AppInstallerCLIE2ETests/Helpers/TestCommon.cs @@ -353,13 +353,15 @@ namespace AppInstallerCLIE2ETests.Helpers /// <param name="productCode">Product code.</param> /// <param name="shouldExist">Should exists.</param> /// <param name="scope">Scope.</param> + /// <param name="installDirectoryAddedToPath">Install directory added to path instead of the symlink directory.</param> public static void VerifyPortablePackage( string installDir, string commandAlias, string filename, string productCode, bool shouldExist, - Scope scope = Scope.User) + Scope scope = Scope.User, + bool installDirectoryAddedToPath = false) { // When portables are installed, if the exe path is inside a directory it will not be aliased // if the exe path is at the root level, it will be aliased. Therefore, if either exist, the exe exists @@ -386,7 +388,7 @@ namespace AppInstallerCLIE2ETests.Helpers { string pathName = "Path"; var currentPathValue = (string)environmentRegistryKey.GetValue(pathName); - var portablePathValue = symlinkDirectory + ';'; + var portablePathValue = (installDirectoryAddedToPath ? installDir : symlinkDirectory) + ';'; isAddedToPath = currentPathValue.Contains(portablePathValue); } @@ -396,9 +398,9 @@ namespace AppInstallerCLIE2ETests.Helpers } Assert.AreEqual(shouldExist, exeExists, $"Expected portable exe path: {exePath}"); - Assert.AreEqual(shouldExist, symlinkExists, $"Expected portable symlink path: {symlinkPath}"); + Assert.AreEqual(shouldExist && !installDirectoryAddedToPath, symlinkExists, $"Expected portable symlink path: {symlinkPath}"); Assert.AreEqual(shouldExist, portableEntryExists, $"Expected {productCode} subkey in path: {uninstallSubKey}"); - Assert.AreEqual(shouldExist, isAddedToPath, $"Expected path variable: {symlinkDirectory}"); + Assert.AreEqual(shouldExist, isAddedToPath, $"Expected path variable: {(installDirectoryAddedToPath ? installDir : symlinkDirectory)}"); } /// <summary> diff --git a/src/AppInstallerCLIE2ETests/InstallCommand.cs b/src/AppInstallerCLIE2ETests/InstallCommand.cs @@ -517,6 +517,25 @@ namespace AppInstallerCLIE2ETests } /// <summary> + /// Test install zip portable with binaries that depend on PATH variable. + /// </summary> + [Test] + public void InstallZip_ArchivePortableWithBinariesDependentOnPath() + { + string installDir = TestCommon.GetPortablePackagesDirectory(); + string packageId, commandAlias, fileName, packageDirName, productCode; + packageId = "AppInstallerTest.ArchivePortableWithBinariesDependentOnPath"; + packageDirName = productCode = packageId + "_" + Constants.TestSourceIdentifier; + commandAlias = "TestPortable.exe"; + fileName = "AppInstallerTestExeInstaller.exe"; + + var result = TestCommon.RunAICLICommand("install", $"{packageId}"); + Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode); + Assert.True(result.StdOut.Contains("Successfully installed")); + TestCommon.VerifyPortablePackage(Path.Combine(installDir, packageDirName), commandAlias, fileName, productCode, true, TestCommon.Scope.User, true); + } + + /// <summary> /// Test install zip with invalid relative file path. /// </summary> [Test] diff --git a/src/AppInstallerCLIE2ETests/TestData/Manifests/TestZipInstaller_Portable_BinariesDependOnPath.yaml b/src/AppInstallerCLIE2ETests/TestData/Manifests/TestZipInstaller_Portable_BinariesDependOnPath.yaml @@ -0,0 +1,19 @@ +PackageIdentifier: AppInstallerTest.ArchivePortableWithBinariesDependentOnPath +PackageVersion: 1.0.0.0 +PackageName: TestArchivePortableWithBinariesDependentOnPath +PackageLocale: en-US +Publisher: AppInstallerTest +License: Test +ShortDescription: E2E test for installing a zip containing a portable with binaries that depend on the PATH variable. +Installers: + - Architecture: x64 + InstallerUrl: https://localhost:5001/TestKit/AppInstallerTestZipInstaller/AppInstallerTestZipInstaller.zip + InstallerType: zip + InstallerSha256: <ZIPHASH> + NestedInstallerType: portable + NestedInstallerFiles: + - RelativeFilePath: AppInstallerTestExeInstaller.exe + PortableCommandAlias: TestPortable + ArchiveBinariesDependOnPath: true +ManifestType: singleton +ManifestVersion: 1.9.0 diff --git a/src/AppInstallerCLITests/TestData/ManifestV1_9-Singleton.yaml b/src/AppInstallerCLITests/TestData/ManifestV1_9-Singleton.yaml @@ -124,6 +124,7 @@ InstallationMetadata: InvocationParameter: "/arg" DisplayName: "DisplayName" DownloadCommandProhibited: true +ArchiveBinariesDependOnPath: true Installers: - Architecture: x86 @@ -194,5 +195,6 @@ Installers: ReturnResponse: custom ReturnResponseUrl: https://defaultReturnResponseUrl.com DownloadCommandProhibited: false + ArchiveBinariesDependOnPath: false ManifestType: singleton -ManifestVersion: 1.9.0- \ No newline at end of file +ManifestVersion: 1.9.0 diff --git a/src/AppInstallerCLITests/TestData/MultiFileManifestV1_9/ManifestV1_9-MultiFile-Installer.yaml b/src/AppInstallerCLITests/TestData/MultiFileManifestV1_9/ManifestV1_9-MultiFile-Installer.yaml @@ -89,6 +89,7 @@ InstallationMetadata: InvocationParameter: "/arg" DisplayName: "DisplayName" DownloadCommandProhibited: true +ArchiveBinariesDependOnPath: true Installers: - Architecture: x86 @@ -159,6 +160,7 @@ Installers: UnsupportedArguments: - location DownloadCommandProhibited: false + ArchiveBinariesDependOnPath: false - Architecture: x64 InstallerType: exe InstallerUrl: https://www.microsoft.com/msixsdk/msixsdkx64.exe @@ -197,6 +199,7 @@ Installers: FileType: other InvocationParameter: "/arg2" DisplayName: "DisplayName2" + ArchiveBinariesDependOnPath: true - Architecture: x64 InstallerType: burn InstallerUrl: https://www.microsoft.com/msixsdk/msixsdkx64.exe @@ -205,4 +208,4 @@ Installers: UpgradeBehavior: deny RepairBehavior: modify ManifestType: installer -ManifestVersion: 1.9.0- \ No newline at end of file +ManifestVersion: 1.9.0 diff --git a/src/AppInstallerCLITests/YamlManifest.cpp b/src/AppInstallerCLITests/YamlManifest.cpp @@ -261,6 +261,11 @@ namespace REQUIRE(defaultSwitches.at(InstallerSwitchType::Repair) == "/repair"); REQUIRE(manifest.DefaultInstallerInfo.RepairBehavior == RepairBehaviorEnum::Modify); } + + if (manifestVer >= ManifestVer{ s_ManifestVersionV1_9 }) + { + REQUIRE(manifest.DefaultInstallerInfo.ArchiveBinariesDependOnPath); + } } if (isSingleton || isExported) @@ -375,6 +380,11 @@ namespace REQUIRE(installer1.RepairBehavior == RepairBehaviorEnum::Modify); } + if (manifestVer >= ManifestVer{ s_ManifestVersionV1_9 }) + { + REQUIRE_FALSE(installer1.ArchiveBinariesDependOnPath); + } + if (!isSingleton) { if (!isExported) @@ -467,6 +477,12 @@ namespace REQUIRE(installer5.Switches.at(InstallerSwitchType::Repair) == "/repair"); REQUIRE(installer5.RepairBehavior == RepairBehaviorEnum::Modify); } + + if (manifestVer >= ManifestVer{ s_ManifestVersionV1_9 }) + { + ManifestInstaller installer4 = manifest.Installers.at(3); + REQUIRE(installer4.ArchiveBinariesDependOnPath); + } } // Localization diff --git a/src/AppInstallerCommonCore/Manifest/ManifestSchemaValidation.cpp b/src/AppInstallerCommonCore/Manifest/ManifestSchemaValidation.cpp @@ -32,7 +32,8 @@ namespace AppInstaller::Manifest::YamlParser { "RequireExplicitUpgrade"sv, YamlScalarType::Bool }, { "DisplayInstallWarnings"sv, YamlScalarType::Bool }, { "InstallerReturnCode"sv, YamlScalarType::Int }, - { "DownloadCommandProhibited", YamlScalarType::Bool } + { "DownloadCommandProhibited", YamlScalarType::Bool }, + { "ArchiveBinariesDependOnPath", YamlScalarType::Bool } }; YamlScalarType GetManifestScalarValueType(const std::string& key) diff --git a/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp b/src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp @@ -63,7 +63,7 @@ namespace AppInstaller::Manifest { AppInstaller::Manifest::ManifestError::ArpValidationError, "Arp Validation Error."sv }, { AppInstaller::Manifest::ManifestError::SchemaError, "Schema Error."sv }, { AppInstaller::Manifest::ManifestError::MsixSignatureHashFailed, "Failed to calculate MSIX signature hash.Please verify that the input file is a valid, signed MSIX."sv }, - { AppInstaller::Manifest::ManifestError::ShadowManifestNotAllowed, "Shadow manifest is not allowed."} + { AppInstaller::Manifest::ManifestError::ShadowManifestNotAllowed, "Shadow manifest is not allowed." } }; return ErrorIdToMessageMap; @@ -414,4 +414,4 @@ namespace AppInstaller::Manifest return Utility::ConvertToUTF8(Message); } -}- \ No newline at end of file +} diff --git a/src/AppInstallerCommonCore/Manifest/ManifestYamlPopulator.cpp b/src/AppInstallerCommonCore/Manifest/ManifestYamlPopulator.cpp @@ -376,6 +376,16 @@ namespace AppInstaller::Manifest std::move(fields_v1_7.begin(), fields_v1_7.end(), std::inserter(result, result.end())); } + + if (m_manifestVersion.get() >= ManifestVer{ s_ManifestVersionV1_9 }) + { + std::vector<FieldProcessInfo> fields_v1_9 = + { + { "ArchiveBinariesDependOnPath", [](const YAML::Node& value, const VariantManifestPtr& v)->ValidationErrors { GetManifestInstallerPtr(v)->ArchiveBinariesDependOnPath = value.as<bool>(); return {}; } }, + }; + + std::move(fields_v1_9.begin(), fields_v1_9.end(), std::inserter(result, result.end())); + } } return result; @@ -1243,4 +1253,4 @@ namespace AppInstaller::Manifest return errors; } -}- \ No newline at end of file +} diff --git a/src/AppInstallerCommonCore/Manifest/YamlWriter.cpp b/src/AppInstallerCommonCore/Manifest/YamlWriter.cpp @@ -66,7 +66,8 @@ namespace AppInstaller::Manifest::YamlWriter constexpr std::string_view DisplayName = "DisplayName"sv; constexpr std::string_view MinimumOSVersion = "MinimumOSVersion"sv; constexpr std::string_view DownloadCommandProhibited = "DownloadCommandProhibited"sv; - constexpr std::string_view RepairBehavior = "RepairBehavior"sv; + constexpr std::string_view RepairBehavior = "RepairBehavior"sv; + constexpr std::string_view ArchiveBinariesDependOnPath = "ArchiveBinariesDependOnPath"sv; // Installer switches constexpr std::string_view InstallerSwitches = "InstallerSwitches"sv; @@ -568,7 +569,8 @@ namespace AppInstaller::Manifest::YamlWriter WRITE_BOOL_PROPERTY(out, InstallLocationRequired, installer.InstallLocationRequired); WRITE_BOOL_PROPERTY(out, RequireExplicitUpgrade, installer.RequireExplicitUpgrade); WRITE_BOOL_PROPERTY(out, DisplayInstallWarnings, installer.DisplayInstallWarnings); - WRITE_BOOL_PROPERTY(out, DownloadCommandProhibited, installer.DownloadCommandProhibited); + WRITE_BOOL_PROPERTY(out, DownloadCommandProhibited, installer.DownloadCommandProhibited); + WRITE_BOOL_PROPERTY(out, ArchiveBinariesDependOnPath, installer.ArchiveBinariesDependOnPath); WRITE_PROPERTY_IF_EXISTS(out, MinimumOSVersion, installer.MinOSVersion); WRITE_PROPERTY_IF_EXISTS(out, ProductCode, installer.ProductCode); WRITE_PROPERTY_IF_EXISTS(out, UpgradeBehavior, UpdateBehaviorToString(installer.UpdateBehavior)); diff --git a/src/AppInstallerCommonCore/Public/winget/ManifestInstaller.h b/src/AppInstallerCommonCore/Public/winget/ManifestInstaller.h @@ -115,5 +115,7 @@ namespace AppInstaller::Manifest InstallationMetadataInfo InstallationMetadata; bool DownloadCommandProhibited = false; + + bool ArchiveBinariesDependOnPath = false; }; -}- \ No newline at end of file +}