Merge pull request #16 from oreiche/stable-1.3

Release 1.3.2

CHANGELOG.md

@@ -1,3 +1,27 @@
+## Release `1.3.2` (2024-09-09)
+
+Bug fixes on top of `1.3.1`.
+
+### Fixes
+
+- Portability improvements of the code by not relying on implementation
+  details of the compiler.
+- Target-level cache entries are only written if all export targets
+  depended upon are also written to or found in cache; previously,
+  it was assumed that all export targets not analysed locally
+  were local cache hits, an assumption that no longer holds in
+  the presence of serve endpoints. This fixes a cache consistency
+  problem if the same remote-execution endpoint is used both, with
+  and without a serve endpoint.
+- A race condition in reconstructing executables from large CAS
+  has been removed that could lead to an open file descriptor being
+  kept alive for too long, resulting EBUSY failures of actions
+  using this binary.
+- Inside action descriptions, paths are always normalized; this improves
+  compatibility with existing remote-execution implementations.
+- `just-mr --help` now returns exit code 0
+- Missing output directories of actions are now reported properly.
+
 ## Release `1.3.1` (2024-05-22)
 
 Bug fixes on top of `1.3.0`.

src/buildtool/build_engine/target_map/built_in_rules.cpp

@@ -1113,7 +1113,7 @@ void GenericRuleWithDeps(
                               true);
                     return;
                 }
-                outs.emplace_back(x->String());
+                outs.emplace_back(ToNormalPath(x->String()).string());
             }
         }
     }
@@ -1144,7 +1144,7 @@ void GenericRuleWithDeps(
                         true);
                     return;
                 }
-                out_dirs.emplace_back(x->String());
+                out_dirs.emplace_back(ToNormalPath(x->String()).string());
             }
         }
     }

src/buildtool/build_engine/target_map/target_map.cpp

@@ -552,16 +552,29 @@ void withDependencies(
                   throw Evaluator::EvaluationError{
                       "either outs or out_dirs must be specified for ACTION"};
               }
-
-              sort_and_deduplicate(&outputs);
-              sort_and_deduplicate(&output_dirs);
+              ActionDescription::outputs_t outputs_norm{};
+              ActionDescription::outputs_t output_dirs_norm{};
+              outputs_norm.reserve(outputs.size());
+              output_dirs_norm.reserve(output_dirs.size());
+              std::for_each(
+                  outputs.begin(), outputs.end(), [&outputs_norm](auto p) {
+                      outputs_norm.emplace_back(ToNormalPath(p));
+                  });
+              std::for_each(output_dirs.begin(),
+                            output_dirs.end(),
+                            [&output_dirs_norm](auto p) {
+                                output_dirs_norm.emplace_back(ToNormalPath(p));
+                            });
+
+              sort_and_deduplicate(&outputs_norm);
+              sort_and_deduplicate(&output_dirs_norm);
 
               // find entries present on both fields
               std::vector<std::string> dups{};
-              std::set_intersection(outputs.begin(),
-                                    outputs.end(),
-                                    output_dirs.begin(),
-                                    output_dirs.end(),
+              std::set_intersection(outputs_norm.begin(),
+                                    outputs_norm.end(),
+                                    output_dirs_norm.begin(),
+                                    output_dirs_norm.end(),
                                     std::back_inserter(dups));
               if (not dups.empty()) {
                   throw Evaluator::EvaluationError{
@@ -691,8 +704,8 @@ void withDependencies(
                   }
               }
               auto action = BuildMaps::Target::Utils::createAction(
-                  outputs,
-                  output_dirs,
+                  outputs_norm,
+                  output_dirs_norm,
                   std::move(cmd),
                   env_exp,
                   may_fail,
@@ -706,12 +719,12 @@ void withDependencies(
               for (auto const& out : outputs) {
                   result.emplace(out,
                                  ExpressionPtr{ArtifactDescription{
-                                     action_id, std::filesystem::path{out}}});
+                                     action_id, ToNormalPath(out)}});
               }
               for (auto const& out : output_dirs) {
                   result.emplace(out,
                                  ExpressionPtr{ArtifactDescription{
-                                     action_id, std::filesystem::path{out}}});
+                                     action_id, ToNormalPath(out)}});
               }
 
               return ExpressionPtr{Expression::map_t{result}};

src/buildtool/execution_api/bazel_msg/bazel_blob_container.hpp

@@ -230,13 +230,14 @@ class BlobContainer {
     explicit BlobContainer(std::vector<BazelBlob> blobs) {
         blobs_.reserve(blobs.size());
         for (auto& blob : blobs) {
-            blobs_.emplace(blob.digest, std::move(blob));
+            this->Emplace(std::move(blob));
         }
     }
 
     /// \brief Emplace new BazelBlob to container.
     void Emplace(BazelBlob&& blob) {
-        blobs_.emplace(blob.digest, std::move(blob));
+        bazel_re::Digest digest = blob.digest;
+        blobs_.emplace(std::move(digest), std::move(blob));
     }
 
     /// \brief Clear all BazelBlobs from container.

src/buildtool/execution_engine/executor/executor.hpp

@@ -559,7 +559,10 @@ class ExecutorImpl {
                     "action executed with missing outputs.\n"
                     " Action outputs should be the following artifacts:"};
                 for (auto const& output : output_files) {
-                    message += "\n  - " + output;
+                    message += "\n  - file: " + output;
+                }
+                for (auto const& output : output_dirs) {
+                    message += "\n  - dir: " + output;
                 }
                 return message;
             });

src/buildtool/main/build_utils.cpp

@@ -83,8 +83,15 @@ auto CreateTargetCacheWriterMap(
             TargetCacheKey tc_key{key};
             // check if entry actually needs storing
             if (not cache_targets.contains(tc_key)) {
-                // entry already in target-cache, so nothing to be done
-                (*setter)(nullptr);
+                if (tc.Read(tc_key)) {
+                    // entry already in target-cache, so nothing to be done
+                    (*setter)(nullptr);
+                    return;
+                }
+                (*logger)(fmt::format("Export target {} not analysed locally; "
+                                      "not caching anything depending on it",
+                                      key.ToString()),
+                          true);
                 return;
             }
             auto const& target = cache_targets.at(tc_key);

src/buildtool/main/version.cpp

@@ -22,7 +22,7 @@
 auto version() -> std::string {
     std::size_t major = 1;
     std::size_t minor = 3;
-    std::size_t revision = 1;
+    std::size_t revision = 2;
     std::string suffix = std::string{};
 #ifdef VERSION_EXTRA_SUFFIX
     suffix += VERSION_EXTRA_SUFFIX;

src/buildtool/storage/local_cas.tpp

@@ -321,8 +321,16 @@ requires(kIsLocalGeneration) auto LocalCAS<kDoGlobalUplink>::LocalUplinkBlob(
     }
 
     // Uplink blob from older generation to the latest generation.
-    return blob_path_latest.has_value() or
-           latest.StoreBlob</*kOwner=*/true>(*blob_path, is_executable);
+    if (spliced and is_executable) {
+        // During multithreaded splicing, the main process can be forked
+        // (inheriting open file descriptors). In this case, an executable file
+        // saved using hardlinking becomes inaccessible. To prevent this,
+        // executables must be stored as copies made in a child process.
+        return latest.StoreBlob</*kOwner=*/false>(*blob_path, is_executable)
+            .has_value();
+    }
+    return latest.StoreBlob</*kOwner=*/true>(*blob_path, is_executable)
+        .has_value();
 }
 
 template <bool kDoGlobalUplink>

src/other_tools/just_mr/main.cpp

@@ -132,8 +132,11 @@ void SetupSetupCommandArguments(
     try {
         app.parse(argc, argv);
     } catch (CLI::Error& e) {
-        [[maybe_unused]] auto err = app.exit(e);
-        std::exit(kExitClargsError);
+        // CLI11 throws for things like --help calls for them to be handled
+        // separately by parse callers. In this case it nevertheless sets the
+        // error code to 0 (success).
+        auto const err = app.exit(e);
+        std::exit(err == 0 ? kExitSuccess : kExitClargsError);
     } catch (std::exception const& ex) {
         Logger::Log(LogLevel::Error, "Command line parse error: {}", ex.what());
         std::exit(kExitClargsError);