Merge pull request #15 from oreiche/stable-1.3

Release v1.3.1

CHANGELOG.md

@@ -1,3 +1,15 @@
+## Release `1.3.1` (2024-05-22)
+
+Bug fixes on top of `1.3.0`.
+
+### Fixes
+
+- A bug was fixed that cased `just serve` to fail with an internal
+  error when building against ignore-special roots.
+- `just` now accurately reports internal errors that occured on
+  the serve endpoint.
+- Dependencies have been updated to also build with gcc 14.
+
 ## Release `1.3.0` (2024-05-08)
 
 A feature release on top of `1.2.0`, backwards compatible.

etc/defaults/patch/TARGETS.boringssl

@@ -0,0 +1,3 @@
+{ "defaults":
+  {"type": ["patch", "defaults"], "base": [["@", "base", "patch", "defaults"]]}
+}

etc/defaults/shell/TARGETS

@@ -0,0 +1,3 @@
+{ "defaults":
+  {"type": "defaults", "base": [["@", "toolchain", "shell", "defaults"]]}
+}

etc/defaults/shell/TARGETS.absl

@@ -0,0 +1,3 @@
+{ "defaults":
+  {"type": "defaults", "base": [["@", "base", "shell", "defaults"]]}
+}

etc/defaults/shell/TARGETS.archive

@@ -0,0 +1,3 @@
+{ "defaults":
+  {"type": "defaults", "base": [["@", "base", "shell", "defaults"]]}
+}

etc/defaults/shell/TARGETS.boringssl

@@ -0,0 +1,3 @@
+{ "defaults":
+  {"type": "defaults", "base": [["@", "base", "shell", "defaults"]]}
+}

etc/defaults/shell/TARGETS.curl

@@ -0,0 +1,3 @@
+{ "defaults":
+  {"type": "defaults", "base": [["@", "base", "shell", "defaults"]]}
+}

etc/defaults/shell/TARGETS.git2

@@ -0,0 +1,3 @@
+{ "defaults":
+  {"type": "defaults", "base": [["@", "base", "shell", "defaults"]]}
+}

etc/defaults/shell/TARGETS.just

@@ -0,0 +1,3 @@
+{ "defaults":
+  {"type": "defaults", "base": [["@", "base", "shell", "defaults"]]}
+}

etc/import/TARGETS.boringssl

@@ -302,6 +302,17 @@
     , "src/third_party/fiat/p256_64_msvc.h"
     ]
   }
+, "src/crypto/internal.h":
+  { "type": ["@", "rules", "patch", "file"]
+  , "src": [["FILE", null, "src/crypto/internal.h"]]
+  , "patch":
+    [ [ "@"
+      , "patches"
+      , ""
+      , "crypto-use-_Generic-only-if-defined-__cplusplus.patch"
+      ]
+    ]
+  }
 , "crypto_sources":
   { "type": "install"
   , "deps":

etc/patches/crypto-use-_Generic-only-if-defined-__cplusplus.patch

@@ -0,0 +1,74 @@
+From 3359a87a71307336100b84e66b69bad385cd3cfc Mon Sep 17 00:00:00 2001
+From: Martin Jansa <[email protected]>
+Date: Mon, 6 May 2024 01:36:39 +0200
+Subject: [PATCH] crypto: use _Generic only if !defined(__cplusplus)
+
+* fixes build with gcc-14 which has __builtin_addc and __builtin_subc
+  with gcc-13 it was already using the #else branch because of missing builtins
+
+* fixes
+  https://github.com/grpc/grpc/issues/35945
+
+* _Generic was introduced in boringssl with:
+  https://boringssl.googlesource.com/boringssl/+/70ca6bc24be103dabd68e448cd3af29b929b771d%5E%21/#F4
+
+* but e.g. third_party/boringssl-with-bazel/src/ssl/d1_both.cc includes
+  this internal.h and from the .cc extension gcc will process it as C++
+  where _Generic isn't available, causing:
+
+In file included from third_party/boringssl-with-bazel/src/ssl/d1_both.cc:125:
+third_party/boringssl-with-bazel/src/ssl/../crypto/internal.h: In function 'uint32_t CRYPTO_addc_u32(uint32_t, uint32_t, uint32_t, uint32_t*)':
+third_party/boringssl-with-bazel/src/ssl/../crypto/internal.h:1159:7: error: expected primary-expression before 'unsigned'
+ 1159 |       unsigned: __builtin_addc,                     \
+      |       ^~~~~~~~
+third_party/boringssl-with-bazel/src/ssl/../crypto/internal.h:1166:10: note: in expansion of macro 'CRYPTO_GENERIC_ADDC'
+ 1166 |   return CRYPTO_GENERIC_ADDC(x, y, carry, out_carry);
+      |          ^~~~~~~~~~~~~~~~~~~
+third_party/boringssl-with-bazel/src/ssl/../crypto/internal.h:1160:7: error: expected primary-expression before 'unsigned'
+ 1160 |       unsigned long: __builtin_addcl,               \
+      |       ^~~~~~~~
+third_party/boringssl-with-bazel/src/ssl/../crypto/internal.h:1166:10: note: in expansion of macro 'CRYPTO_GENERIC_ADDC'
+ 1166 |   return CRYPTO_GENERIC_ADDC(x, y, carry, out_carry);
+      |          ^~~~~~~~~~~~~~~~~~~
+third_party/boringssl-with-bazel/src/ssl/../crypto/internal.h:1161:7: error: expected primary-expression before 'unsigned'
+ 1161 |       unsigned long long: __builtin_addcll))((x), (y), (carry), (out_carry))
+      |       ^~~~~~~~
+third_party/boringssl-with-bazel/src/ssl/../crypto/internal.h:1166:10: note: in expansion of macro 'CRYPTO_GENERIC_ADDC'
+ 1166 |   return CRYPTO_GENERIC_ADDC(x, y, carry, out_carry);
+      |          ^~~~~~~~~~~~~~~~~~~
+third_party/boringssl-with-bazel/src/ssl/../crypto/internal.h:1158:4: error: '_Generic' was not declared in this scope
+ 1158 |   (_Generic((x),                                    \
+      |    ^~~~~~~~
+third_party/boringssl-with-bazel/src/ssl/../crypto/internal.h:1166:10: note: in expansion of macro 'CRYPTO_GENERIC_ADDC'
+ 1166 |   return CRYPTO_GENERIC_ADDC(x, y, carry, out_carry);
+      |          ^~~~~~~~~~~~~~~~~~~
+
+Signed-off-by: Martin Jansa <[email protected]>
+---
+Upstream-Status: Submitted [https://boringssl-review.googlesource.com/c/boringssl/+/68227 crypto: use _Generic only if !defined(__cplusplus)]
+
+ crypto/internal.h | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/crypto/internal.h b/crypto/internal.h
+index a77102d76..30d6826dd 100644
+--- a/crypto/internal.h
++++ b/crypto/internal.h
+@@ -1152,7 +1152,7 @@ static inline uint64_t CRYPTO_rotr_u64(uint64_t value, int shift) {
+
+ // CRYPTO_addc_* returns |x + y + carry|, and sets |*out_carry| to the carry
+ // bit. |carry| must be zero or one.
+-#if OPENSSL_HAS_BUILTIN(__builtin_addc)
++#if OPENSSL_HAS_BUILTIN(__builtin_addc) && !defined(__cplusplus)
+
+ #define CRYPTO_GENERIC_ADDC(x, y, carry, out_carry) \
+   (_Generic((x),                                    \
+@@ -1204,7 +1204,7 @@ static inline uint64_t CRYPTO_addc_u64(uint64_t x, uint64_t y, uint64_t carry,
+
+ // CRYPTO_subc_* returns |x - y - borrow|, and sets |*out_borrow| to the borrow
+ // bit. |borrow| must be zero or one.
+-#if OPENSSL_HAS_BUILTIN(__builtin_subc)
++#if OPENSSL_HAS_BUILTIN(__builtin_subc) && !defined(__cplusplus)
+
+ #define CRYPTO_GENERIC_SUBC(x, y, borrow, out_borrow) \
+   (_Generic((x),                                      \

etc/repos.json

@@ -245,7 +245,7 @@
       }
     , "target_root": "import targets"
     , "target_file_name": "TARGETS.boringssl"
-    , "bindings": {"rules": "rules-boringssl"}
+    , "bindings": {"rules": "rules-boringssl", "patches": "patches"}
     , "bootstrap":
       { "arch_map": {"arm64": "aarch64"}
       , "build": "{cc} {cflags} -I . -I src/include -c *.c src/crypto/*.c  src/crypto/*/*.c src/crypto/*/*.S src/third_party/fiat/asm/*.S {os}-{arch}/crypto/fipsmodule/*.S && {ar} cqs libcrypto.a *.o"

etc/toolchain/shell/TARGETS

@@ -0,0 +1 @@
+{"defaults": {"type": "defaults"}}

src/buildtool/build_engine/target_map/absent_target_map.cpp

@@ -95,30 +95,39 @@ void WithFlexibleVariables(
                       /*fatal=*/true);
             return;
         }
-        if (res->index() == 0) {
-            if (serve_failure_reporter != nullptr) {
-                (*serve_failure_reporter)(key, std::get<0>(*res));
+        switch (auto const& ind = res->index(); ind) {
+            case 0: {
+                if (serve_failure_reporter != nullptr) {
+                    (*serve_failure_reporter)(key, std::get<0>(*res));
+                }
+                // target found but failed to analyse/build: log it as fatal
+                (*logger)(
+                    fmt::format("Failure to remotely analyse or build absent "
+                                "target {}\nDetailed log available on the "
+                                "remote-execution endpoint as blob {}",
+                                key.target.ToString(),
+                                std::get<0>(*res)),
+                    /*fatal=*/true);
+                return;
+            }
+            case 1:  // fallthrough
+            case 2: {
+                // Other errors, including INTERNAL: log it as fatal
+                (*logger)(fmt::format(
+                              "While querying serve endpoint for absent export "
+                              "target {}:\n{}",
+                              key.target.ToString(),
+                              ind == 1 ? std::get<1>(*res) : std::get<2>(*res)),
+                          /*fatal=*/true);
+                return;
+            }
+            default: {
+                // index == 3
+                target_cache_value = std::get<3>(*res);
+                exports_progress->TaskTracker().Stop(task);
+                from_just_serve = true;
             }
-            (*logger)(fmt::format("Failure to remotely analyse or build absent "
-                                  "target {}\nDetailed log available on the "
-                                  "remote-execution endpoint as blob {}",
-                                  key.target.ToString(),
-                                  std::get<0>(*res)),
-                      /*fatal=*/true);
-            return;
-        }
-        if (res->index() == 1) {
-            (*logger)(fmt::format("While querying serve endpoint for "
-                                  "absent export target {}:\n{}",
-                                  key.target.ToString(),
-                                  std::get<1>(*res)),
-                      /*fatal=*/true);
-            return;
         }
-        // index == 2
-        target_cache_value = std::get<2>(*res);
-        exports_progress->TaskTracker().Stop(task);
-        from_just_serve = true;
     }
 
     if (!target_cache_value) {

src/buildtool/build_engine/target_map/export.cpp

@@ -160,31 +160,44 @@ void ExportRule(
                             key.target.ToString());
             }
             else {
-                if (res->index() == 0) {
-                    // target found but failed to analyse/build: this should be
-                    // a fatal error for the local build too
-                    (*logger)(
-                        fmt::format("Failure to remotely analyse or build "
-                                    "target {}\nDetailed log available on the "
-                                    "remote-execution endpoint as blob {}",
-                                    key.target.ToString(),
-                                    std::get<0>(*res)),
-                        /*fatal=*/true);
-                    return;
-                }
-                if (res->index() == 1) {
-                    // some other failure occurred while querying the serve
-                    // endpoint; log to debug and continue locally
-                    Logger::Log(LogLevel::Debug,
-                                "While querying serve endpoint for export "
-                                "target {}:\n{}",
+                switch (res->index()) {
+                    case 0: {
+                        // target found but failed to analyse/build: this should
+                        // be a fatal error for the local build too
+                        (*logger)(
+                            fmt::format(
+                                "Failure to remotely analyse or build target "
+                                "{}\nDetailed log available on the "
+                                "remote-execution endpoint as blob {}",
                                 key.target.ToString(),
-                                std::get<1>(*res));
-                }
-                else {
-                    // index == 2
-                    target_cache_value = std::get<2>(*res);
-                    from_just_serve = true;
+                                std::get<0>(*res)),
+                            /*fatal=*/true);
+                        return;
+                    }
+                    case 1: {
+                        // internal failure on the serve endpoint, or failures
+                        // on the client side: local build should not continue
+                        (*logger)(fmt::format("While querying serve endpoint "
+                                              "for export target {}:\n{}",
+                                              key.target.ToString(),
+                                              std::get<1>(*res)),
+                                  /*fatal=*/true);
+                        return;
+                    }
+                    case 2: {
+                        // some other failure occurred on the serve endpoint;
+                        // log to debug and continue locally
+                        Logger::Log(LogLevel::Debug,
+                                    "While querying serve endpoint for export "
+                                    "target {}:\n{}",
+                                    key.target.ToString(),
+                                    std::get<2>(*res));
+                    } break;
+                    default: {
+                        // index == 3
+                        target_cache_value = std::get<3>(*res);
+                        from_just_serve = true;
+                    }
                 }
             }
             exports_progress->TaskTracker().Stop(task);

src/buildtool/execution_engine/executor/executor.hpp

@@ -269,7 +269,8 @@ class ExecutorImpl {
 
         Logger::Log(LogLevel::Trace, [&tree]() {
             std::ostringstream oss{};
-            oss << "upload directory content of " << tree.Hash() << std::endl;
+            oss << "upload directory content of " << tree.FileRootHash()
+                << std::endl;
             for (auto const& [path, entry] : tree) {
                 oss << fmt::format(" - {}: {}", path, entry->Hash())
                     << std::endl;
@@ -344,7 +345,7 @@ class ExecutorImpl {
             if (not VerifyOrUploadTree(api, *tree)) {
                 Logger::Log(LogLevel::Error,
                             "failed to verifyorupload git tree {} [{}]",
-                            tree->Hash(),
+                            tree->FileRootHash(),
                             hash);
                 return false;
             }

src/buildtool/file_system/file_root.hpp

@@ -225,17 +225,15 @@ class FileRoot {
             if (std::holds_alternative<tree_t>(data_)) {
                 try {
                     auto const& data = std::get<tree_t>(data_);
-                    // check if tree is ignore_special
-                    if (data->RawHash().empty()) {
-                        return std::nullopt;
-                    }
-                    auto const& id = data->Hash();
-                    auto const& size = data->Size();
-                    if (size) {
-                        return ArtifactDescription{
-                            ArtifactDigest{id, *size, /*is_tree=*/true},
-                            ObjectType::Tree,
-                            repository};
+                    // only consider tree if we have it unmodified
+                    if (auto id = data->Hash()) {
+                        auto const& size = data->Size();
+                        if (size) {
+                            return ArtifactDescription{
+                                ArtifactDigest{*id, *size, /*is_tree=*/true},
+                                ObjectType::Tree,
+                                repository};
+                        }
                     }
                 } catch (...) {
                     return std::nullopt;
@@ -351,7 +349,8 @@ class FileRoot {
                 nlohmann::json j;
                 j.push_back(ignore_special_ ? kGitTreeIgnoreSpecialMarker
                                             : kGitTreeMarker);
-                j.push_back(std::get<git_root_t>(root_).tree->Hash());
+                // we need the root tree id, irrespective of ignore_special flag
+                j.push_back(std::get<git_root_t>(root_).tree->FileRootHash());
                 return j;
             }
             if (std::holds_alternative<absent_root_t>(root_)) {

src/buildtool/file_system/git_tree.cpp

@@ -88,12 +88,10 @@ auto GitTree::Read(gsl::not_null<GitCASPtr> const& cas,
                                               check_symlinks,
                                               /*is_hex_id=*/false,
                                               ignore_special)) {
-                // the raw_id value is NOT recomputed when ignore_special==true,
-                // so we set it to empty to signal that it should not be used!
-                return GitTree::FromEntries(cas,
-                                            std::move(*entries),
-                                            ignore_special ? "" : *raw_id,
-                                            ignore_special);
+                // NOTE: the raw_id value is NOT recomputed when
+                // ignore_special==true.
+                return GitTree::FromEntries(
+                    cas, std::move(*entries), *raw_id, ignore_special);
             }
         }
         else {
@@ -164,7 +162,6 @@ auto GitTreeEntry::Tree(bool ignore_special) const& noexcept
                                                   check_symlinks,
                                                   /*is_hex_id=*/false,
                                                   ignore_special)) {
-                    // the raw_id value is not used when ignore_special==true
                     return GitTree::FromEntries(
                         cas_, std::move(*entries), raw_id_, ignore_special);
                 }