From 1c717cdadcdaae533acbb27aadef93536deafcfb Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Wed, 22 Feb 2023 11:30:06 -0800 Subject: [PATCH 1/9] copy instead of rename for directory file type --- src/AppInstallerCLICore/PortableInstaller.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/AppInstallerCLICore/PortableInstaller.cpp b/src/AppInstallerCLICore/PortableInstaller.cpp index bbe3311570..329651801d 100644 --- a/src/AppInstallerCLICore/PortableInstaller.cpp +++ b/src/AppInstallerCLICore/PortableInstaller.cpp @@ -107,8 +107,9 @@ namespace AppInstaller::CLI::Portable } else if (fileType == PortableFileType::Directory) { - AICLI_LOG(Core, Info, << "Moving directory to: " << filePath); - Filesystem::RenameFile(entry.CurrentPath, filePath); + // Copy directory instead of renaming as there is a known issue with renaming across filesystems. + AICLI_LOG(Core, Info, << "Copying directory to: " << filePath); + std::filesystem::copy(entry.CurrentPath, filePath, std::filesystem::copy_options::overwrite_existing | std::filesystem::copy_options::recursive); } else if (entry.FileType == PortableFileType::Symlink) { From 41b08748a18236100734431aa227c563d212cafe Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Wed, 22 Feb 2023 11:36:44 -0800 Subject: [PATCH 2/9] fix spelling --- src/AppInstallerCLICore/PortableInstaller.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/AppInstallerCLICore/PortableInstaller.cpp b/src/AppInstallerCLICore/PortableInstaller.cpp index 329651801d..924d914f66 100644 --- a/src/AppInstallerCLICore/PortableInstaller.cpp +++ b/src/AppInstallerCLICore/PortableInstaller.cpp @@ -107,7 +107,7 @@ namespace AppInstaller::CLI::Portable } else if (fileType == PortableFileType::Directory) { - // Copy directory instead of renaming as there is a known issue with renaming across filesystems. + // Copy directory instead of renaming as there is a known issue with renaming across drives. AICLI_LOG(Core, Info, << "Copying directory to: " << filePath); std::filesystem::copy(entry.CurrentPath, filePath, std::filesystem::copy_options::overwrite_existing | std::filesystem::copy_options::recursive); } From 0c1c76189b5bc6f89179d1e4608362e62dd775ff Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Fri, 3 Mar 2023 14:30:42 -0800 Subject: [PATCH 3/9] add isSameVolume and unit tests --- src/AppInstallerCLICore/PortableInstaller.cpp | 13 +++++++--- src/AppInstallerCLITests/Filesystem.cpp | 25 +++++++++++++++++++ src/AppInstallerCommonCore/Filesystem.cpp | 14 +++++++++++ .../Public/winget/Filesystem.h | 3 +++ 4 files changed, 52 insertions(+), 3 deletions(-) diff --git a/src/AppInstallerCLICore/PortableInstaller.cpp b/src/AppInstallerCLICore/PortableInstaller.cpp index 924d914f66..b78790ce65 100644 --- a/src/AppInstallerCLICore/PortableInstaller.cpp +++ b/src/AppInstallerCLICore/PortableInstaller.cpp @@ -107,9 +107,16 @@ namespace AppInstaller::CLI::Portable } else if (fileType == PortableFileType::Directory) { - // Copy directory instead of renaming as there is a known issue with renaming across drives. - AICLI_LOG(Core, Info, << "Copying directory to: " << filePath); - std::filesystem::copy(entry.CurrentPath, filePath, std::filesystem::copy_options::overwrite_existing | std::filesystem::copy_options::recursive); + if (Filesystem::IsSameVolume(entry.CurrentPath, filePath)) + { + Filesystem::RenameFile(entry.CurrentPath, filePath); + } + else + { + // Copy directory instead of renaming as there is a known issue with renaming across drives. + AICLI_LOG(Core, Info, << "Copying directory to: " << filePath); + std::filesystem::copy(entry.CurrentPath, filePath, std::filesystem::copy_options::overwrite_existing | std::filesystem::copy_options::recursive); + } } else if (entry.FileType == PortableFileType::Symlink) { diff --git a/src/AppInstallerCLITests/Filesystem.cpp b/src/AppInstallerCLITests/Filesystem.cpp index 2e851cac06..b30dc8ce4f 100644 --- a/src/AppInstallerCLITests/Filesystem.cpp +++ b/src/AppInstallerCLITests/Filesystem.cpp @@ -56,4 +56,29 @@ TEST_CASE("VerifySymlink", "[filesystem]") std::filesystem::remove(symlinkPath); REQUIRE_FALSE(SymlinkExists(symlinkPath)); +} + +TEST_CASE("VerifyIsSameVolume", "[filesystem]") +{ + std::filesystem::path path1 = L"C:\\Program Files\\WinGet\\Packages"; + std::filesystem::path path2 = L"C:\\Users\\testUser\\AppData\\Local\\Microsoft\\WinGet\\Packages"; + std::filesystem::path path3 = L"localPath\\test\\folder"; + + std::filesystem::path path4 = L"D:\\test\\folder"; + std::filesystem::path path5 = L"F:\\test\\folder"; + std::filesystem::path path6 = L"d:\\randomFolder"; + + // Verify true is returned for paths on the same volume. + REQUIRE(IsSameVolume(path1, path2)); + REQUIRE(IsSameVolume(path1, path3)); + REQUIRE(IsSameVolume(path4, path6)); + + // Verify false is returned for paths on different volumes. + REQUIRE_FALSE(IsSameVolume(path1, path4)); + REQUIRE_FALSE(IsSameVolume(path1, path5)); + REQUIRE_FALSE(IsSameVolume(path2, path4)); + REQUIRE_FALSE(IsSameVolume(path2, path5)); + REQUIRE_FALSE(IsSameVolume(path3, path4)); + REQUIRE_FALSE(IsSameVolume(path3, path5)); + REQUIRE_FALSE(IsSameVolume(path4, path5)); } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Filesystem.cpp b/src/AppInstallerCommonCore/Filesystem.cpp index d7ca5017a0..97a829faa6 100644 --- a/src/AppInstallerCommonCore/Filesystem.cpp +++ b/src/AppInstallerCommonCore/Filesystem.cpp @@ -238,4 +238,18 @@ namespace AppInstaller::Filesystem THROW_IF_FAILED(SHGetKnownFolderPath(id, KF_FLAG_NO_ALIAS | KF_FLAG_DONT_VERIFY | KF_FLAG_NO_PACKAGE_REDIRECTION, NULL, &knownFolder)); return knownFolder.get(); } + + bool IsSameVolume(const std::filesystem::path& current, const std::filesystem::path& target) + { + LPWSTR currentVolumeBuffer = new WCHAR[MAX_PATH]; + LPWSTR targetVolumeBuffer = new WCHAR[MAX_PATH]; + + GetVolumePathNameW(current.c_str(), currentVolumeBuffer, sizeof(currentVolumeBuffer)); + GetVolumePathNameW(target.c_str(), targetVolumeBuffer, sizeof(targetVolumeBuffer)); + + std::wstring_view currentVolume { currentVolumeBuffer }; + std::wstring_view targetVolume { targetVolumeBuffer }; + + return Utility::CaseInsensitiveEquals(Utility::ConvertToUTF8(currentVolume), Utility::ConvertToUTF8(targetVolume)); + } } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Public/winget/Filesystem.h b/src/AppInstallerCommonCore/Public/winget/Filesystem.h index 43b3be75d5..da0036bc01 100644 --- a/src/AppInstallerCommonCore/Public/winget/Filesystem.h +++ b/src/AppInstallerCommonCore/Public/winget/Filesystem.h @@ -42,4 +42,7 @@ namespace AppInstaller::Filesystem // Gets the path of a known folder. std::filesystem::path GetKnownFolderPath(const KNOWNFOLDERID& id); + + // Verifies that the target path is on the same volume as the current path. + bool IsSameVolume(const std::filesystem::path& current, const std::filesystem::path& target); } \ No newline at end of file From 759fe6eafcbba7e2972e86068c7f20b39b70abc2 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Fri, 3 Mar 2023 16:00:42 -0800 Subject: [PATCH 4/9] fix unit tests --- src/AppInstallerCLITests/Filesystem.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/AppInstallerCLITests/Filesystem.cpp b/src/AppInstallerCLITests/Filesystem.cpp index b30dc8ce4f..d7c220f8fa 100644 --- a/src/AppInstallerCLITests/Filesystem.cpp +++ b/src/AppInstallerCLITests/Filesystem.cpp @@ -60,6 +60,9 @@ TEST_CASE("VerifySymlink", "[filesystem]") TEST_CASE("VerifyIsSameVolume", "[filesystem]") { + TestCommon::TempDirectory tempDirectory("TempDirectory"); + std::filesystem::path path = tempDirectory.GetPath(); + std::filesystem::path path1 = L"C:\\Program Files\\WinGet\\Packages"; std::filesystem::path path2 = L"C:\\Users\\testUser\\AppData\\Local\\Microsoft\\WinGet\\Packages"; std::filesystem::path path3 = L"localPath\\test\\folder"; @@ -68,12 +71,11 @@ TEST_CASE("VerifyIsSameVolume", "[filesystem]") std::filesystem::path path5 = L"F:\\test\\folder"; std::filesystem::path path6 = L"d:\\randomFolder"; - // Verify true is returned for paths on the same volume. + // Verify that a relative path points to the current volume. + REQUIRE(IsSameVolume(path, path3)); REQUIRE(IsSameVolume(path1, path2)); - REQUIRE(IsSameVolume(path1, path3)); REQUIRE(IsSameVolume(path4, path6)); - // Verify false is returned for paths on different volumes. REQUIRE_FALSE(IsSameVolume(path1, path4)); REQUIRE_FALSE(IsSameVolume(path1, path5)); REQUIRE_FALSE(IsSameVolume(path2, path4)); From 7e4878597bbaaa86669aefa9d9785630380d2475 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Mon, 6 Mar 2023 10:01:34 -0800 Subject: [PATCH 5/9] fix tests --- src/AppInstallerCLITests/Filesystem.cpp | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/AppInstallerCLITests/Filesystem.cpp b/src/AppInstallerCLITests/Filesystem.cpp index d7c220f8fa..511e2dda9d 100644 --- a/src/AppInstallerCLITests/Filesystem.cpp +++ b/src/AppInstallerCLITests/Filesystem.cpp @@ -60,27 +60,26 @@ TEST_CASE("VerifySymlink", "[filesystem]") TEST_CASE("VerifyIsSameVolume", "[filesystem]") { - TestCommon::TempDirectory tempDirectory("TempDirectory"); - std::filesystem::path path = tempDirectory.GetPath(); - std::filesystem::path path1 = L"C:\\Program Files\\WinGet\\Packages"; - std::filesystem::path path2 = L"C:\\Users\\testUser\\AppData\\Local\\Microsoft\\WinGet\\Packages"; + std::filesystem::path path2 = L"c:\\Users\\testUser\\AppData\\Local\\Microsoft\\WinGet\\Packages"; std::filesystem::path path3 = L"localPath\\test\\folder"; + std::filesystem::path path4 = L"\\test\\folder"; - std::filesystem::path path4 = L"D:\\test\\folder"; - std::filesystem::path path5 = L"F:\\test\\folder"; - std::filesystem::path path6 = L"d:\\randomFolder"; + std::filesystem::path path5 = L"D:\\test\\folder"; + std::filesystem::path path6 = L"F:\\test\\folder"; + std::filesystem::path path7 = L"d:\\randomFolder"; // Verify that a relative path points to the current volume. - REQUIRE(IsSameVolume(path, path3)); REQUIRE(IsSameVolume(path1, path2)); - REQUIRE(IsSameVolume(path4, path6)); + REQUIRE(IsSameVolume(path5, path7)); + REQUIRE(IsSameVolume(path3, path4)); - REQUIRE_FALSE(IsSameVolume(path1, path4)); REQUIRE_FALSE(IsSameVolume(path1, path5)); - REQUIRE_FALSE(IsSameVolume(path2, path4)); + REQUIRE_FALSE(IsSameVolume(path1, path6)); REQUIRE_FALSE(IsSameVolume(path2, path5)); - REQUIRE_FALSE(IsSameVolume(path3, path4)); + REQUIRE_FALSE(IsSameVolume(path2, path6)); REQUIRE_FALSE(IsSameVolume(path3, path5)); + REQUIRE_FALSE(IsSameVolume(path3, path6)); + REQUIRE_FALSE(IsSameVolume(path5, path6)); REQUIRE_FALSE(IsSameVolume(path4, path5)); } \ No newline at end of file From 29c4ad6df2225d544c923c8eadc5e28f052a7d8c Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Mon, 6 Mar 2023 10:50:38 -0800 Subject: [PATCH 6/9] add comment --- src/AppInstallerCLITests/Filesystem.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/AppInstallerCLITests/Filesystem.cpp b/src/AppInstallerCLITests/Filesystem.cpp index 511e2dda9d..5aec4f30a5 100644 --- a/src/AppInstallerCLITests/Filesystem.cpp +++ b/src/AppInstallerCLITests/Filesystem.cpp @@ -60,6 +60,7 @@ TEST_CASE("VerifySymlink", "[filesystem]") TEST_CASE("VerifyIsSameVolume", "[filesystem]") { + // Note: Pipeline build machine uses 'D:\' as the volume. std::filesystem::path path1 = L"C:\\Program Files\\WinGet\\Packages"; std::filesystem::path path2 = L"c:\\Users\\testUser\\AppData\\Local\\Microsoft\\WinGet\\Packages"; std::filesystem::path path3 = L"localPath\\test\\folder"; @@ -78,8 +79,7 @@ TEST_CASE("VerifyIsSameVolume", "[filesystem]") REQUIRE_FALSE(IsSameVolume(path1, path6)); REQUIRE_FALSE(IsSameVolume(path2, path5)); REQUIRE_FALSE(IsSameVolume(path2, path6)); - REQUIRE_FALSE(IsSameVolume(path3, path5)); REQUIRE_FALSE(IsSameVolume(path3, path6)); REQUIRE_FALSE(IsSameVolume(path5, path6)); - REQUIRE_FALSE(IsSameVolume(path4, path5)); + REQUIRE_FALSE(IsSameVolume(path4, path6)); } \ No newline at end of file From 8ff04fd18a723c873688db396906b71619e823aa Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Mon, 13 Mar 2023 10:43:24 -0700 Subject: [PATCH 7/9] address comments --- src/AppInstallerCLICore/PortableInstaller.cpp | 1 + src/AppInstallerCommonCore/Filesystem.cpp | 24 +++++++++---------- .../Public/winget/Filesystem.h | 4 ++-- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/AppInstallerCLICore/PortableInstaller.cpp b/src/AppInstallerCLICore/PortableInstaller.cpp index b78790ce65..81db9a78fb 100644 --- a/src/AppInstallerCLICore/PortableInstaller.cpp +++ b/src/AppInstallerCLICore/PortableInstaller.cpp @@ -109,6 +109,7 @@ namespace AppInstaller::CLI::Portable { if (Filesystem::IsSameVolume(entry.CurrentPath, filePath)) { + AICLI_LOG(Core, Info, << "Renaming directory to: " << filePath); Filesystem::RenameFile(entry.CurrentPath, filePath); } else diff --git a/src/AppInstallerCommonCore/Filesystem.cpp b/src/AppInstallerCommonCore/Filesystem.cpp index 97a829faa6..45bd68824d 100644 --- a/src/AppInstallerCommonCore/Filesystem.cpp +++ b/src/AppInstallerCommonCore/Filesystem.cpp @@ -239,17 +239,17 @@ namespace AppInstaller::Filesystem return knownFolder.get(); } - bool IsSameVolume(const std::filesystem::path& current, const std::filesystem::path& target) - { - LPWSTR currentVolumeBuffer = new WCHAR[MAX_PATH]; - LPWSTR targetVolumeBuffer = new WCHAR[MAX_PATH]; - - GetVolumePathNameW(current.c_str(), currentVolumeBuffer, sizeof(currentVolumeBuffer)); - GetVolumePathNameW(target.c_str(), targetVolumeBuffer, sizeof(targetVolumeBuffer)); - - std::wstring_view currentVolume { currentVolumeBuffer }; - std::wstring_view targetVolume { targetVolumeBuffer }; - - return Utility::CaseInsensitiveEquals(Utility::ConvertToUTF8(currentVolume), Utility::ConvertToUTF8(targetVolume)); + bool IsSameVolume(const std::filesystem::path& path1, const std::filesystem::path& path2) + { + std::wstring path1Volume; + std::wstring path2Volume; + DWORD path1Length = static_cast(path1.u16string().length()); + DWORD path2Length = static_cast(path2.u16string().length()); + path1Volume.resize(path1Length); + path2Volume.resize(path2Length); + + GetVolumePathNameW(path1.c_str(), path1Volume.data(), path1Length); + GetVolumePathNameW(path2.c_str(), path2Volume.data(), path2Length); + return Utility::CaseInsensitiveEquals(Utility::ConvertToUTF8(path1Volume.data()), Utility::ConvertToUTF8(path2Volume.data())); } } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Public/winget/Filesystem.h b/src/AppInstallerCommonCore/Public/winget/Filesystem.h index da0036bc01..ca7a777e15 100644 --- a/src/AppInstallerCommonCore/Public/winget/Filesystem.h +++ b/src/AppInstallerCommonCore/Public/winget/Filesystem.h @@ -43,6 +43,6 @@ namespace AppInstaller::Filesystem // Gets the path of a known folder. std::filesystem::path GetKnownFolderPath(const KNOWNFOLDERID& id); - // Verifies that the target path is on the same volume as the current path. - bool IsSameVolume(const std::filesystem::path& current, const std::filesystem::path& target); + // Verifies that the paths are on the same volume. + bool IsSameVolume(const std::filesystem::path& path1, const std::filesystem::path& path2); } \ No newline at end of file From 66de40f7814bdecd46a3089049890952e700d4e0 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Mon, 13 Mar 2023 15:21:16 -0700 Subject: [PATCH 8/9] fix length --- src/AppInstallerCommonCore/Filesystem.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/AppInstallerCommonCore/Filesystem.cpp b/src/AppInstallerCommonCore/Filesystem.cpp index 45bd68824d..902655f7e1 100644 --- a/src/AppInstallerCommonCore/Filesystem.cpp +++ b/src/AppInstallerCommonCore/Filesystem.cpp @@ -243,8 +243,8 @@ namespace AppInstaller::Filesystem { std::wstring path1Volume; std::wstring path2Volume; - DWORD path1Length = static_cast(path1.u16string().length()); - DWORD path2Length = static_cast(path2.u16string().length()); + DWORD path1Length = static_cast(path1.wstring().length() + 1); + DWORD path2Length = static_cast(path2.wstring().length() + 1); path1Volume.resize(path1Length); path2Volume.resize(path2Length); From af3d6ed1c55700ca014a4350b0d30f931cb63e9a Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Mon, 13 Mar 2023 16:27:19 -0700 Subject: [PATCH 9/9] use maxpath --- src/AppInstallerCLITests/Filesystem.cpp | 8 ++++++-- src/AppInstallerCommonCore/Filesystem.cpp | 19 +++++++++---------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/AppInstallerCLITests/Filesystem.cpp b/src/AppInstallerCLITests/Filesystem.cpp index 5aec4f30a5..f3bff14163 100644 --- a/src/AppInstallerCLITests/Filesystem.cpp +++ b/src/AppInstallerCLITests/Filesystem.cpp @@ -64,16 +64,19 @@ TEST_CASE("VerifyIsSameVolume", "[filesystem]") std::filesystem::path path1 = L"C:\\Program Files\\WinGet\\Packages"; std::filesystem::path path2 = L"c:\\Users\\testUser\\AppData\\Local\\Microsoft\\WinGet\\Packages"; std::filesystem::path path3 = L"localPath\\test\\folder"; - std::filesystem::path path4 = L"\\test\\folder"; - + std::filesystem::path path4 = L"test\\folder"; std::filesystem::path path5 = L"D:\\test\\folder"; std::filesystem::path path6 = L"F:\\test\\folder"; std::filesystem::path path7 = L"d:\\randomFolder"; + std::filesystem::path path8 = L"f:\\randomFolder"; + std::filesystem::path path9 = L"a"; + std::filesystem::path path10 = L"b"; // Verify that a relative path points to the current volume. REQUIRE(IsSameVolume(path1, path2)); REQUIRE(IsSameVolume(path5, path7)); REQUIRE(IsSameVolume(path3, path4)); + REQUIRE(IsSameVolume(path9, path10)); REQUIRE_FALSE(IsSameVolume(path1, path5)); REQUIRE_FALSE(IsSameVolume(path1, path6)); @@ -82,4 +85,5 @@ TEST_CASE("VerifyIsSameVolume", "[filesystem]") REQUIRE_FALSE(IsSameVolume(path3, path6)); REQUIRE_FALSE(IsSameVolume(path5, path6)); REQUIRE_FALSE(IsSameVolume(path4, path6)); + REQUIRE_FALSE(IsSameVolume(path6, path8)); } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Filesystem.cpp b/src/AppInstallerCommonCore/Filesystem.cpp index 902655f7e1..01c4bd5260 100644 --- a/src/AppInstallerCommonCore/Filesystem.cpp +++ b/src/AppInstallerCommonCore/Filesystem.cpp @@ -241,15 +241,14 @@ namespace AppInstaller::Filesystem bool IsSameVolume(const std::filesystem::path& path1, const std::filesystem::path& path2) { - std::wstring path1Volume; - std::wstring path2Volume; - DWORD path1Length = static_cast(path1.wstring().length() + 1); - DWORD path2Length = static_cast(path2.wstring().length() + 1); - path1Volume.resize(path1Length); - path2Volume.resize(path2Length); - - GetVolumePathNameW(path1.c_str(), path1Volume.data(), path1Length); - GetVolumePathNameW(path2.c_str(), path2Volume.data(), path2Length); - return Utility::CaseInsensitiveEquals(Utility::ConvertToUTF8(path1Volume.data()), Utility::ConvertToUTF8(path2Volume.data())); + WCHAR volumeName1[MAX_PATH]; + WCHAR volumeName2[MAX_PATH]; + + // Note: GetVolumePathNameW will return false if the volume drive does not exist. + if (!GetVolumePathNameW(path1.c_str(), volumeName1, MAX_PATH) || !GetVolumePathNameW(path2.c_str(), volumeName2, MAX_PATH)) + { + return false; + } + return Utility::CaseInsensitiveEquals(Utility::ConvertToUTF8(volumeName1), Utility::ConvertToUTF8(volumeName2)); } } \ No newline at end of file