diff options
4 files changed, 124 insertions, 18 deletions
diff --git a/src/buildtool/execution_api/local/local_api.hpp b/src/buildtool/execution_api/local/local_api.hpp index ebbb10ef..03f116a8 100644 --- a/src/buildtool/execution_api/local/local_api.hpp +++ b/src/buildtool/execution_api/local/local_api.hpp @@ -62,7 +62,8 @@ class LocalApi final : public IExecutionApi { if (not blob_path or not FileSystemManager::CreateDirectory( output_paths[i].parent_path()) or - not FileSystemManager::CopyFileAs( + not FileSystemManager::CopyFileAs</*kSetEpochTime=*/true, + /*kSetWritable=*/true>( *blob_path, output_paths[i], info.type)) { return false; } diff --git a/src/buildtool/execution_api/remote/bazel/bazel_api.cpp b/src/buildtool/execution_api/remote/bazel/bazel_api.cpp index 28425400..ca5d4946 100644 --- a/src/buildtool/execution_api/remote/bazel/bazel_api.cpp +++ b/src/buildtool/execution_api/remote/bazel/bazel_api.cpp @@ -95,7 +95,8 @@ auto BazelApi::CreateAction( for (std::size_t pos = 0; pos < blobs.size(); ++pos) { auto gpos = artifact_pos[count + pos]; auto const& type = artifacts_info[gpos].type; - if (not FileSystemManager::WriteFileAs( + if (not FileSystemManager::WriteFileAs</*kSetEpochTime=*/true, + /*kSetWritable=*/true>( blobs[pos].data, output_paths[gpos], type)) { return false; } diff --git a/src/buildtool/file_system/file_system_manager.hpp b/src/buildtool/file_system/file_system_manager.hpp index f2efbc3c..50118f81 100644 --- a/src/buildtool/file_system/file_system_manager.hpp +++ b/src/buildtool/file_system/file_system_manager.hpp @@ -212,7 +212,9 @@ class FileSystemManager { return CopyFileImpl(src, dst, opt); } - template <ObjectType kType, bool kSetEpochTime = false> + template <ObjectType kType, + bool kSetEpochTime = false, + bool kSetWritable = false> requires(IsFileObject(kType)) [[nodiscard]] static auto CopyFileAs( std::filesystem::path const& src, std::filesystem::path const& dst, @@ -221,11 +223,12 @@ class FileSystemManager { std::filesystem::copy_options::overwrite_existing) noexcept -> bool { return CopyFile(src, dst, fd_less, opt) and - SetFilePermissions(dst, IsExecutableObject(kType)) and + SetFilePermissions<kSetWritable>(dst, + IsExecutableObject(kType)) and (not kSetEpochTime or SetEpochTime(dst)); } - template <bool kSetEpochTime = false> + template <bool kSetEpochTime = false, bool kSetWritable = false> [[nodiscard]] static auto CopyFileAs( std::filesystem::path const& src, std::filesystem::path const& dst, @@ -236,11 +239,13 @@ class FileSystemManager { -> bool { switch (type) { case ObjectType::File: - return CopyFileAs<ObjectType::File, kSetEpochTime>( - src, dst, fd_less, opt); + return CopyFileAs<ObjectType::File, + kSetEpochTime, + kSetWritable>(src, dst, fd_less, opt); case ObjectType::Executable: - return CopyFileAs<ObjectType::Executable, kSetEpochTime>( - src, dst, fd_less, opt); + return CopyFileAs<ObjectType::Executable, + kSetEpochTime, + kSetWritable>(src, dst, fd_less, opt); case ObjectType::Tree: break; } @@ -522,18 +527,21 @@ class FileSystemManager { return WriteFileImpl(content, file); } - template <ObjectType kType, bool kSetEpochTime = false> + template <ObjectType kType, + bool kSetEpochTime = false, + bool kSetWritable = false> requires(IsFileObject(kType)) [[nodiscard]] static auto WriteFileAs(std::string const& content, std::filesystem::path const& file, bool fd_less = false) noexcept -> bool { return WriteFile(content, file, fd_less) and - SetFilePermissions(file, IsExecutableObject(kType)) and + SetFilePermissions<kSetWritable>(file, + IsExecutableObject(kType)) and (not kSetEpochTime or SetEpochTime(file)); } - template <bool kSetEpochTime = false> + template <bool kSetEpochTime = false, bool kSetWritable = false> [[nodiscard]] static auto WriteFileAs(std::string const& content, std::filesystem::path const& file, ObjectType output_type, @@ -541,11 +549,13 @@ class FileSystemManager { -> bool { switch (output_type) { case ObjectType::File: - return WriteFileAs<ObjectType::File, kSetEpochTime>( - content, file, fd_less); + return WriteFileAs<ObjectType::File, + kSetEpochTime, + kSetWritable>(content, file, fd_less); case ObjectType::Executable: - return WriteFileAs<ObjectType::Executable, kSetEpochTime>( - content, file, fd_less); + return WriteFileAs<ObjectType::Executable, + kSetEpochTime, + kSetWritable>(content, file, fd_less); case ObjectType::Tree: return false; } @@ -677,12 +687,17 @@ class FileSystemManager { } /// \brief Set special permissions for files. - /// Set to 0444 for non-executables and set to 0555 for executables. + /// By default, we set to 0444 for non-executables and set to 0555 for + /// executables. When we install or install-cas, we add the owner write + /// permission to allow for, e.g., overwriting if we re-install the same + /// target after a recompilation + template <bool kSetWritable = false> static auto SetFilePermissions(std::filesystem::path const& path, bool is_executable) noexcept -> bool { try { using std::filesystem::perms; - perms p{perms::owner_read | perms::group_read | perms::others_read}; + perms p{(kSetWritable ? perms::owner_write : perms::none) | + perms::owner_read | perms::group_read | perms::others_read}; if (is_executable) { p |= perms::owner_exec | perms::group_exec | perms::others_exec; } diff --git a/test/buildtool/file_system/file_system_manager.test.cpp b/test/buildtool/file_system/file_system_manager.test.cpp index 607e3dd2..dfbbd1ce 100644 --- a/test/buildtool/file_system/file_system_manager.test.cpp +++ b/test/buildtool/file_system/file_system_manager.test.cpp @@ -53,6 +53,8 @@ namespace fs = std::filesystem; constexpr auto kFilePerms = fs::perms::owner_read | fs::perms::group_read | fs::perms::others_read; +constexpr auto kInstalledPerms = fs::perms::owner_write; + constexpr auto kExecPerms = fs::perms::owner_exec | fs::perms::group_exec | fs::perms::others_exec; @@ -64,6 +66,14 @@ auto HasFilePermissions(fs::path const& path) noexcept -> bool { } } +auto HasInstalledFilePermissions(fs::path const& path) noexcept -> bool { + try { + return fs::status(path).permissions() == (kFilePerms | kInstalledPerms); + } catch (...) { + return false; + } +} + auto HasExecutablePermissions(fs::path const& path) noexcept -> bool { try { return fs::status(path).permissions() == (kFilePerms | kExecPerms); @@ -72,6 +82,15 @@ auto HasExecutablePermissions(fs::path const& path) noexcept -> bool { } } +auto HasInstalledExecutablePermissions(fs::path const& path) noexcept -> bool { + try { + return fs::status(path).permissions() == + (kFilePerms | kExecPerms | kInstalledPerms); + } catch (...) { + return false; + } +} + auto HasEpochTime(fs::path const& path) noexcept -> bool { try { return fs::last_write_time(path) == @@ -232,6 +251,41 @@ TEST_CASE_METHOD(CopyFileFixture, "CopyFileAs", "[file_system]") { run_test.template operator()<true>(true); } } + SECTION("as installed file") { + auto run_test = [&]<bool kSetEpochTime = false>(bool fd_less) { + // Copy as file was successful + CHECK(FileSystemManager::CopyFileAs<kSetEpochTime, + /*kSetWritable=*/true>( + from_, to_, ObjectType::File, fd_less)); + + // file exists + CHECK(std::filesystem::exists(to_)); + CHECK(std::filesystem::is_regular_file(to_)); + CHECK(not FileSystemManager::IsExecutable(to_)); + + // Contents are equal + auto const content_from = FileSystemManager::ReadFile(from_); + CHECK(content_from.has_value()); + auto const content_to = FileSystemManager::ReadFile(to_); + CHECK(content_to.has_value()); + CHECK(content_from == content_to); + + // permissions should be 0644 + CHECK(HasInstalledFilePermissions(to_)); + if constexpr (kSetEpochTime) { + CHECK(HasEpochTime(to_)); + } + }; + + SECTION("direct") { run_test(false); } + SECTION("fd_less") { run_test(true); } + SECTION("direct with epoch") { + run_test.template operator()<true>(false); + } + SECTION("fd_less with epoch") { + run_test.template operator()<true>(true); + } + } SECTION("as executable") { auto run_test = [&]<bool kSetEpochTime = false>(bool fd_less) { // Copy as file was successful @@ -266,6 +320,41 @@ TEST_CASE_METHOD(CopyFileFixture, "CopyFileAs", "[file_system]") { run_test.template operator()<true>(true); } } + SECTION("as installed executable") { + auto run_test = [&]<bool kSetEpochTime = false>(bool fd_less) { + // Copy as file was successful + CHECK(FileSystemManager::CopyFileAs<kSetEpochTime, + /*kSetWritable=*/true>( + from_, to_, ObjectType::Executable, fd_less)); + + // file exists + CHECK(std::filesystem::exists(to_)); + CHECK(std::filesystem::is_regular_file(to_)); + CHECK(FileSystemManager::IsExecutable(to_)); + + // Contents are equal + auto const content_from = FileSystemManager::ReadFile(from_); + CHECK(content_from.has_value()); + auto const content_to = FileSystemManager::ReadFile(to_); + CHECK(content_to.has_value()); + CHECK(content_from == content_to); + + // permissions should be 0755 + CHECK(HasInstalledExecutablePermissions(to_)); + if constexpr (kSetEpochTime) { + CHECK(HasEpochTime(to_)); + } + }; + + SECTION("direct") { run_test(false); } + SECTION("fd_less") { run_test(true); } + SECTION("direct with epoch") { + run_test.template operator()<true>(false); + } + SECTION("fd_less with epoch") { + run_test.template operator()<true>(true); + } + } } TEST_CASE("RemoveFile", "[file_system]") { |