Fix PATH issue for archive containing nested portables with binary de…

…pendencies (#4816)

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"
   ]
-}
+}

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"
   ]
-}
+}

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))

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);
     };
-}
+}

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;
         }
     }
-}
+}

src/AppInstallerCLIE2ETests/Helpers/TestCommon.cs

@@ -353,13 +353,15 @@ public static string GetCheckpointsDirectory()
         /// <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 @@ public static void VerifyPortablePackage(
             {
                 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 @@ public static void VerifyPortablePackage(
             }
 
             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>

src/AppInstallerCLIE2ETests/InstallCommand.cs

@@ -516,6 +516,25 @@ public void InstallZip_Portable()
             TestCommon.VerifyPortablePackage(Path.Combine(installDir, packageDirName), commandAlias, fileName, productCode, true, TestCommon.Scope.User);
         }
 
+        /// <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>

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

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
+ManifestVersion: 1.9.0

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
+ManifestVersion: 1.9.0

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

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)

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);
     }
-}
+}

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;
     }
-}
+}