From 69d4951ab5dea551c7b2fff2f78767d2c42b174d Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 25 Jun 2024 20:02:42 +0200 Subject: [PATCH 01/25] Add path mapping support for C++ compile action templates --- .../lib/rules/cpp/CcToolchainVariables.java | 7 +++ .../rules/cpp/CppCompileActionTemplate.java | 24 ++++------ src/test/shell/bazel/path_mapping_test.sh | 48 ++++++++++++++++++- .../build/remote/worker/ExecutionServer.java | 6 +-- 4 files changed, 64 insertions(+), 21 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java index 12c4d9085e9b93..3eb08a3f0cbda4 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainVariables.java @@ -1404,6 +1404,13 @@ public Builder addArtifactVariable(String name, Artifact value) { return this; } + @CanIgnoreReturnValue + public Builder overrideArtifactVariable(String name, Artifact value) { + Preconditions.checkNotNull(value, "Cannot set null as a value for variable '%s'", name); + variablesMap.put(name, value); + return this; + } + /** * Add an artifact or string variable that expands {@code name} to {@code value}. * diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java index 67cb06bb597d19..518d9d17e94b47 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java @@ -245,27 +245,23 @@ private CppCompileAction createAction( CcToolchainVariables.Builder buildVariables = CcToolchainVariables.builder(cppCompileActionBuilder.getVariables()); - buildVariables.overrideStringVariable( - CompileBuildVariables.SOURCE_FILE.getVariableName(), - sourceTreeFileArtifact.getExecPathString()); - buildVariables.overrideStringVariable( - CompileBuildVariables.OUTPUT_FILE.getVariableName(), - outputTreeFileArtifact.getExecPathString()); + buildVariables.overrideArtifactVariable( + CompileBuildVariables.SOURCE_FILE.getVariableName(), sourceTreeFileArtifact); + buildVariables.overrideArtifactVariable( + CompileBuildVariables.OUTPUT_FILE.getVariableName(), outputTreeFileArtifact); if (dotdFileArtifact != null) { - buildVariables.overrideStringVariable( - CompileBuildVariables.DEPENDENCY_FILE.getVariableName(), - dotdFileArtifact.getExecPathString()); + buildVariables.overrideArtifactVariable( + CompileBuildVariables.DEPENDENCY_FILE.getVariableName(), dotdFileArtifact); } if (diagnosticsFileArtifact != null) { - buildVariables.overrideStringVariable( + buildVariables.overrideArtifactVariable( CompileBuildVariables.SERIALIZED_DIAGNOSTICS_FILE.getVariableName(), - diagnosticsFileArtifact.getExecPathString()); + diagnosticsFileArtifact); } if (ltoIndexFileArtifact != null) { - buildVariables.overrideStringVariable( - CompileBuildVariables.LTO_INDEXING_BITCODE_FILE.getVariableName(), - ltoIndexFileArtifact.getExecPathString()); + buildVariables.overrideArtifactVariable( + CompileBuildVariables.LTO_INDEXING_BITCODE_FILE.getVariableName(), ltoIndexFileArtifact); } builder.setVariables(buildVariables.build()); diff --git a/src/test/shell/bazel/path_mapping_test.sh b/src/test/shell/bazel/path_mapping_test.sh index 560625b8993106..7b8d8623d6a240 100755 --- a/src/test/shell/bazel/path_mapping_test.sh +++ b/src/test/shell/bazel/path_mapping_test.sh @@ -332,17 +332,25 @@ function test_path_stripping_cc_remote() { mkdir -p "$pkg" cat > "$pkg/BUILD" < "$pkg/main.cc" < +#include #include "$pkg/lib1/lib1.h" #include "lib2.h" +std::string TreeArtifactGreeting(); + int main() { std::cout << GetLib1Greeting() << std::endl; std::cout << GetLib2Greeting() << std::endl; + std::cout << TreeArtifactGreeting() << std::endl; return 0; } EOF @@ -510,6 +522,34 @@ transition_wrapper = rule( }, executable = True, ) + +def _gen_cc_impl(ctx): + out = ctx.actions.declare_directory(ctx.label.name) + ctx.actions.run_shell( + outputs = [out], + command = """\ +cat >{out_path}/gen.cc < + +std::string TreeArtifactGreeting() {{ + return "Hello, {subject}!"; +}} +EOF2 + """.format( + out_path = out.path, + subject = ctx.attr.subject, + ), + ) + return [ + DefaultInfo(files = depset([out])), + ] + +gen_cc = rule( + implementation = _gen_cc_impl, + attrs = { + "subject": attr.string(), + }, +) EOF cat > "$pkg/common/utils/utils.cc.tpl" <<'EOF' #include "utils.h" @@ -525,10 +565,12 @@ EOF --modify_execution_info=CppCompile=+supports-path-mapping \ --remote_executor=grpc://localhost:${worker_port} \ --features=-module_maps \ + --features=-supports_start_end_lib \ "//$pkg:main" &>"$TEST_log" || fail "Expected success" expect_log 'Hello, lib1!' expect_log 'Hello, lib2!' + expect_log 'Hello, TreeArtifact!' expect_not_log 'remote cache hit' bazel run \ @@ -537,10 +579,12 @@ EOF --modify_execution_info=CppCompile=+supports-path-mapping \ --remote_executor=grpc://localhost:${worker_port} \ --features=-module_maps \ + --features=-supports_start_end_lib \ "//$pkg:transitioned_main" &>"$TEST_log" || fail "Expected success" expect_log 'Hi there, lib1!' expect_log 'Hi there, lib2!' + expect_log 'Hello, TreeArtifact!' # Compilation actions for lib1, lib2 and main should result in cache hits due # to path stripping, utils is legitimately different and should not. expect_log ' 3 remote cache hit' diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index cac99e3adc05f6..fdac75953fc327 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -217,11 +217,7 @@ public void execute(ExecuteRequest request, StreamObserver responseOb executorService.submit(() -> execute(context, request, opName)); operationsCache.put(opName, future); ((ServerCallStreamObserver) responseObserver) - .setOnCancelHandler( - () -> { - future.cancel(true); - operationsCache.remove(opName); - }); + .setOnCancelHandler(() -> operationsCache.remove(opName)); // Send the first operation. responseObserver.onNext(Operation.newBuilder().setName(opName).build()); // When the operation completes, send the result. From cc8afdb9c250ef34d9b2c0b8b9fadb27cf2a085c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 28 Jun 2024 17:43:38 +0000 Subject: [PATCH 02/25] Update path_mapping_test.sh --- src/test/shell/bazel/path_mapping_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/shell/bazel/path_mapping_test.sh b/src/test/shell/bazel/path_mapping_test.sh index 7b8d8623d6a240..a166db073224f8 100755 --- a/src/test/shell/bazel/path_mapping_test.sh +++ b/src/test/shell/bazel/path_mapping_test.sh @@ -587,7 +587,7 @@ EOF expect_log 'Hello, TreeArtifact!' # Compilation actions for lib1, lib2 and main should result in cache hits due # to path stripping, utils is legitimately different and should not. - expect_log ' 3 remote cache hit' + expect_log ' 4 remote cache hit' } run_suite "path mapping tests" From 6c69e05a2a7fc67f237eda95b898718d74201f02 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sat, 29 Jun 2024 10:02:09 +0000 Subject: [PATCH 03/25] TMP: Debug --- tools/cpp/unix_cc_configure.bzl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/cpp/unix_cc_configure.bzl b/tools/cpp/unix_cc_configure.bzl index 72910ec4d8d57a..29e744e8e13494 100644 --- a/tools/cpp/unix_cc_configure.bzl +++ b/tools/cpp/unix_cc_configure.bzl @@ -446,6 +446,9 @@ def configure_unix_toolchain(repository_ctx, cpu_value, overriden_tools): # TODO: It's unclear why these flags aren't added on macOS. if bin_search_flags and not darwin: force_linker_flags.extend(bin_search_flags) + print("force", force_linker_flags) + print("bin", bin_search_flags) + print("path", gold_or_lld_linker_path) use_libcpp = darwin or bsd is_as_needed_supported = _is_linker_option_supported( repository_ctx, From 87a79b8aadf3c070947c1d3603386e2ef6297667 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sat, 29 Jun 2024 12:20:36 +0000 Subject: [PATCH 04/25] Update path_mapping_test.sh --- src/test/shell/bazel/path_mapping_test.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/test/shell/bazel/path_mapping_test.sh b/src/test/shell/bazel/path_mapping_test.sh index a166db073224f8..aa50e341775ca3 100755 --- a/src/test/shell/bazel/path_mapping_test.sh +++ b/src/test/shell/bazel/path_mapping_test.sh @@ -330,6 +330,10 @@ EOF function test_path_stripping_cc_remote() { local -r pkg="${FUNCNAME[0]}" + cat > MODULE.bazel < "$pkg/BUILD" <"$TEST_log" || fail "Expected success" expect_log 'Hello, lib1!' @@ -579,7 +582,6 @@ EOF --modify_execution_info=CppCompile=+supports-path-mapping \ --remote_executor=grpc://localhost:${worker_port} \ --features=-module_maps \ - --features=-supports_start_end_lib \ "//$pkg:transitioned_main" &>"$TEST_log" || fail "Expected success" expect_log 'Hi there, lib1!' From a4a6a640bf6031d54220fc3f01630de952b51d05 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sat, 29 Jun 2024 12:23:29 +0000 Subject: [PATCH 05/25] Update BUILD --- src/test/shell/bazel/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD index fa60df0dd53ebf..dd136688b7eaca 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD @@ -1359,7 +1359,7 @@ sh_test( ":test-deps", "//src/tools/remote:worker", ], - tags = ["no_windows"], + tags = ["no_windows", "requires-network"], deps = [ "//src/test/shell/bazel/remote:remote_utils", "@bazel_tools//tools/bash/runfiles", From e654a0b2371a0197cd4cb3960988947a6c5439d6 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sat, 29 Jun 2024 15:30:20 +0000 Subject: [PATCH 06/25] Update unix_cc_configure.bzl --- tools/cpp/unix_cc_configure.bzl | 3 --- 1 file changed, 3 deletions(-) diff --git a/tools/cpp/unix_cc_configure.bzl b/tools/cpp/unix_cc_configure.bzl index 29e744e8e13494..72910ec4d8d57a 100644 --- a/tools/cpp/unix_cc_configure.bzl +++ b/tools/cpp/unix_cc_configure.bzl @@ -446,9 +446,6 @@ def configure_unix_toolchain(repository_ctx, cpu_value, overriden_tools): # TODO: It's unclear why these flags aren't added on macOS. if bin_search_flags and not darwin: force_linker_flags.extend(bin_search_flags) - print("force", force_linker_flags) - print("bin", bin_search_flags) - print("path", gold_or_lld_linker_path) use_libcpp = darwin or bsd is_as_needed_supported = _is_linker_option_supported( repository_ctx, From 342e68b5e8f1fda35890a6e797efe509c57168e2 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sat, 29 Jun 2024 22:13:58 +0000 Subject: [PATCH 07/25] Update XcodeLocalEnvProvider.java --- .../devtools/build/lib/exec/local/XcodeLocalEnvProvider.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java b/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java index 5903dabe09d583..05c466e1102084 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java +++ b/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java @@ -83,6 +83,7 @@ public ImmutableMap rewriteLocalEnv( newEnvBuilder.put("TMPDIR", p); if (!containsXcodeVersion && !containsAppleSdkPlatform) { + System.err.println("no extra env"); return newEnvBuilder.buildOrThrow(); } @@ -94,11 +95,13 @@ public ImmutableMap rewriteLocalEnv( String version = env.get(AppleConfiguration.XCODE_VERSION_ENV_NAME); developerDir = getDeveloperDir(binTools, DottedVersion.fromStringUnchecked(version)); newEnvBuilder.put("DEVELOPER_DIR", developerDir); + System.err.println("developer_dir: " + developerDir); } if (containsAppleSdkPlatform) { String appleSdkPlatform = env.get(AppleConfiguration.APPLE_SDK_PLATFORM_ENV_NAME); newEnvBuilder.put("SDKROOT", getSdkRoot(developerDir, appleSdkPlatform)); } + System.err.println(newEnvBuilder.buildOrThrow()); return newEnvBuilder.buildOrThrow(); } From 9ab156f068449ccb16314d854e1465718da8c993 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 30 Jun 2024 00:40:45 +0000 Subject: [PATCH 08/25] Update XcodeLocalEnvProvider.java --- .../devtools/build/lib/exec/local/XcodeLocalEnvProvider.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java b/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java index 05c466e1102084..0cb31a80ca5ec5 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java +++ b/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java @@ -65,6 +65,7 @@ public final class XcodeLocalEnvProvider implements LocalEnvProvider { public ImmutableMap rewriteLocalEnv( Map env, BinTools binTools, String fallbackTmpDir) throws IOException, InterruptedException { + System.err.println("env: " + env); boolean containsDeveloperDir = env.containsKey(AppleConfiguration.DEVELOPER_DIR_ENV_NAME); boolean containsXcodeVersion = env.containsKey(AppleConfiguration.XCODE_VERSION_ENV_NAME); boolean containsAppleSdkPlatform = From 722bf05da56f8a56fdd80b30102a51e8b14ea5f4 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 30 Jun 2024 08:34:44 +0000 Subject: [PATCH 09/25] Update path_mapping_test.sh --- src/test/shell/bazel/path_mapping_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/shell/bazel/path_mapping_test.sh b/src/test/shell/bazel/path_mapping_test.sh index aa50e341775ca3..fc8e6970f37296 100755 --- a/src/test/shell/bazel/path_mapping_test.sh +++ b/src/test/shell/bazel/path_mapping_test.sh @@ -569,7 +569,7 @@ EOF --modify_execution_info=CppCompile=+supports-path-mapping \ --remote_executor=grpc://localhost:${worker_port} \ --features=-module_maps \ - "//$pkg:main" &>"$TEST_log" || fail "Expected success" + "//$pkg:main" &>"$TEST_log" && fail "Expected success" expect_log 'Hello, lib1!' expect_log 'Hello, lib2!' From 7c21931d7a3f9a099788adb2d639b989a1abeff5 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 30 Jun 2024 08:35:12 +0000 Subject: [PATCH 10/25] Update path_mapping_test.sh --- src/test/shell/bazel/path_mapping_test.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/shell/bazel/path_mapping_test.sh b/src/test/shell/bazel/path_mapping_test.sh index fc8e6970f37296..30cbf8a23d7c38 100755 --- a/src/test/shell/bazel/path_mapping_test.sh +++ b/src/test/shell/bazel/path_mapping_test.sh @@ -569,7 +569,8 @@ EOF --modify_execution_info=CppCompile=+supports-path-mapping \ --remote_executor=grpc://localhost:${worker_port} \ --features=-module_maps \ - "//$pkg:main" &>"$TEST_log" && fail "Expected success" + "//$pkg:main" &>"$TEST_log" + fail "Expected success" expect_log 'Hello, lib1!' expect_log 'Hello, lib2!' From a5b1b6650272ef1ef04409a4eb19c1850409d765 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 30 Jun 2024 11:09:45 +0000 Subject: [PATCH 11/25] Update ExecutionServer.java --- .../devtools/build/remote/worker/ExecutionServer.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index fdac75953fc327..24f768bab1d2d5 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -217,7 +217,11 @@ public void execute(ExecuteRequest request, StreamObserver responseOb executorService.submit(() -> execute(context, request, opName)); operationsCache.put(opName, future); ((ServerCallStreamObserver) responseObserver) - .setOnCancelHandler(() -> operationsCache.remove(opName)); + .setOnCancelHandler( + () -> { + future.cancel(false); + operationsCache.remove(opName) + }); // Send the first operation. responseObserver.onNext(Operation.newBuilder().setName(opName).build()); // When the operation completes, send the result. From 119f0970148e77fd571d62c431ae893371dee814 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 30 Jun 2024 11:10:43 +0000 Subject: [PATCH 12/25] Update ExecutionServer.java From f54da8f948d8742440e4b77ae84b75ad79958c3b Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 30 Jun 2024 11:12:12 +0000 Subject: [PATCH 13/25] Update XcodeLocalEnvProvider.java --- .../devtools/build/lib/exec/local/XcodeLocalEnvProvider.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java b/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java index 0cb31a80ca5ec5..5903dabe09d583 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java +++ b/src/main/java/com/google/devtools/build/lib/exec/local/XcodeLocalEnvProvider.java @@ -65,7 +65,6 @@ public final class XcodeLocalEnvProvider implements LocalEnvProvider { public ImmutableMap rewriteLocalEnv( Map env, BinTools binTools, String fallbackTmpDir) throws IOException, InterruptedException { - System.err.println("env: " + env); boolean containsDeveloperDir = env.containsKey(AppleConfiguration.DEVELOPER_DIR_ENV_NAME); boolean containsXcodeVersion = env.containsKey(AppleConfiguration.XCODE_VERSION_ENV_NAME); boolean containsAppleSdkPlatform = @@ -84,7 +83,6 @@ public ImmutableMap rewriteLocalEnv( newEnvBuilder.put("TMPDIR", p); if (!containsXcodeVersion && !containsAppleSdkPlatform) { - System.err.println("no extra env"); return newEnvBuilder.buildOrThrow(); } @@ -96,13 +94,11 @@ public ImmutableMap rewriteLocalEnv( String version = env.get(AppleConfiguration.XCODE_VERSION_ENV_NAME); developerDir = getDeveloperDir(binTools, DottedVersion.fromStringUnchecked(version)); newEnvBuilder.put("DEVELOPER_DIR", developerDir); - System.err.println("developer_dir: " + developerDir); } if (containsAppleSdkPlatform) { String appleSdkPlatform = env.get(AppleConfiguration.APPLE_SDK_PLATFORM_ENV_NAME); newEnvBuilder.put("SDKROOT", getSdkRoot(developerDir, appleSdkPlatform)); } - System.err.println(newEnvBuilder.buildOrThrow()); return newEnvBuilder.buildOrThrow(); } From ba84a4d46c5f9ab32acfe5c9b93bfe8cc285437d Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 30 Jun 2024 11:12:44 +0000 Subject: [PATCH 14/25] Update ExecutionServer.java --- .../google/devtools/build/remote/worker/ExecutionServer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index 24f768bab1d2d5..8a16a4260ad8c6 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -220,7 +220,7 @@ public void execute(ExecuteRequest request, StreamObserver responseOb .setOnCancelHandler( () -> { future.cancel(false); - operationsCache.remove(opName) + operationsCache.remove(opName); }); // Send the first operation. responseObserver.onNext(Operation.newBuilder().setName(opName).build()); From 5c12364b0dae01db35ae7501b29fddd6cdc1d99a Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 30 Jun 2024 11:21:14 +0000 Subject: [PATCH 15/25] Update path_mapping_test.sh --- src/test/shell/bazel/path_mapping_test.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/test/shell/bazel/path_mapping_test.sh b/src/test/shell/bazel/path_mapping_test.sh index 30cbf8a23d7c38..aa50e341775ca3 100755 --- a/src/test/shell/bazel/path_mapping_test.sh +++ b/src/test/shell/bazel/path_mapping_test.sh @@ -569,8 +569,7 @@ EOF --modify_execution_info=CppCompile=+supports-path-mapping \ --remote_executor=grpc://localhost:${worker_port} \ --features=-module_maps \ - "//$pkg:main" &>"$TEST_log" - fail "Expected success" + "//$pkg:main" &>"$TEST_log" || fail "Expected success" expect_log 'Hello, lib1!' expect_log 'Hello, lib2!' From 0b687bda2b0fe4e9ce531e8787b7da7fb60da762 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 30 Jun 2024 11:22:05 +0000 Subject: [PATCH 16/25] Update BUILD --- src/test/shell/bazel/BUILD | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD index dd136688b7eaca..fd65d1c7d30d61 100644 --- a/src/test/shell/bazel/BUILD +++ b/src/test/shell/bazel/BUILD @@ -1359,7 +1359,10 @@ sh_test( ":test-deps", "//src/tools/remote:worker", ], - tags = ["no_windows", "requires-network"], + tags = [ + "no_windows", + "requires-network", # for Bzlmod + ], deps = [ "//src/test/shell/bazel/remote:remote_utils", "@bazel_tools//tools/bash/runfiles", From 0c392bb06bbb49e70399abb5ca6556510c7d39ab Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 30 Jun 2024 13:57:04 +0000 Subject: [PATCH 17/25] Update path_mapping_test.sh --- src/test/shell/bazel/path_mapping_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/shell/bazel/path_mapping_test.sh b/src/test/shell/bazel/path_mapping_test.sh index aa50e341775ca3..47cda239dfe5a1 100755 --- a/src/test/shell/bazel/path_mapping_test.sh +++ b/src/test/shell/bazel/path_mapping_test.sh @@ -568,7 +568,7 @@ EOF --experimental_output_paths=strip \ --modify_execution_info=CppCompile=+supports-path-mapping \ --remote_executor=grpc://localhost:${worker_port} \ - --features=-module_maps \ + --features=+module_maps \ "//$pkg:main" &>"$TEST_log" || fail "Expected success" expect_log 'Hello, lib1!' From ac4562fecc841f3f893e17f6771ba64850900226 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 30 Jun 2024 14:10:51 +0000 Subject: [PATCH 18/25] Update ExecutionServer.java --- .../devtools/build/remote/worker/ExecutionServer.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index 8a16a4260ad8c6..fdac75953fc327 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -217,11 +217,7 @@ public void execute(ExecuteRequest request, StreamObserver responseOb executorService.submit(() -> execute(context, request, opName)); operationsCache.put(opName, future); ((ServerCallStreamObserver) responseObserver) - .setOnCancelHandler( - () -> { - future.cancel(false); - operationsCache.remove(opName); - }); + .setOnCancelHandler(() -> operationsCache.remove(opName)); // Send the first operation. responseObserver.onNext(Operation.newBuilder().setName(opName).build()); // When the operation completes, send the result. From 9e237d2e0296901e3e6e519f84b66feba57d3a49 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 30 Jun 2024 14:21:06 +0000 Subject: [PATCH 19/25] Update path_mapping_test.sh --- src/test/shell/bazel/path_mapping_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/shell/bazel/path_mapping_test.sh b/src/test/shell/bazel/path_mapping_test.sh index 47cda239dfe5a1..aa50e341775ca3 100755 --- a/src/test/shell/bazel/path_mapping_test.sh +++ b/src/test/shell/bazel/path_mapping_test.sh @@ -568,7 +568,7 @@ EOF --experimental_output_paths=strip \ --modify_execution_info=CppCompile=+supports-path-mapping \ --remote_executor=grpc://localhost:${worker_port} \ - --features=+module_maps \ + --features=-module_maps \ "//$pkg:main" &>"$TEST_log" || fail "Expected success" expect_log 'Hello, lib1!' From 258427011a85ad02b296581dd33d049b6e0dd43c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 30 Jun 2024 22:46:45 +0200 Subject: [PATCH 20/25] WIP --- .../google/devtools/build/remote/worker/BUILD | 13 +++++++++ .../build/remote/worker/ExecutionServer.java | 28 +++++++++++++++++-- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/BUILD b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/BUILD index 870f6cf1d976ba..14d67f9f2fd7ab 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/BUILD +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/BUILD @@ -1,3 +1,4 @@ +load("@bazel_skylib//rules:copy_file.bzl", "copy_file") load("@rules_java//java:defs.bzl", "java_library") package( @@ -13,15 +14,26 @@ filegroup( visibility = ["//src:__subpackages__"], ) +copy_file( + name = "xcode_locator", + src = "//tools/osx:xcode-locator", + out = "xcode-locator", +) + java_library( name = "worker", srcs = glob(["*.java"]), + data = [ + ":xcode_locator", + ], resources = ["//src/main/tools:linux-sandbox"], visibility = ["//src/tools/remote:__subpackages__"], deps = [ "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", "//src/main/java/com/google/devtools/build/lib/events", + "//src/main/java/com/google/devtools/build/lib/exec:bin_tools", + "//src/main/java/com/google/devtools/build/lib/exec/local", "//src/main/java/com/google/devtools/build/lib/remote", "//src/main/java/com/google/devtools/build/lib/remote:store", "//src/main/java/com/google/devtools/build/lib/remote/common", @@ -46,6 +58,7 @@ java_library( "//third_party/grpc-java:grpc-jar", "//third_party/protobuf:protobuf_java", "//third_party/protobuf:protobuf_java_util", + "@bazel_tools//tools/java/runfiles", "@googleapis//:google_bytestream_bytestream_java_grpc", "@googleapis//:google_bytestream_bytestream_java_proto", "@googleapis//:google_longrunning_operations_java_proto", diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index fdac75953fc327..4f1794e9424893 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -29,6 +29,7 @@ import build.bazel.remote.execution.v2.RequestMetadata; import build.bazel.remote.execution.v2.WaitExecutionRequest; import com.google.common.base.Throwables; +import com.google.common.collect.ImmutableList; import com.google.common.flogger.GoogleLogger; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; @@ -36,6 +37,8 @@ import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.events.NullEventHandler; +import com.google.devtools.build.lib.exec.BinTools; +import com.google.devtools.build.lib.exec.local.LocalEnvProvider; import com.google.devtools.build.lib.remote.ExecutionStatusException; import com.google.devtools.build.lib.remote.UploadManifest; import com.google.devtools.build.lib.remote.common.CacheNotFoundException; @@ -50,6 +53,7 @@ import com.google.devtools.build.lib.shell.FutureCommandResult; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.runfiles.Runfiles; import com.google.longrunning.Operation; import com.google.protobuf.Any; import com.google.protobuf.ExtensionRegistry; @@ -102,6 +106,8 @@ final class ExecutionServer extends ExecutionImplBase { private final ConcurrentHashMap> operationsCache; private final ListeningExecutorService executorService; private final DigestUtil digestUtil; + private final LocalEnvProvider localEnvProvider; + private final BinTools binTools; public ExecutionServer( Path workPath, @@ -135,6 +141,21 @@ public ExecutionServer( // Allow the core threads to die. realExecutor.allowCoreThreadTimeOut(true); this.executorService = MoreExecutors.listeningDecorator(realExecutor); + this.localEnvProvider = LocalEnvProvider.forCurrentOs(System.getenv()); + String xcodeLocator; + try { + xcodeLocator = + Runfiles.preload() + .withSourceRepository("") + .rlocation( + "io_bazel/src/tools/remote/src/main/java/com/google/devtools/build/remote/xcode-locator"); + } catch (IOException e) { + throw new IllegalStateException(e); + } + this.binTools = + BinTools.forEmbeddedBin( + workPath.getFileSystem().getPath(xcodeLocator).getParentDirectory(), + ImmutableList.of("xcode-locator")); } @Override @@ -410,12 +431,13 @@ private static boolean wasTimeout(long timeoutMillis, long wallTimeMillis) { return timeoutMillis > 0 && wallTimeMillis > timeoutMillis; } - private static Map getEnvironmentVariables(Command command) { + private Map getEnvironmentVariables(Command command) + throws IOException, InterruptedException { HashMap result = new HashMap<>(); for (EnvironmentVariable v : command.getEnvironmentVariablesList()) { result.put(v.getName(), v.getValue()); } - return result; + return localEnvProvider.rewriteLocalEnv(result, binTools, "/tmp"); } // Gets the uid of the current user. If uid could not be successfully fetched (e.g., on other @@ -490,7 +512,7 @@ private static String platformAsString(@Nullable Platform platform) { // arguments. Otherwise, returns a Command that would run the specified command inside the // specified docker container. private com.google.devtools.build.lib.shell.Command getCommand(Command cmd, String pathString) - throws StatusException, InterruptedException { + throws StatusException, InterruptedException, IOException { Map environmentVariables = getEnvironmentVariables(cmd); // This allows Bazel's integration tests to test for the remote platform. environmentVariables.put("BAZEL_REMOTE_PLATFORM", platformAsString(cmd.getPlatform())); From 87cc723a409fd3710dffffb94a3479093e12ba65 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 30 Jun 2024 21:05:46 +0000 Subject: [PATCH 21/25] Update Runfiles.java --- tools/java/runfiles/Runfiles.java | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/java/runfiles/Runfiles.java b/tools/java/runfiles/Runfiles.java index 23d7f6ac54a4ad..432fab6fd817a2 100644 --- a/tools/java/runfiles/Runfiles.java +++ b/tools/java/runfiles/Runfiles.java @@ -495,6 +495,7 @@ private static Map loadRunfiles(String path) throws IOException int index = line.indexOf(' '); String runfile = (index == -1) ? line : line.substring(0, index); String realPath = (index == -1) ? line : line.substring(index + 1); + System.err.println(runfile + ": " + realPath); result.put(runfile, realPath); } } From a53a0855600ed887645a2ccf4474909bdb776b8d Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 1 Jul 2024 01:16:18 +0200 Subject: [PATCH 22/25] Attempt to fix tests --- .../build/lib/remote/util/IntegrationTestUtils.java | 8 +++++++- .../devtools/build/remote/worker/ExecutionServer.java | 2 +- tools/java/runfiles/Runfiles.java | 1 - 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/IntegrationTestUtils.java b/src/test/java/com/google/devtools/build/lib/remote/util/IntegrationTestUtils.java index 53b7a2fe46078e..8b5a30f166ad07 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/util/IntegrationTestUtils.java +++ b/src/test/java/com/google/devtools/build/lib/remote/util/IntegrationTestUtils.java @@ -18,6 +18,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.shell.Subprocess; import com.google.devtools.build.lib.shell.SubprocessBuilder; import com.google.devtools.build.lib.util.OS; @@ -130,9 +131,14 @@ private void start() throws IOException, InterruptedException { ensureMkdir(stdPath); ensureTouchFile(stdoutPath); ensureTouchFile(stderrPath); - String workerPath = Runfiles.create().rlocation(WORKER_PATH.getSafePathString()); + Runfiles runfiles = Runfiles.preload().withSourceRepository(""); + String workerPath = runfiles.rlocation(WORKER_PATH.getSafePathString()); + ImmutableMap.Builder env = ImmutableMap.builder(); + env.putAll(System.getenv()); + env.putAll(runfiles.getEnvVars()); process = new SubprocessBuilder() + .setEnv(env.buildKeepingLast()) .setStdout(new File(stdoutPath.getSafePathString())) .setStderr(new File(stderrPath.getSafePathString())) .setArgv( diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index 4f1794e9424893..0c870f9c1b383f 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -437,7 +437,7 @@ private Map getEnvironmentVariables(Command command) for (EnvironmentVariable v : command.getEnvironmentVariablesList()) { result.put(v.getName(), v.getValue()); } - return localEnvProvider.rewriteLocalEnv(result, binTools, "/tmp"); + return new HashMap<>(localEnvProvider.rewriteLocalEnv(result, binTools, "/tmp")); } // Gets the uid of the current user. If uid could not be successfully fetched (e.g., on other diff --git a/tools/java/runfiles/Runfiles.java b/tools/java/runfiles/Runfiles.java index 432fab6fd817a2..23d7f6ac54a4ad 100644 --- a/tools/java/runfiles/Runfiles.java +++ b/tools/java/runfiles/Runfiles.java @@ -495,7 +495,6 @@ private static Map loadRunfiles(String path) throws IOException int index = line.indexOf(' '); String runfile = (index == -1) ? line : line.substring(0, index); String realPath = (index == -1) ? line : line.substring(index + 1); - System.err.println(runfile + ": " + realPath); result.put(runfile, realPath); } } From 69e74d5bd54664c44753b3e56f5e9ebbcf1d649c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 1 Jul 2024 10:03:14 +0000 Subject: [PATCH 23/25] Update ExecutionServer.java --- .../google/devtools/build/remote/worker/ExecutionServer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index 0c870f9c1b383f..c1f863221a8804 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -148,7 +148,7 @@ public ExecutionServer( Runfiles.preload() .withSourceRepository("") .rlocation( - "io_bazel/src/tools/remote/src/main/java/com/google/devtools/build/remote/xcode-locator"); + "io_bazel/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/xcode-locator"); } catch (IOException e) { throw new IllegalStateException(e); } From d51954ce57418082a153f19e21b3b6c623d8389c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 1 Jul 2024 10:42:52 +0000 Subject: [PATCH 24/25] Update test_base.py --- src/test/py/bazel/test_base.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/test/py/bazel/test_base.py b/src/test/py/bazel/test_base.py index 40cbfaa46b1261..0ea1a0bda0ccd3 100644 --- a/src/test/py/bazel/test_base.py +++ b/src/test/py/bazel/test_base.py @@ -475,13 +475,6 @@ def StartRemoteWorker(self): port = s.getsockname()[1] s.close() - env_add = {} - try: - env_add['RUNFILES_MANIFEST_FILE'] = TestBase.GetEnv( - 'RUNFILES_MANIFEST_FILE') - except EnvVarUndefinedError: - pass - # Tip: To help debug remote build problems, add the --debug flag below. self._worker_proc = subprocess.Popen( [ @@ -496,7 +489,7 @@ def StartRemoteWorker(self): stdout=self._worker_stdout, stderr=self._worker_stderr, cwd=self._test_cwd, - env=self._EnvMap(env_add=env_add)) + env=self._EnvMap(env_add=self._runfiles.EnvVars())) return port From f6f0ac3eea6718e26cfc063b496e78e2e51f5d98 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 3 Jul 2024 22:06:10 +0000 Subject: [PATCH 25/25] Update ExecutionServer.java --- .../devtools/build/remote/worker/ExecutionServer.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java index c1f863221a8804..e47a28aa27c4b7 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java @@ -238,7 +238,11 @@ public void execute(ExecuteRequest request, StreamObserver responseOb executorService.submit(() -> execute(context, request, opName)); operationsCache.put(opName, future); ((ServerCallStreamObserver) responseObserver) - .setOnCancelHandler(() -> operationsCache.remove(opName)); + .setOnCancelHandler( + () -> { + future.cancel(true); + operationsCache.remove(opName); + }); // Send the first operation. responseObserver.onNext(Operation.newBuilder().setName(opName).build()); // When the operation completes, send the result.