Merge pull request #6 from oreiche/release-1.1.3

Release 1.1.3

CHANGELOG.md

@@ -1,3 +1,20 @@
+## Release `1.1.3` (2023-07-14)
+
+Bug fixes on top of release `1.1.2`.
+
+### Fixes
+
+- `just` no longer unnecessarily errors out on absence of
+  a working directory.
+- `just-mr` now correctly performs a forced add in order to stage
+  all entries in a Git repository. Previously it was possible for
+  entries to be skipped inadvertently in, e.g., imported archives
+  if `gitignore` files were present.
+- Temporary files generated by `just execute` are now created inside
+  the local build root.
+- Fix an issue in `just-mr` that caused the unpack of archives to
+  fail if they were compressed with bzip2.
+
 ## Release `1.1.2` (2023-06-09)
 
 Bug fixes on top of release `1.1.1`.

bin/just-mr.py

@@ -474,7 +474,7 @@ def import_to_git(target, repo_type, content_id):
         env=dict(os.environ, **GIT_NOBODY_ENV),
     )
     run_cmd(
-        ["git", "add", "."],
+        ["git", "add", "-f", "."],
         cwd=target,
         env=dict(os.environ, **GIT_NOBODY_ENV),
     )
@@ -582,7 +582,9 @@ def distdir_checkout(desc, repos):
             content[get_distfile(repo_desc)] = content_id
 
     # Hash the map as unique id for the distdir repo entry
-    distdir_content_id = git_hash(json.dumps(content, sort_keys=True, separators=(',', ':')).encode('utf-8'))
+    distdir_content_id = git_hash(
+        json.dumps(content, sort_keys=True,
+                   separators=(',', ':')).encode('utf-8'))
     target_distdir_dir = distdir_repo_dir(distdir_content_id)
 
     # Check if content already exists

src/buildtool/common/TARGETS

@@ -14,6 +14,8 @@
     , ["src/buildtool/build_engine/expression", "expression"]
     , ["src/buildtool/compatibility", "compatibility"]
     , ["src/buildtool/logging", "log_level"]
+    , ["src/buildtool/logging", "logging"]
+    , ["src/utils/cpp", "path"]
     , ["@", "cli11", "", "cli11"]
     , ["@", "json", "", "json"]
     , ["@", "fmt", "", "fmt"]

src/buildtool/common/cli.hpp

@@ -30,6 +30,7 @@
 #include "src/buildtool/common/clidefaults.hpp"
 #include "src/buildtool/compatibility/compatibility.hpp"
 #include "src/buildtool/logging/log_level.hpp"
+#include "src/utils/cpp/path.hpp"
 
 constexpr auto kDefaultTimeout = std::chrono::milliseconds{300000};
 
@@ -168,8 +169,19 @@ static inline auto SetupCommonArguments(
     app->add_option_function<std::string>(
            "-w,--workspace-root",
            [clargs](auto const& workspace_root_raw) {
-               clargs->workspace_root = std::filesystem::canonical(
-                   std::filesystem::absolute(workspace_root_raw));
+               std::filesystem::path root = ToNormalPath(workspace_root_raw);
+               if (not root.is_absolute()) {
+                   try {
+                       root = std::filesystem::absolute(root);
+                   } catch (std::exception const& e) {
+                       Logger::Log(LogLevel::Error,
+                                   "Failed to convert workspace root {} ({})",
+                                   workspace_root_raw,
+                                   e.what());
+                       throw e;
+                   }
+               }
+               clargs->workspace_root = root;
            },
            "Path of the workspace's root directory.")
         ->type_name("PATH");
@@ -342,8 +354,20 @@ static inline auto SetupCacheArguments(
     app->add_option_function<std::string>(
            "--local-build-root",
            [clargs](auto const& build_root_raw) {
-               clargs->local_root = std::filesystem::weakly_canonical(
-                   std::filesystem::absolute(build_root_raw));
+               std::filesystem::path root = ToNormalPath(build_root_raw);
+               if (!root.is_absolute()) {
+                   try {
+                       root = std::filesystem::absolute(root);
+                   } catch (std::exception const& e) {
+                       Logger::Log(
+                           LogLevel::Error,
+                           "Failed to convert local build root {} ({}).",
+                           build_root_raw,
+                           e.what());
+                       throw e;
+                   }
+               }
+               clargs->local_root = root;
            },
            "Root for local CAS, cache, and build directories.")
         ->type_name("PATH");
@@ -420,8 +444,20 @@ static inline auto SetupStageArguments(
     app->add_option_function<std::string>(
            "-o,--output-dir",
            [clargs](auto const& output_dir_raw) {
-               clargs->output_dir = std::filesystem::weakly_canonical(
-                   std::filesystem::absolute(output_dir_raw));
+               std::filesystem::path out = ToNormalPath(output_dir_raw);
+               if (not out.is_absolute()) {
+                   try {
+                       out = std::filesystem::absolute(out);
+                   } catch (std::exception const& e) {
+                       Logger::Log(
+                           LogLevel::Error,
+                           "Failed to convert output directory {} ({}).",
+                           output_dir_raw,
+                           e.what());
+                       throw e;
+                   }
+               }
+               clargs->output_dir = out;
            },
            "Path of the directory where outputs will be copied.")
         ->type_name("PATH")
@@ -459,8 +495,19 @@ static inline auto SetupFetchArguments(
     app->add_option_function<std::string>(
            "-o,--output-path",
            [clargs](auto const& output_path_raw) {
-               clargs->output_path = std::filesystem::weakly_canonical(
-                   std::filesystem::absolute(output_path_raw));
+               std::filesystem::path out = ToNormalPath(output_path_raw);
+               if (not out.is_absolute()) {
+                   try {
+                       out = std::filesystem::absolute(out);
+                   } catch (std::exception const& e) {
+                       Logger::Log(LogLevel::Error,
+                                   "Failed to convert output path {} ({})",
+                                   output_path_raw,
+                                   e.what());
+                       throw e;
+                   }
+               }
+               clargs->output_path = out;
            },
            "Install path for the artifact. (omit to dump to stdout)")
         ->type_name("PATH");

src/buildtool/execution_api/execution_service/bytestream_server.cpp

@@ -113,7 +113,14 @@ auto BytestreamServiceImpl::Write(
                  *hash,
                  request.write_offset(),
                  request.finish_write());
-    auto tmp_dir = TmpDir::Create("execution-service");
+    auto lock = GarbageCollector::SharedLock();
+    if (!lock) {
+        auto str = fmt::format("Could not acquire SharedLock");
+        logger_.Emit(LogLevel::Error, str);
+        return grpc::Status{grpc::StatusCode::INTERNAL, str};
+    }
+    auto tmp_dir = TmpDir::Create(StorageConfig::GenerationCacheRoot(0) /
+                                  "execution-service");
     if (!tmp_dir) {
         return ::grpc::Status{::grpc::StatusCode::INTERNAL,
                               "could not create TmpDir"};
@@ -129,21 +136,16 @@ auto BytestreamServiceImpl::Write(
         of.write(request.data().data(),
                  static_cast<std::streamsize>(request.data().size()));
     }
-    auto lock = GarbageCollector::SharedLock();
-    if (!lock) {
-        auto str = fmt::format("Could not acquire SharedLock");
-        logger_.Emit(LogLevel::Error, str);
-        return grpc::Status{grpc::StatusCode::INTERNAL, str};
-    }
     if (NativeSupport::IsTree(*hash)) {
-        if (not storage_->CAS().StoreTree(tmp)) {
+        if (not storage_->CAS().StoreTree</*kOwner=*/true>(tmp)) {
             auto str = fmt::format("could not store tree {}", *hash);
             logger_.Emit(LogLevel::Error, str);
             return ::grpc::Status{::grpc::StatusCode::INVALID_ARGUMENT, str};
         }
     }
     else {
-        if (not storage_->CAS().StoreBlob(tmp, /*is_executable=*/false)) {
+        if (not storage_->CAS().StoreBlob</*kOwner=*/true>(
+                tmp, /*is_executable=*/false)) {
             auto str = fmt::format("could not store blob {}", *hash);
             logger_.Emit(LogLevel::Error, str);
             return ::grpc::Status{::grpc::StatusCode::INVALID_ARGUMENT, str};

src/buildtool/file_system/TARGETS

@@ -57,6 +57,7 @@
   , "private-deps":
     [ ["src/buildtool/logging", "logging"]
     , ["src/utils/cpp", "hex_string"]
+    , ["src/utils/cpp", "path"]
     , ["", "libgit2"]
     ]
   }
@@ -104,6 +105,7 @@
     , ["src/utils/cpp", "path"]
     , ["src/utils/cpp", "hex_string"]
     , ["src/utils/cpp", "gsl"]
+    , ["src/buildtool/file_system", "file_system_manager"]
     ]
   , "cflags": ["-pthread"]
   }

src/buildtool/file_system/file_system_manager.hpp

@@ -23,6 +23,7 @@
 #include <filesystem>
 #include <fstream>
 #include <optional>
+#include <unordered_set>
 
 #include <fcntl.h>
 
@@ -53,6 +54,9 @@ class FileSystemManager {
     using ReadDirEntryFunc =
         std::function<bool(std::filesystem::path const&, ObjectType type)>;
 
+    using UseDirEntryFunc =
+        std::function<bool(std::filesystem::path const&, bool /*is_tree*/)>;
+
     class DirectoryAnchor {
         friend class FileSystemManager;
 
@@ -577,6 +581,43 @@ class FileSystemManager {
         return true;
     }
 
+    /// \brief Read all entries recursively in a filesystem directory tree.
+    /// \param dir root directory to traverse
+    /// \param use_entry callback to call with found valid entries
+    /// \param ignored_subdirs directory names to be ignored wherever found in
+    ///                        the directory tree of dir.
+    [[nodiscard]] static auto ReadDirectoryEntriesRecursive(
+        std::filesystem::path const& dir,
+        UseDirEntryFunc const& use_entry,
+        std::unordered_set<std::string> const& ignored_subdirs = {}) noexcept
+        -> bool {
+        try {
+            // constructor of this iterator points to end by default;
+            for (auto it = std::filesystem::recursive_directory_iterator(dir);
+                 it != std::filesystem::recursive_directory_iterator();
+                 ++it) {
+                // check for ignored subdirs
+                if (std::filesystem::is_directory(it->symlink_status()) and
+                    ignored_subdirs.contains(*--it->path().end())) {
+                    it.disable_recursion_pending();
+                    continue;
+                }
+                // use the entry
+                if (not use_entry(
+                        it->path().lexically_relative(dir),
+                        std::filesystem::is_directory(it->symlink_status()))) {
+                    return false;
+                }
+            }
+        } catch (std::exception const& ex) {
+            Logger::Log(LogLevel::Error,
+                        "reading directory {} recursively failed",
+                        dir.string());
+            return false;
+        }
+        return true;
+    }
+
     /// \brief Write file
     /// If argument fd_less is given, the write will be performed in a child
     /// process to prevent polluting the parent with open writable file

src/buildtool/file_system/git_cas.cpp

@@ -21,6 +21,7 @@
 #include "gsl/gsl"
 #include "src/buildtool/logging/logger.hpp"
 #include "src/utils/cpp/hex_string.hpp"
+#include "src/utils/cpp/path.hpp"
 
 #ifndef BOOTSTRAP_BUILD_TOOL
 
@@ -156,8 +157,25 @@ auto GitCAS::OpenODB(std::filesystem::path const& repo_path) noexcept -> bool {
         git_repository_odb(&odb_ptr, repo);
         odb_.reset(odb_ptr);  // retain odb pointer
         // set root
-        git_path_ = std::filesystem::weakly_canonical(std::filesystem::absolute(
-            std::filesystem::path(git_repository_path(repo))));
+        std::filesystem::path git_path{};
+        if (git_repository_is_bare(repo) != 0) {
+            git_path = ToNormalPath((git_repository_path(repo)));
+        }
+        else {
+            git_path = ToNormalPath(git_repository_workdir(repo));
+        }
+        if (not git_path.is_absolute()) {
+            try {
+                git_path = std::filesystem::absolute(git_path);
+            } catch (std::exception const& e) {
+                Logger::Log(LogLevel::Error,
+                            "Failed to obtain absolute path for {}: {}",
+                            git_path.string(),
+                            e.what());
+                return false;
+            }
+        }
+        git_path_ = git_path;
         // release resources
         git_repository_free(repo);
     }

src/buildtool/file_system/git_repo.cpp

@@ -16,6 +16,7 @@
 
 #include <thread>
 
+#include "src/buildtool/file_system/file_system_manager.hpp"
 #include "src/buildtool/logging/logger.hpp"
 #include "src/utils/cpp/gsl.hpp"
 #include "src/utils/cpp/hex_string.hpp"
@@ -353,9 +354,15 @@ GitRepo::GitRepo(std::filesystem::path const& repo_path) noexcept {
         }
         cas->odb_.reset(odb_ptr);  // retain odb pointer
         is_repo_fake_ = false;
-        // save root path
-        cas->git_path_ = ToNormalPath(std::filesystem::absolute(
-            std::filesystem::path(git_repository_path(repo_->Ptr()))));
+        // save root path; this differs if repository is bare or not
+        if (git_repository_is_bare(repo_->Ptr()) != 0) {
+            cas->git_path_ = std::filesystem::absolute(
+                ToNormalPath(git_repository_path(repo_->Ptr())));
+        }
+        else {
+            cas->git_path_ = std::filesystem::absolute(
+                ToNormalPath(git_repository_workdir(repo_->Ptr())));
+        }
         // retain the pointer
         git_cas_ = std::static_pointer_cast<GitCAS const>(cas);
     } catch (std::exception const& ex) {
@@ -477,28 +484,39 @@ auto GitRepo::StageAndCommitAllAnonymous(std::string const& message,
         // share the odb lock
         std::shared_lock lock{GetGitCAS()->mutex_};
 
+        // cannot perform this operation on a bare repository; this has to be
+        // checked because git_index_add_bypath will not do it for us!
+        if (not FileSystemManager::Exists(GetGitCAS()->git_path_ / ".git")) {
+            (*logger)("cannot stage and commit files in a bare repository!",
+                      true /*fatal*/);
+            return std::nullopt;
+        }
+
         // add all files to be staged
         git_index* index_ptr{nullptr};
         git_repository_index(&index_ptr, repo_->Ptr());
         auto index = std::unique_ptr<git_index, decltype(&index_closer)>(
             index_ptr, index_closer);
 
-        git_strarray array_obj{};
-        PopulateStrarray(&array_obj, {"."});
-        auto array = std::unique_ptr<git_strarray, decltype(&strarray_deleter)>(
-            &array_obj, strarray_deleter);
-
-        if (git_index_add_all(index.get(), array.get(), 0, nullptr, nullptr) !=
-            0) {
+        // due to mismanagement of .gitignore rules by libgit2 when doing a
+        // forced add all, we resort to using git_index_add_bypath manually for
+        // all entries, instead of git_index_add_all with GIT_INDEX_ADD_FORCE.
+        auto use_entry = [&index](std::filesystem::path const& name,
+                                  bool is_tree) {
+            return is_tree or
+                   git_index_add_bypath(index.get(), name.c_str()) == 0;
+        };
+        if (not FileSystemManager::ReadDirectoryEntriesRecursive(
+                GetGitCAS()->git_path_,
+                use_entry,
+                /*ignored_subdirs=*/{".git"})) {
             (*logger)(fmt::format(
                           "staging files in git repository {} failed with:\n{}",
                           GetGitCAS()->git_path_.string(),
                           GitLastError()),
                       true /*fatal*/);
             return std::nullopt;
         }
-        // release unused resources
-        array.reset(nullptr);
         // build tree from staged files
         git_oid tree_oid;
         if (git_index_write_tree(&tree_oid, index.get()) != 0) {