Skip to content

Commit

Permalink
Fix PATH behavior of non-symlink installations for Portables/Zip (#3002)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ryfu-msft
ryfu-msft authored Mar 16, 2023
1 parent e6c9359 commit 1b0c45f
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 43 deletions.
63 changes: 41 additions & 22 deletions src/AppInstallerCLICore/PortableInstaller.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -262,7 +275,10 @@ namespace AppInstaller::CLI::Portable

ApplyDesiredState();

AddToPathVariable();
if (!InstallDirectoryAddedToPath)
{
AddToPathVariable(GetPortableLinksLocation(GetScope()));
}
}

void PortableInstaller::Uninstall()
Expand All @@ -271,7 +287,10 @@ namespace AppInstaller::CLI::Portable

RemoveInstallDirectory();

RemoveFromPathVariable();
if (!InstallDirectoryAddedToPath)
{
RemoveFromPathVariable(GetPortableLinksLocation(GetScope()));
}

m_portableARPEntry.Delete();
AICLI_LOG(CLI, Info, << "PortableARPEntry deleted.");
Expand Down Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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)));
}
}
}
Expand Down
9 changes: 2 additions & 7 deletions src/AppInstallerCLICore/PortableInstaller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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);
};
}
7 changes: 2 additions & 5 deletions src/AppInstallerCLICore/Workflows/PortableFlow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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)
Expand Down
12 changes: 7 additions & 5 deletions src/AppInstallerCLITests/InstallFlow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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]")
Expand Down
10 changes: 6 additions & 4 deletions src/AppInstallerCLITests/UpdateFlow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,22 +180,25 @@ 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();
bool overrideCreateSymlinkStatus = false;
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;
Expand All @@ -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]")
Expand Down

0 comments on commit 1b0c45f

Please sign in to comment.