commit 1b0c45fbe9c822808db85fa1d6b43564db9a11c6
parent e6c9359e7ea53a85c005b9e597ca1bdaa668f0fb
Author: Ryan Fu <69221034+ryfu-msft@users.noreply.github.com>
Date: Thu, 16 Mar 2023 00:15:01 -0700
Fix PATH behavior of non-symlink installations for Portables/Zip (#3002)
Diffstat:
5 files changed, 58 insertions(+), 43 deletions(-)
diff --git a/src/AppInstallerCLICore/PortableInstaller.cpp b/src/AppInstallerCLICore/PortableInstaller.cpp
@@ -147,9 +147,9 @@ namespace AppInstaller::CLI::Portable
}
else
{
- // Symlink creation should only fail if the user executes without admin rights or developer mode.
- // Resort to adding install directory to PATH directly.
- AICLI_LOG(Core, Info, << "Portable install executed in user mode. Adding package directory to PATH.");
+ // If symlink creation fails, resort to adding the package directory to PATH.
+ AICLI_LOG(Core, Info, << "Failed to create symlink at: " << filePath);
+ AddToPathVariable(std::filesystem::path(entry.SymlinkTarget).parent_path());
CommitToARPEntry(PortableValueName::InstallDirectoryAddedToPath, InstallDirectoryAddedToPath = true);
}
}
@@ -167,6 +167,19 @@ namespace AppInstaller::CLI::Portable
AICLI_LOG(CLI, Info, << "Deleting portable exe at: " << filePath);
std::filesystem::remove(filePath);
}
+ else if (fileType == PortableFileType::Symlink)
+ {
+ if (Filesystem::SymlinkExists(filePath))
+ {
+ AICLI_LOG(CLI, Info, << "Deleting portable symlink at: " << filePath);
+ std::filesystem::remove(filePath);
+ }
+ else if (InstallDirectoryAddedToPath)
+ {
+ // If symlink doesn't exist, check if install directory was added to PATH directly and remove.
+ RemoveFromPathVariable(std::filesystem::path(entry.SymlinkTarget).parent_path());
+ }
+ }
else if (fileType == PortableFileType::Symlink && Filesystem::SymlinkExists(filePath))
{
AICLI_LOG(CLI, Info, << "Deleting portable symlink at: " << filePath);
@@ -188,7 +201,7 @@ namespace AppInstaller::CLI::Portable
bool deleteIndex = false;
{
PortableIndex existingIndex = PortableIndex::Open(existingIndexPath.u8string(), SQLiteStorageBase::OpenDisposition::ReadWrite);
-
+
for (auto expectedEntry : m_expectedEntries)
{
RemoveFile(expectedEntry);
@@ -262,7 +275,10 @@ namespace AppInstaller::CLI::Portable
ApplyDesiredState();
- AddToPathVariable();
+ if (!InstallDirectoryAddedToPath)
+ {
+ AddToPathVariable(GetPortableLinksLocation(GetScope()));
+ }
}
void PortableInstaller::Uninstall()
@@ -271,7 +287,10 @@ namespace AppInstaller::CLI::Portable
RemoveInstallDirectory();
- RemoveFromPathVariable();
+ if (!InstallDirectoryAddedToPath)
+ {
+ RemoveFromPathVariable(GetPortableLinksLocation(GetScope()));
+ }
m_portableARPEntry.Delete();
AICLI_LOG(CLI, Info, << "PortableARPEntry deleted.");
@@ -314,36 +333,34 @@ namespace AppInstaller::CLI::Portable
}
}
- void PortableInstaller::AddToPathVariable()
+ void PortableInstaller::AddToPathVariable(const std::filesystem::path& value)
{
- const std::filesystem::path& pathValue = InstallDirectoryAddedToPath ? TargetInstallLocation : GetPortableLinksLocation(GetScope());
- if (PathVariable(GetScope()).Append(pathValue))
+ if (PathVariable(GetScope()).Append(value))
{
- AICLI_LOG(Core, Info, << "Appended target directory to PATH registry: " << pathValue);
+ AICLI_LOG(Core, Info, << "Appending portable target directory to PATH registry: " << value);
m_stream << Resource::String::ModifiedPathRequiresShellRestart << std::endl;
}
else
{
- AICLI_LOG(CLI, Info, << "Target directory already exists in PATH registry: " << pathValue);
+ AICLI_LOG(CLI, Info, << "Portable target directory already exists in PATH registry: " << value);
}
}
- void PortableInstaller::RemoveFromPathVariable()
+ void PortableInstaller::RemoveFromPathVariable(const std::filesystem::path& value)
{
- const std::filesystem::path& pathValue = InstallDirectoryAddedToPath ? InstallLocation : GetPortableLinksLocation(GetScope());
- if (std::filesystem::exists(pathValue) && !std::filesystem::is_empty(pathValue))
+ if (std::filesystem::exists(value) && !std::filesystem::is_empty(value))
{
- AICLI_LOG(Core, Info, << "Install directory is not empty: " << pathValue);
+ AICLI_LOG(Core, Info, << "Install directory is not empty: " << value);
}
else
{
- if (PathVariable(GetScope()).Remove(pathValue))
+ if (PathVariable(GetScope()).Remove(value))
{
- AICLI_LOG(CLI, Info, << "Removed target directory from PATH registry: " << pathValue);
+ AICLI_LOG(CLI, Info, << "Removed target directory from PATH registry: " << value);
}
else
{
- AICLI_LOG(CLI, Info, << "Target directory not removed from PATH registry: " << pathValue);
+ AICLI_LOG(CLI, Info, << "Target directory not removed from PATH registry: " << value);
}
}
}
@@ -391,14 +408,16 @@ namespace AppInstaller::CLI::Portable
std::filesystem::path targetFullPath = PortableTargetFullPath;
std::filesystem::path symlinkFullPath = PortableSymlinkFullPath;
- if (!symlinkFullPath.empty())
+ // Order matters here so that file entries are removed before symlink entries during uninstall from registry.
+ // This is to ensure that the directory is fully uninstalled before attempting to remove from PATH registry.
+ if (!targetFullPath.empty())
{
- m_expectedEntries.emplace_back(std::move(PortableFileEntry::CreateSymlinkEntry(symlinkFullPath, targetFullPath)));
+ m_expectedEntries.emplace_back(std::move(PortableFileEntry::CreateFileEntry({}, targetFullPath, SHA256)));
}
- if (!targetFullPath.empty())
+ if (!symlinkFullPath.empty())
{
- m_expectedEntries.emplace_back(std::move(PortableFileEntry::CreateFileEntry({}, targetFullPath, SHA256)));
+ m_expectedEntries.emplace_back(std::move(PortableFileEntry::CreateSymlinkEntry(symlinkFullPath, targetFullPath)));
}
}
}
diff --git a/src/AppInstallerCLICore/PortableInstaller.h b/src/AppInstallerCLICore/PortableInstaller.h
@@ -67,11 +67,6 @@ namespace AppInstaller::CLI::Portable
m_portableARPEntry.SetValue(valueName, value);
}
- std::filesystem::path GetInstallDirectoryForPathVariable()
- {
- return InstallDirectoryAddedToPath ? InstallLocation : GetPortableLinksLocation(GetScope());
- }
-
std::filesystem::path GetPortableIndexFileName()
{
return Utility::ConvertToUTF16(GetProductCode() + ".db");
@@ -114,7 +109,7 @@ namespace AppInstaller::CLI::Portable
void CreateTargetInstallDirectory();
void RemoveInstallDirectory();
- void AddToPathVariable();
- void RemoveFromPathVariable();
+ 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
@@ -172,6 +172,8 @@ namespace AppInstaller::CLI::Workflow
// InstallerPath will point to a directory if it is extracted from an archive.
if (std::filesystem::is_directory(installerPath))
{
+ portableInstaller.RecordToIndex = true;
+
for (const auto& entry : std::filesystem::directory_iterator(installerPath))
{
std::filesystem::path entryPath = entry.path();
@@ -189,11 +191,6 @@ namespace AppInstaller::CLI::Workflow
}
}
- if (entries.size() > 1)
- {
- portableInstaller.RecordToIndex = true;
- }
-
const std::vector<Manifest::NestedInstallerFile>& nestedInstallerFiles = context.Get<Execution::Data::Installer>()->NestedInstallerFiles;
for (const auto& nestedInstallerFile : nestedInstallerFiles)
diff --git a/src/AppInstallerCLITests/InstallFlow.cpp b/src/AppInstallerCLITests/InstallFlow.cpp
@@ -636,23 +636,24 @@ TEST_CASE("InstallFlow_Portable", "[InstallFlow][workflow]")
TEST_CASE("InstallFlow_Portable_SymlinkCreationFail", "[InstallFlow][workflow]")
{
+ TestCommon::TempDirectory tempDirectory("TestPortableInstallRoot", false);
std::ostringstream installOutput;
TestContext installContext{ installOutput, std::cin };
auto PreviousThreadGlobals = installContext.SetForCurrentThread();
OverridePortableInstaller(installContext);
TestHook::SetCreateSymlinkResult_Override createSymlinkResultOverride(false);
+ const auto& targetDirectory = tempDirectory.GetPath();
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());
- // 'DefaultSource' is expected because we are installing from a local manifest.
- const auto& portableUserRoot = AppInstaller::Runtime::GetPathTo(AppInstaller::Runtime::PathName::PortablePackageUserRoot);
- const auto& portableTargetDirectory = portableUserRoot / "AppInstallerCliTest.TestPortableInstaller__DefaultSource";
- const auto& portableTargetPath = portableTargetDirectory / "AppInstallerTestExeInstaller.exe";
+ const auto& portableTargetPath = targetDirectory / "AppInstallerTestExeInstaller.exe";
REQUIRE(std::filesystem::exists(portableTargetPath));
- REQUIRE(AppInstaller::Registry::Environment::PathVariable(AppInstaller::Manifest::ScopeEnum::User).Contains(portableTargetDirectory));
+ REQUIRE(AppInstaller::Registry::Environment::PathVariable(AppInstaller::Manifest::ScopeEnum::User).Contains(targetDirectory));
// Perform uninstall
std::ostringstream uninstallOutput;
@@ -664,6 +665,7 @@ TEST_CASE("InstallFlow_Portable_SymlinkCreationFail", "[InstallFlow][workflow]")
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/UpdateFlow.cpp b/src/AppInstallerCLITests/UpdateFlow.cpp
@@ -180,6 +180,7 @@ TEST_CASE("UpdateFlow_UpdatePortable", "[UpdateFlow][workflow]")
TEST_CASE("UpdateFlow_Portable_SymlinkCreationFail", "[UpdateFlow][workflow]")
{
// Update portable with symlink creation failure verify that it succeeds.
+ TestCommon::TempDirectory tempDirectory("TestPortableInstallRoot", false);
std::ostringstream updateOutput;
TestContext context{ updateOutput, std::cin };
auto PreviousThreadGlobals = context.SetForCurrentThread();
@@ -187,15 +188,17 @@ TEST_CASE("UpdateFlow_Portable_SymlinkCreationFail", "[UpdateFlow][workflow]")
AppInstaller::Filesystem::TestHook_SetCreateSymlinkResult_Override(&overrideCreateSymlinkStatus);
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);
INFO(updateOutput.str());
- const auto& portableTargetDirectory = AppInstaller::Runtime::GetPathTo(AppInstaller::Runtime::PathName::PortablePackageUserRoot) / "AppInstallerCliTest.TestPortableInstaller__TestSource";
- const auto& portableTargetPath = portableTargetDirectory / "AppInstallerTestExeInstaller.exe";
+ const auto& portableTargetPath = targetDirectory / "AppInstallerTestExeInstaller.exe";
REQUIRE(std::filesystem::exists(portableTargetPath));
- REQUIRE(AppInstaller::Registry::Environment::PathVariable(AppInstaller::Manifest::ScopeEnum::User).Contains(portableTargetDirectory));
+ REQUIRE(AppInstaller::Registry::Environment::PathVariable(AppInstaller::Manifest::ScopeEnum::User).Contains(targetDirectory));
// Perform uninstall
std::ostringstream uninstallOutput;
@@ -209,7 +212,6 @@ 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(portableTargetDirectory));
}
TEST_CASE("UpdateFlow_UpdateExeWithUnsupportedArgs", "[UpdateFlow][workflow]")
