diff --git a/src/AppInstallerCLICore/PortableInstaller.cpp b/src/AppInstallerCLICore/PortableInstaller.cpp index 1e3498e55a..e11d96483a 100644 --- a/src/AppInstallerCLICore/PortableInstaller.cpp +++ b/src/AppInstallerCLICore/PortableInstaller.cpp @@ -126,7 +126,7 @@ namespace AppInstaller::CLI::Portable { std::filesystem::path symlinkTargetPath{ Utility::ConvertToUTF16(entry.SymlinkTarget) }; - if (BinariesDependOnPath && !InstallDirectoryAddedToPath) + if (BinariesDependOnPath) { // Scenario indicated by 'ArchiveBinariesDependOnPath' manifest entry. // Skip symlink creation for portables dependent on binaries that require the install directory to be added to PATH. @@ -135,7 +135,7 @@ namespace AppInstaller::CLI::Portable AICLI_LOG(Core, Info, << "Install directory added to PATH: " << installDirectory); CommitToARPEntry(PortableValueName::InstallDirectoryAddedToPath, InstallDirectoryAddedToPath = true); } - else if (!InstallDirectoryAddedToPath) + else { std::filesystem::file_status status = std::filesystem::status(filePath); if (std::filesystem::is_directory(status)) @@ -157,8 +157,14 @@ namespace AppInstaller::CLI::Portable if (Filesystem::CreateSymlink(symlinkTargetPath, filePath)) { + m_hasCreatedSymlink = true; AICLI_LOG(Core, Info, << "Symlink created at: " << filePath << " with target path: " << symlinkTargetPath); } + else if (Filesystem::CreateFileHardLink(symlinkTargetPath, filePath)) + { + m_hasCreatedSymlink = true; + AICLI_LOG(Core, Info, << "Hardlink created at: " << filePath << " with target path: " << symlinkTargetPath); + } else { // If symlink creation fails, resort to adding the package directory to PATH. @@ -187,18 +193,20 @@ namespace AppInstaller::CLI::Portable { AICLI_LOG(CLI, Info, << "Deleting portable symlink at: " << filePath); std::filesystem::remove(filePath); + m_hasRemovedSymlink = true; } - else if (InstallDirectoryAddedToPath) + else if (std::filesystem::exists(filePath) && std::filesystem::is_regular_file(std::filesystem::status(filePath))) + { + AICLI_LOG(CLI, Info, << "Deleting portable hard link at: " << filePath); + std::filesystem::remove(filePath); + m_hasRemovedSymlink = true; + } + else { // If symlink doesn't exist, check if install directory was added to PATH directly and remove. - RemoveFromPathVariable(std::filesystem::path(Utility::ConvertToUTF16(entry.SymlinkTarget)).parent_path()); + RemoveFromPathVariable(std::filesystem::path(Utility::ConvertToUTF16(entry.SymlinkTarget)).parent_path(), false); } } - else if (fileType == PortableFileType::Symlink && Filesystem::SymlinkExists(filePath)) - { - AICLI_LOG(CLI, Info, << "Deleting portable symlink at: " << filePath); - std::filesystem::remove(filePath); - } else if (fileType == PortableFileType::Directory && std::filesystem::exists(filePath)) { AICLI_LOG(CLI, Info, << "Removing directory at " << filePath); @@ -294,7 +302,7 @@ namespace AppInstaller::CLI::Portable ApplyDesiredState(); - if (!InstallDirectoryAddedToPath) + if (m_hasCreatedSymlink) { AddToPathVariable(GetPortableLinksLocation(GetScope())); } @@ -307,20 +315,20 @@ namespace AppInstaller::CLI::Portable } } - void PortableInstaller::Uninstall() - { - ApplyDesiredState(); + void PortableInstaller::Uninstall() + { + ApplyDesiredState(); - RemoveInstallDirectory(); + RemoveInstallDirectory(); - if (!InstallDirectoryAddedToPath) - { - RemoveFromPathVariable(GetPortableLinksLocation(GetScope())); - } + if (m_hasRemovedSymlink) + { + RemoveFromPathVariable(GetPortableLinksLocation(GetScope())); + } - m_portableARPEntry.Delete(); - AICLI_LOG(CLI, Info, << "PortableARPEntry deleted."); - } + m_portableARPEntry.Delete(); + AICLI_LOG(CLI, Info, << "PortableARPEntry deleted."); + } void PortableInstaller::CreateTargetInstallDirectory() { @@ -375,27 +383,26 @@ namespace AppInstaller::CLI::Portable } } - void PortableInstaller::RemoveFromPathVariable(std::filesystem::path value) - { - if (std::filesystem::exists(value) && !std::filesystem::is_empty(value)) - { - AICLI_LOG(Core, Info, << "Install directory is not empty: " << value); - } - else - { + void PortableInstaller::RemoveFromPathVariable(std::filesystem::path value, bool checkIfEmpty /*= true*/) + { + if (checkIfEmpty && std::filesystem::exists(value) && !std::filesystem::is_empty(value)) + { + AICLI_LOG(Core, Info, << "Install directory is not empty: " << value); + } + else + { // Attempt to remove both the original and the preferred format to ensure removal - // Necessary for handling old path values associated with winget-cli#5033 - if (PathVariable(GetScope()).Remove(value) || PathVariable(GetScope()).Remove(value.make_preferred())) - { - InstallDirectoryAddedToPath = false; - AICLI_LOG(CLI, Info, << "Removed target directory from PATH registry: " << value); - } - else - { - AICLI_LOG(CLI, Info, << "Target directory not removed from PATH registry: " << value); - } - } - } + // Necessary for handling old path values associated with winget-cli#5033 + if (PathVariable(GetScope()).Remove(value) || PathVariable(GetScope()).Remove(value.make_preferred())) + { + AICLI_LOG(CLI, Info, << "Removed target directory from PATH registry: " << value); + } + else + { + AICLI_LOG(CLI, Info, << "Target directory not removed from PATH registry: " << value); + } + } + } void PortableInstaller::SetAppsAndFeaturesMetadata(const Manifest::Manifest& manifest, const std::vector& entries) { diff --git a/src/AppInstallerCLICore/PortableInstaller.h b/src/AppInstallerCLICore/PortableInstaller.h index 9f282efcd0..c66767b91d 100644 --- a/src/AppInstallerCLICore/PortableInstaller.h +++ b/src/AppInstallerCLICore/PortableInstaller.h @@ -35,6 +35,7 @@ namespace AppInstaller::CLI::Portable bool InstallDirectoryCreated = false; bool BinariesDependOnPath = false; // If we fail to create a symlink, add install directory to PATH variable + // TODO: Variable is redundant, remove it. bool InstallDirectoryAddedToPath = false; bool IsUpdate = false; @@ -94,6 +95,9 @@ namespace AppInstaller::CLI::Portable AppInstaller::Manifest::AppsAndFeaturesEntry GetAppsAndFeaturesEntry(); private: + // True if any symlink creation succeeds. No persistence required. + bool m_hasCreatedSymlink = false; + bool m_hasRemovedSymlink = false; PortableARPEntry m_portableARPEntry; std::vector m_desiredEntries; std::vector m_expectedEntries; @@ -114,6 +118,6 @@ namespace AppInstaller::CLI::Portable void RemoveInstallDirectory(); void AddToPathVariable(std::filesystem::path value); - void RemoveFromPathVariable(std::filesystem::path value); + void RemoveFromPathVariable(std::filesystem::path value, bool checkIfEmpty = true); }; } diff --git a/src/AppInstallerCLITests/InstallFlow.cpp b/src/AppInstallerCLITests/InstallFlow.cpp index 64fb767bbc..21fa91ecb8 100644 --- a/src/AppInstallerCLITests/InstallFlow.cpp +++ b/src/AppInstallerCLITests/InstallFlow.cpp @@ -731,7 +731,7 @@ TEST_CASE("InstallFlow_Portable_SymlinkCreationFail", "[InstallFlow][workflow]") TestCommon::TempDirectory tempDirectory("TestPortableInstallRoot", false); std::ostringstream installOutput; TestContext installContext{ installOutput, std::cin }; - auto PreviousThreadGlobals = installContext.SetForCurrentThread(); + installContext.SetForCurrentThread(); OverridePortableInstaller(installContext); TestHook::SetCreateSymlinkResult_Override createSymlinkResultOverride(false); const auto& targetDirectory = tempDirectory.GetPath(); @@ -743,18 +743,22 @@ TEST_CASE("InstallFlow_Portable_SymlinkCreationFail", "[InstallFlow][workflow]") InstallCommand install({}); install.Execute(installContext); - { - INFO(installOutput.str()); + AppInstaller::CLI::Portable::PortableInstaller& portableInstaller = installContext.Get(); + std::filesystem::path portableLinksLocation = AppInstaller::CLI::Portable::GetPortableLinksLocation(portableInstaller.GetScope()); + std::filesystem::path symlinkFile = portableLinksLocation / "AppInstallerTestExeInstaller.exe"; - // Use CHECK to allow the uninstall to still occur - CHECK(std::filesystem::exists(portableTargetPath)); - CHECK(AppInstaller::Registry::Environment::PathVariable(AppInstaller::Manifest::ScopeEnum::User).Contains(targetDirectory)); - } + INFO(installOutput.str()); + + // Use CHECK to allow the uninstall to still occur + CHECK(std::filesystem::exists(portableTargetPath)); + + CHECK(std::filesystem::exists(symlinkFile)); + CHECK(AppInstaller::Registry::Environment::PathVariable(AppInstaller::Manifest::ScopeEnum::User).Contains(portableLinksLocation)); // Perform uninstall std::ostringstream uninstallOutput; TestContext uninstallContext{ uninstallOutput, std::cin }; - auto previousThreadGlobals = uninstallContext.SetForCurrentThread(); + uninstallContext.SetForCurrentThread(); uninstallContext.Args.AddArg(Execution::Args::Type::Name, "AppInstaller Test Portable Exe"sv); uninstallContext.Args.AddArg(Execution::Args::Type::AcceptSourceAgreements); @@ -762,6 +766,44 @@ TEST_CASE("InstallFlow_Portable_SymlinkCreationFail", "[InstallFlow][workflow]") uninstall.Execute(uninstallContext); INFO(uninstallOutput.str()); REQUIRE_FALSE(std::filesystem::exists(portableTargetPath)); + REQUIRE_FALSE(std::filesystem::exists(symlinkFile)); +} + +TEST_CASE("InstallFlow_Portable_HardLinkCreationFail", "[InstallFlow][workflow]") +{ + TestCommon::TempDirectory tempDirectory("TestPortableInstallRoot", false); + std::ostringstream installOutput; + TestContext installContext{ installOutput, std::cin }; + installContext.SetForCurrentThread(); + OverridePortableInstaller(installContext); + TestHook::SetCreateSymlinkResult_Override createSymlinkResultOverride(false); + TestHook::SetCreateHardLinkResult_Override createHardLinkResultOverride(false); + const auto& targetDirectory = tempDirectory.GetPath(); + const auto& portableTargetPath = targetDirectory / "AppInstallerTestExeInstaller.exe"; + installContext.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_Portable.yaml").GetPath().u8string()); + installContext.Args.AddArg(Execution::Args::Type::InstallLocation, targetDirectory.u8string()); + installContext.Args.AddArg(Execution::Args::Type::InstallScope, "user"sv); + + InstallCommand install({}); + install.Execute(installContext); + + INFO(installOutput.str()); + + // Use CHECK to allow the uninstall to still occur + CHECK(std::filesystem::exists(portableTargetPath)); + CHECK(AppInstaller::Registry::Environment::PathVariable(AppInstaller::Manifest::ScopeEnum::User).Contains(targetDirectory)); + + // Perform uninstall + std::ostringstream uninstallOutput; + TestContext uninstallContext{ uninstallOutput, std::cin }; + uninstallContext.SetForCurrentThread(); + uninstallContext.Args.AddArg(Execution::Args::Type::Name, "AppInstaller Test Portable Exe"sv); + uninstallContext.Args.AddArg(Execution::Args::Type::AcceptSourceAgreements); + + UninstallCommand uninstall({}); + uninstall.Execute(uninstallContext); + INFO(uninstallOutput.str()); + REQUIRE_FALSE(std::filesystem::exists(portableTargetPath)); } TEST_CASE("PortableInstallFlow_UserScope", "[InstallFlow][workflow]") diff --git a/src/AppInstallerCLITests/TestHooks.h b/src/AppInstallerCLITests/TestHooks.h index 8c976a1381..fde2c7ba60 100644 --- a/src/AppInstallerCLITests/TestHooks.h +++ b/src/AppInstallerCLITests/TestHooks.h @@ -68,6 +68,7 @@ namespace AppInstaller namespace Filesystem { void TestHook_SetCreateSymlinkResult_Override(bool* status); + void TestHook_SetCreateHardLinkResult_Override(bool* status); } namespace Archive @@ -138,6 +139,22 @@ namespace TestHook bool m_status; }; + struct SetCreateHardLinkResult_Override + { + SetCreateHardLinkResult_Override(bool status) : m_status(status) + { + AppInstaller::Filesystem::TestHook_SetCreateHardLinkResult_Override(&m_status); + } + + ~SetCreateHardLinkResult_Override() + { + AppInstaller::Filesystem::TestHook_SetCreateHardLinkResult_Override(nullptr); + } + + private: + bool m_status; + }; + struct SetScanArchiveResult_Override { SetScanArchiveResult_Override(bool status) : m_status(status) diff --git a/src/AppInstallerCLITests/UpdateFlow.cpp b/src/AppInstallerCLITests/UpdateFlow.cpp index 9d70283c78..c2ee003050 100644 --- a/src/AppInstallerCLITests/UpdateFlow.cpp +++ b/src/AppInstallerCLITests/UpdateFlow.cpp @@ -184,9 +184,49 @@ TEST_CASE("UpdateFlow_Portable_SymlinkCreationFail", "[UpdateFlow][workflow]") TestCommon::TempDirectory tempDirectory("TestPortableInstallRoot", false); std::ostringstream updateOutput; TestContext context{ updateOutput, std::cin }; - auto PreviousThreadGlobals = context.SetForCurrentThread(); - bool overrideCreateSymlinkStatus = false; - AppInstaller::Filesystem::TestHook_SetCreateSymlinkResult_Override(&overrideCreateSymlinkStatus); + context.SetForCurrentThread(); + TestHook::SetCreateSymlinkResult_Override createSymlinkResultOverride(false); + OverridePortableInstaller(context); + OverrideForCompositeInstalledSource(context, CreateTestSource({ TSR::TestInstaller_Portable })); + const auto& targetDirectory = tempDirectory.GetPath(); + context.Args.AddArg(Execution::Args::Type::Query, TSR::TestInstaller_Portable.Query); + context.Args.AddArg(Execution::Args::Type::InstallLocation, targetDirectory.u8string()); + context.Args.AddArg(Execution::Args::Type::InstallScope, "user"sv); + + UpgradeCommand update({}); + update.Execute(context); + + // Get the PortableInstaller to access the scope information + AppInstaller::CLI::Portable::PortableInstaller& portableInstaller = context.Get(); + std::filesystem::path portableLinksLocation = AppInstaller::CLI::Portable::GetPortableLinksLocation(portableInstaller.GetScope()); + std::filesystem::path symlinkFile = portableLinksLocation / "AppInstallerTestExeInstaller.exe"; + + INFO(updateOutput.str()); + CHECK(std::filesystem::exists(symlinkFile)); + CHECK(AppInstaller::Registry::Environment::PathVariable(AppInstaller::Manifest::ScopeEnum::User).Contains(portableLinksLocation)); + + // Perform uninstall + std::ostringstream uninstallOutput; + TestContext uninstallContext{ uninstallOutput, std::cin }; + uninstallContext.SetForCurrentThread(); + OverrideForCompositeInstalledSource(uninstallContext, CreateTestSource({ TSR::TestInstaller_Portable })); + uninstallContext.Args.AddArg(Execution::Args::Type::Query, TSR::TestInstaller_Portable.Query); + + UninstallCommand uninstall({}); + uninstall.Execute(uninstallContext); + INFO(uninstallOutput.str()); + + REQUIRE_FALSE(std::filesystem::exists(symlinkFile)); +} + +TEST_CASE("UpdateFlow_Portable_HardLinkCreationFail", "[UpdateFlow][workflow]") +{ + TestCommon::TempDirectory tempDirectory("TestPortableInstallRoot", false); + std::ostringstream updateOutput; + TestContext context{ updateOutput, std::cin }; + context.SetForCurrentThread(); + TestHook::SetCreateSymlinkResult_Override createSymlinkResultOverride(false); + TestHook::SetCreateHardLinkResult_Override createHardLinkResultOverride(false); OverridePortableInstaller(context); OverrideForCompositeInstalledSource(context, CreateTestSource({ TSR::TestInstaller_Portable })); const auto& targetDirectory = tempDirectory.GetPath(); @@ -198,13 +238,13 @@ TEST_CASE("UpdateFlow_Portable_SymlinkCreationFail", "[UpdateFlow][workflow]") update.Execute(context); INFO(updateOutput.str()); const auto& portableTargetPath = targetDirectory / "AppInstallerTestExeInstaller.exe"; - REQUIRE(std::filesystem::exists(portableTargetPath)); - REQUIRE(AppInstaller::Registry::Environment::PathVariable(AppInstaller::Manifest::ScopeEnum::User).Contains(targetDirectory)); + CHECK(std::filesystem::exists(portableTargetPath)); + CHECK(AppInstaller::Registry::Environment::PathVariable(AppInstaller::Manifest::ScopeEnum::User).Contains(targetDirectory)); // Perform uninstall std::ostringstream uninstallOutput; TestContext uninstallContext{ uninstallOutput, std::cin }; - auto previousThreadGlobals = uninstallContext.SetForCurrentThread(); + uninstallContext.SetForCurrentThread(); OverrideForCompositeInstalledSource(uninstallContext, CreateTestSource({ TSR::TestInstaller_Portable })); uninstallContext.Args.AddArg(Execution::Args::Type::Query, TSR::TestInstaller_Portable.Query); @@ -213,6 +253,7 @@ TEST_CASE("UpdateFlow_Portable_SymlinkCreationFail", "[UpdateFlow][workflow]") INFO(uninstallOutput.str()); REQUIRE_FALSE(std::filesystem::exists(portableTargetPath)); + REQUIRE_FALSE(AppInstaller::Registry::Environment::PathVariable(AppInstaller::Manifest::ScopeEnum::User).Contains(targetDirectory)); } TEST_CASE("UpdateFlow_UpdateExeWithUnsupportedArgs", "[UpdateFlow][workflow]") diff --git a/src/AppInstallerSharedLib/Filesystem.cpp b/src/AppInstallerSharedLib/Filesystem.cpp index 85acbf09a3..5cdb5c0106 100644 --- a/src/AppInstallerSharedLib/Filesystem.cpp +++ b/src/AppInstallerSharedLib/Filesystem.cpp @@ -474,6 +474,34 @@ namespace AppInstaller::Filesystem } } +#ifndef AICLI_DISABLE_TEST_HOOKS + static bool* s_CreateHardLinkResult_TestHook_Override = nullptr; + + void TestHook_SetCreateHardLinkResult_Override(bool* status) + { + s_CreateHardLinkResult_TestHook_Override = status; + } +#endif + + bool CreateFileHardLink(const std::filesystem::path& target, const std::filesystem::path& link) + { +#ifndef AICLI_DISABLE_TEST_HOOKS + if (s_CreateHardLinkResult_TestHook_Override) + { + return *s_CreateHardLinkResult_TestHook_Override; + } +#endif + try + { + std::filesystem::create_hard_link(target, link); + return true; + } + catch (...) + { + return false; + } + } + bool VerifySymlink(const std::filesystem::path& symlink, const std::filesystem::path& target) { // Use read_symlink to get the symlink's recorded target without traversing the filesystem diff --git a/src/AppInstallerSharedLib/Public/winget/Filesystem.h b/src/AppInstallerSharedLib/Public/winget/Filesystem.h index f450444234..01dc7ecf2e 100644 --- a/src/AppInstallerSharedLib/Public/winget/Filesystem.h +++ b/src/AppInstallerSharedLib/Public/winget/Filesystem.h @@ -30,6 +30,9 @@ namespace AppInstaller::Filesystem // Creates a symlink that points to the target path. bool CreateSymlink(const std::filesystem::path& target, const std::filesystem::path& link); + // Creates a hard link that points to the target path. + bool CreateFileHardLink(const std::filesystem::path& target, const std::filesystem::path& link); + // Verifies that a symlink points to the target path. bool VerifySymlink(const std::filesystem::path& symlink, const std::filesystem::path& target); @@ -38,7 +41,6 @@ namespace AppInstaller::Filesystem // Checks if the path is a symlink and exists. bool SymlinkExists(const std::filesystem::path& symlinkPath); - bool CreateSymlink(const std::filesystem::path& path, const std::filesystem::path& target); // Get expanded file system path. std::filesystem::path GetExpandedPath(const std::string& path);