From d21f3ee922183ab3a9b38c6599a6a940b48a41bf Mon Sep 17 00:00:00 2001 From: --global Date: Wed, 11 Dec 2024 12:27:10 -0800 Subject: [PATCH] address PR comments --- .../package-manager/winget/returnCodes.md | 4 ++- .../Commands/FontCommand.cpp | 1 - src/AppInstallerCLICore/FontInstaller.cpp | 29 +++++++++++++++---- src/AppInstallerCLICore/FontInstaller.h | 2 +- .../Workflows/DownloadFlow.cpp | 2 +- .../Workflows/FontFlow.cpp | 8 ++--- .../Workflows/InstallFlow.cpp | 1 + src/AppInstallerCLIE2ETests/FontCommand.cs | 17 ++++++----- .../Helpers/TestCommon.cs | 2 +- .../Manifest/ManifestCommon.cpp | 5 ++++ .../Public/winget/ManifestCommon.h | 3 ++ src/AppInstallerSharedLib/Errors.cpp | 2 +- 12 files changed, 53 insertions(+), 23 deletions(-) diff --git a/doc/windows/package-manager/winget/returnCodes.md b/doc/windows/package-manager/winget/returnCodes.md index caad7a5ef4..2acacdd1b4 100644 --- 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 index 454b7c194e..1cb01acbd7 100644 --- 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 index de6d35cc63..dde9699265 100644 --- 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() }; + + 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 index 9f548e0908..51d3a6ab0c 100644 --- 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 index 219be19126..d97d046a46 100644 --- 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 index 1d1a1c12b5..b99ed61660 100644 --- 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(ERROR_SUCCESS); + context.Add(S_OK); } catch (...) { diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index dcd299340e..eb7b528784 100644 --- 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 index 3fdc287c63..3dc6eb00e3 100644 --- a/src/AppInstallerCLIE2ETests/FontCommand.cs +++ b/src/AppInstallerCLIE2ETests/FontCommand.cs @@ -24,27 +24,28 @@ public void OneTimeSetup() } /// - /// Test install a font with user scope. + /// Test install a font with machine scope. /// [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); } /// - /// Test install a font with machine scope. + /// Test install a font with user scope. /// - [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); } /// diff --git a/src/AppInstallerCLIE2ETests/Helpers/TestCommon.cs b/src/AppInstallerCLIE2ETests/Helpers/TestCommon.cs index f0b640abff..c0d94c4393 100644 --- 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 index c02d042dbe..07006a8bad 100644 --- 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 index 07e23db191..6fe764c297 100644 --- 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 index 58392ad0a4..c192c97932 100644 --- 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.