address PR comments

ryfu-msft · Dec 11, 2024 · d21f3ee · d21f3ee
1 parent 9550aed
commit d21f3ee
Show file tree

Hide file tree

Showing 12 changed files with 53 additions and 23 deletions.
diff --git a/doc/windows/package-manager/winget/returnCodes.md b/doc/windows/package-manager/winget/returnCodes.md
@@ -146,6 +146,8 @@ ms.localizationpriority: medium
 | 0x8A150084 | -1978335100 | APPINSTALLER_CLI_ERROR_SFSCLIENT_PACKAGE_NOT_SUPPORTED | The Microsoft Store package does not support download command. |
 | 0x8A150085 | -1978335099 | APPINSTALLER_CLI_ERROR_LICENSING_API_FAILED_FORBIDDEN | Failed to retrieve Microsoft Store package license. The Microsoft Entra Id account does not have required privilege. |
 | 0x8A150086 | -1978335098 | APPINSTALLER_CLI_ERROR_INSTALLER_ZERO_BYTE_FILE | Downloaded zero byte installer; ensure that your network connection is working properly. |
+| 0x8A150087 | -1978335097 | APPINSTALLER_CLI_ERROR_FONT_INSTALL_FAILED | Failed to install font package. |
+| 0x8A150088 | -1978335096 | APPINSTALLER_CLI_ERROR_FONT_FILE_NOT_SUPPORTED | Font file is not supported and cannot be installed. |
 
 ## Install errors.
 
@@ -170,7 +172,7 @@ ms.localizationpriority: medium
 | 0x8A150111 | -1978334959 | APPINSTALLER_CLI_ERROR_INSTALL_PACKAGE_IN_USE_BY_APPLICATION | Application is currently in use by another application. |
 | 0x8A150112 | -1978334958 | APPINSTALLER_CLI_ERROR_INSTALL_INVALID_PARAMETER | Invalid parameter. |
 | 0x8A150113 | -1978334957 | APPINSTALLER_CLI_ERROR_INSTALL_SYSTEM_NOT_SUPPORTED | Package not supported by the system. |
-| 0x8A150114 | -1978334956 | APPINSTALLER_CLI_ERROR_INSTALL_UPGRADE_NOT_SUPPORTED | The installer does not support upgrading an existing package. |
+| 0x8A150114 | -1978334956 | APPINSTALLER_CLI_ERROR_INSTALL_UPGRADE_NOT_SUPPORTED | The installer does not support upgrading an existing package. |
 | 0x8A150115 | -1978334955 | APPINSTALLER_CLI_ERROR_INSTALL_CUSTOM_ERROR | Installation failed with installer custom error. |
 
 ## Check for package installed status

diff --git a/src/AppInstallerCLICore/Commands/FontCommand.cpp b/src/AppInstallerCLICore/Commands/FontCommand.cpp
@@ -49,7 +49,6 @@ namespace AppInstaller::CLI
     {
         return {
             Argument::ForType(Args::Type::Manifest),
-            Argument{ Args::Type::InstallScope, Resource::String::InstallScopeDescription, ArgumentType::Standard, Argument::Visibility::Help },
         };
     }
 

diff --git a/src/AppInstallerCLICore/FontInstaller.cpp b/src/AppInstallerCLICore/FontInstaller.cpp
@@ -31,12 +31,12 @@ namespace AppInstaller::CLI::Font
         if (scope == Manifest::ScopeEnum::Machine)
         {
             m_installLocation = Runtime::GetPathTo(Runtime::PathName::FontsMachineInstallLocation);
-            m_key = Registry::Key::OpenIfExists(HKEY_LOCAL_MACHINE, std::wstring{ s_FontsPathSubkey }, 0, KEY_WRITE );
+            m_key = Registry::Key::Create(HKEY_LOCAL_MACHINE, std::wstring{ s_FontsPathSubkey });
         }
         else
         {
             m_installLocation = Runtime::GetPathTo(Runtime::PathName::FontsUserInstallLocation);
-            m_key = Registry::Key::OpenIfExists(HKEY_CURRENT_USER, std::wstring{ s_FontsPathSubkey }, 0, KEY_WRITE);
+            m_key = Registry::Key::Create(HKEY_CURRENT_USER, std::wstring{ s_FontsPathSubkey });
         }
     }
 
@@ -48,7 +48,7 @@ namespace AppInstaller::CLI::Font
             const auto& fileName = filePath.filename();
             const auto& destinationPath = m_installLocation / fileName;
 
-            AICLI_LOG(CLI, Info, << "Getting Font title");
+            AICLI_LOG(CLI, Verbose, << "Getting Font title");
 
             std::wstring title = AppInstaller::Fonts::GetFontFileTitle(filePath);
 
@@ -57,8 +57,24 @@ namespace AppInstaller::CLI::Font
                 title += s_TrueType;
             }
 
-            AICLI_LOG(CLI, Info, << "Moving font file to: " << destinationPath);
-            AppInstaller::Filesystem::RenameFile(filePath, destinationPath);
+            // If font subkey already exists, remove the font file if it exists.
+            if (m_key[title].has_value())
+            {
+                AICLI_LOG(CLI, Info, << "Existing font subkey found:" << AppInstaller::Utility::ConvertToUTF8(title));
+                std::filesystem::path existingFontFilePath = { m_key[title]->GetValue<Registry::Value::Type::String>() };
+
+                if (m_scope == Manifest::ScopeEnum::Machine)
+                {
+                    // Font entries in the HKEY_LOCAL_MACHINE hive only have the filename specified as the value. Prepend install location.
+                    existingFontFilePath = m_installLocation / existingFontFilePath;
+                }
+
+                if (std::filesystem::exists(existingFontFilePath))
+                {
+                    AICLI_LOG(CLI, Info, << "Removing existing font file at:" << existingFontFilePath);
+                    std::filesystem::remove(existingFontFilePath);
+                }
+            }
 
             AICLI_LOG(CLI, Info, << "Creating font subkey with name: " << AppInstaller::Utility::ConvertToUTF8(title));
             if (m_scope == Manifest::ScopeEnum::Machine)
@@ -69,6 +85,9 @@ namespace AppInstaller::CLI::Font
             {
                 m_key.SetValue(title, destinationPath, REG_SZ);
             }
+
+            AICLI_LOG(CLI, Info, << "Moving font file to: " << destinationPath);
+            AppInstaller::Filesystem::RenameFile(filePath, destinationPath);
         }
     }
 

diff --git a/src/AppInstallerCLICore/FontInstaller.h b/src/AppInstallerCLICore/FontInstaller.h
@@ -9,7 +9,7 @@ namespace AppInstaller::CLI::Font
     struct FontFile
     {
         FontFile(std::filesystem::path filePath, DWRITE_FONT_FILE_TYPE fileType)
-            : FilePath(filePath), FileType(fileType) {}
+            : FilePath(std::move(filePath)), FileType(fileType) {}
 
         std::filesystem::path FilePath;
         DWRITE_FONT_FILE_TYPE FileType;

diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp
@@ -77,7 +77,7 @@ namespace AppInstaller::CLI::Workflow
             std::filesystem::path filename = GetFileNameFromURI(installer->Url);
 
             // Default to URI for fonts since fonts can have multiple file extensions.
-            if (installer->BaseInstallerType != InstallerTypeEnum::Font)
+            if (!DoesInstallerTypeSupportMultipleFileExtensions(installer->BaseInstallerType))
             {
                 std::wstring_view installerExtension = GetInstallerFileExtension(context);
 

diff --git a/src/AppInstallerCLICore/Workflows/FontFlow.cpp b/src/AppInstallerCLICore/Workflows/FontFlow.cpp
@@ -159,18 +159,18 @@ namespace AppInstaller::CLI::Workflow
                 }
                 else
                 {
-                    AICLI_LOG(CLI, Warning, << "Font file is supported: " << file);
+                    AICLI_LOG(CLI, Verbose, << "Font file is supported: " << file);
                     fontFiles.emplace_back(FontFile(file, fileType));
                 }
             }
 
             context.Reporter.Info() << Resource::String::InstallFlowStartingPackageInstall << std::endl;
 
-            Manifest::ScopeEnum scope = AppInstaller::Manifest::ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope));
-            FontInstaller fontInstaller = FontInstaller(scope);
+            // TODO: Default to installing to HKEY_LOCAL_MACHINE registry as user install is not yet fully supported. 
+            FontInstaller fontInstaller = FontInstaller(Manifest::ScopeEnum::Machine);
 
             fontInstaller.Install(fontFiles);
-            context.Add<Execution::Data::OperationReturnCode>(ERROR_SUCCESS);
+            context.Add<Execution::Data::OperationReturnCode>(S_OK);
         }
         catch (...)
         {

diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp
@@ -285,6 +285,7 @@ namespace AppInstaller::CLI::Workflow
         void FontInstall(Execution::Context& context)
         {
             context <<
+                EnsureRunningAsAdmin <<
                 FontInstallImpl <<
                 ReportInstallerResult("Font"sv, APPINSTALLER_CLI_ERROR_FONT_INSTALL_FAILED, true);
         }

diff --git a/src/AppInstallerCLIE2ETests/FontCommand.cs b/src/AppInstallerCLIE2ETests/FontCommand.cs
@@ -24,27 +24,28 @@ public void OneTimeSetup()
         }
 
         /// <summary>
-        /// Test install a font with user scope.
+        /// Test install a font with machine scope.
         /// </summary>
         [Test]
-        public void InstallFont()
+        public void InstallFont_MachineScope()
         {
             var result = TestCommon.RunAICLICommand("install", "AppInstallerTest.TestFont");
             Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
             Assert.True(result.StdOut.Contains("Successfully installed"));
-            TestCommon.VerifyFontPackage(Constants.TestFontSubKeyName, Constants.FontFileName);
+            TestCommon.VerifyFontPackage(Constants.TestFontSubKeyName, Constants.FontFileName, TestCommon.Scope.Machine);
         }
 
         /// <summary>
-        /// Test install a font with machine scope.
+        /// Test install a font with user scope.
         /// </summary>
-        [Test]
-        public void InstallFont_MachineScope()
+        // [Test]
+        public void InstallFont_UserScope()
         {
-            var result = TestCommon.RunAICLICommand("install", "AppInstallerTest.TestFont --scope Machine");
+            // TODO: Enable this test once user scope installs are fully supported.
+            var result = TestCommon.RunAICLICommand("install", "AppInstallerTest.TestFont --scope User");
             Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
             Assert.True(result.StdOut.Contains("Successfully installed"));
-            TestCommon.VerifyFontPackage(Constants.TestFontSubKeyName, Constants.FontFileName, TestCommon.Scope.Machine);
+            TestCommon.VerifyFontPackage(Constants.TestFontSubKeyName, Constants.FontFileName, TestCommon.Scope.User);
         }
 
         /// <summary>

diff --git a/src/AppInstallerCLIE2ETests/Helpers/TestCommon.cs b/src/AppInstallerCLIE2ETests/Helpers/TestCommon.cs
@@ -371,7 +371,7 @@ public static string GetFontsDirectory(Scope scope)
         public static void VerifyFontPackage(
             string fontSubKeyName,
             string fontFileName,
-            Scope scope = Scope.User,
+            Scope scope = Scope.Machine,
             bool shouldExist = true)
         {
             // Expected font file path.

diff --git a/src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp b/src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp
@@ -950,6 +950,11 @@ namespace AppInstaller::Manifest
             installerType == InstallerTypeEnum::Exe;
     }
 
+    bool DoesInstallerTypeSupportMultipleFileExtensions(InstallerTypeEnum installerType)
+    {
+        return (installerType == InstallerTypeEnum::Font);
+    }
+
     bool IsArchiveType(InstallerTypeEnum installerType)
     {
         return (installerType == InstallerTypeEnum::Zip);

diff --git a/src/AppInstallerCommonCore/Public/winget/ManifestCommon.h b/src/AppInstallerCommonCore/Public/winget/ManifestCommon.h
@@ -448,6 +448,9 @@ namespace AppInstaller::Manifest
     // Gets a value indicating whether the given installer requires RepairBehavior for repair.
     bool DoesInstallerTypeRequireRepairBehaviorForRepair(InstallerTypeEnum installerType);
 
+    // Gets a value indicating whether the given installer can have multiple file extensions.
+    bool DoesInstallerTypeSupportMultipleFileExtensions(InstallerTypeEnum installerType);
+
     // Gets a value indicating whether the given installer type is an archive.
     bool IsArchiveType(InstallerTypeEnum installerType);
 

diff --git a/src/AppInstallerSharedLib/Errors.cpp b/src/AppInstallerSharedLib/Errors.cpp
@@ -222,7 +222,7 @@ namespace AppInstaller
             WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_SFSCLIENT_PACKAGE_NOT_SUPPORTED, "The Microsoft Store package does not support download."),
             WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_LICENSING_API_FAILED_FORBIDDEN, "Failed to retrieve Microsoft Store package license. The Microsoft Entra Id account does not have the required privilege."),
             WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALLER_ZERO_BYTE_FILE, "Downloaded zero byte installer; ensure that your network connection is working properly."),
-            WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_FONT_INSTALL_FAILED, "Failed to install font package"),
+            WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_FONT_INSTALL_FAILED, "Failed to install font package."),
             WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_FONT_FILE_NOT_SUPPORTED, "Font file is not supported and cannot be installed."),
 
             // Install errors.
-Original file line number
+Diff line change
@@ Expand Up / @@ -49,7 +49,6 @@ namespace AppInstaller::CLI @@
         {
             return {
                 Argument::ForType(Args::Type::Manifest),
-                Argument{ Args::Type::InstallScope, Resource::String::InstallScopeDescription, ArgumentType::Standard, Argument::Visibility::Help },
             };
         }
@@ Expand Down @@