From 8216e7d62717e396857b2cc385090dab6268ad5c Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Wed, 14 Apr 2021 15:10:00 +0800 Subject: [PATCH] Put the hack into CppCompileAction and fix nits. --- .../build/lib/actions/ExecutionRequirements.java | 5 +++++ .../google/devtools/build/lib/actions/Spawns.java | 6 ++++++ .../lib/remote/RemoteActionContextProvider.java | 4 ++-- .../build/lib/remote/RemoteSpawnRunner.java | 15 +++++---------- .../lib/remote/common/RemotePathResolver.java | 6 +++--- .../remote/merkletree/DirectoryTreeBuilder.java | 7 ------- .../build/lib/rules/cpp/CppCompileAction.java | 13 +++++++++++++ .../build/lib/remote/GrpcCacheClientTest.java | 4 ++-- 8 files changed, 36 insertions(+), 24 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java index 7b1ac3cdfe9090..53b1ae1b5c0460 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java @@ -260,4 +260,9 @@ public enum WorkerProtocolFormat { * "requires-xcode-label:unstable" and "requires-xcode:1.0". */ public static final String REQUIRES_XCODE_LABEL = "requires-xcode-label"; + + /** + * Requires the execution service do NOT share caches across different workspace. + */ + public static final String DIFFERENTIATE_WORKSPACE_CACHE = "internal-differentiate-workspace-cache"; } diff --git a/src/main/java/com/google/devtools/build/lib/actions/Spawns.java b/src/main/java/com/google/devtools/build/lib/actions/Spawns.java index 06e4b613497bc7..abe4bf1fdc5d58 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Spawns.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Spawns.java @@ -75,6 +75,12 @@ public static boolean requiresNetwork(Spawn spawn, boolean defaultSandboxDisallo return defaultSandboxDisallowNetwork; } + /** Returns whether caches from other workspace can be used for this spawn. */ + public static boolean shouldDifferentiateWorkspaceCache(Spawn spawn) { + return spawn.getExecutionInfo() + .containsKey(ExecutionRequirements.DIFFERENTIATE_WORKSPACE_CACHE); + } + /** * Returns whether a Spawn claims to support being executed with the persistent worker strategy * according to its execution info tags. diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java index 6f3843e94279c0..2514e9e0966b18 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionContextProvider.java @@ -31,7 +31,7 @@ import com.google.devtools.build.lib.remote.common.RemoteExecutionClient; import com.google.devtools.build.lib.remote.common.RemotePathResolver; import com.google.devtools.build.lib.remote.common.RemotePathResolver.DefaultRemotePathResolver; -import com.google.devtools.build.lib.remote.common.RemotePathResolver.SiblingExternalLayoutResolver; +import com.google.devtools.build.lib.remote.common.RemotePathResolver.SiblingRepositoryLayoutResolver; import com.google.devtools.build.lib.remote.options.RemoteOptions; import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.runtime.CommandEnvironment; @@ -91,7 +91,7 @@ RemotePathResolver createRemotePathResolver() { RemotePathResolver remotePathResolver; if (buildLanguageOptions != null && buildLanguageOptions.experimentalSiblingRepositoryLayout) { RemoteOptions remoteOptions = checkNotNull(env.getOptions().getOptions(RemoteOptions.class)); - remotePathResolver = new SiblingExternalLayoutResolver(execRoot, remoteOptions.incompatibleRemoteOutputPathsRelativeToInputRoot); + remotePathResolver = new SiblingRepositoryLayoutResolver(execRoot, remoteOptions.incompatibleRemoteOutputPathsRelativeToInputRoot); } else { remotePathResolver = new DefaultRemotePathResolver(execRoot); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index c9fb821498950d..0a9374b586afa4 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -229,7 +229,8 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context) maybeWriteParamFilesLocally(spawn); // Get the remote platform properties. - Platform platform = PlatformUtils.getPlatformProto(spawn, remoteOptions); + Platform platform = PlatformUtils + .getPlatformProto(spawn, remoteOptions, getExtraPlatformProperties(spawn, execRoot)); Command command = buildCommand( @@ -698,16 +699,10 @@ private SpawnResult handleError( } static List getExtraPlatformProperties(Spawn spawn, Path execRoot) { - // Hack on Windows. The included headers dumped by cl.exe in stdout contain absolute paths. - // When compiling the file from different workspace, the shared cache will cause header - // dependency checking to fail. This was initially fixed by a hack (see - // https://github.com/bazelbuild/bazel/issues/9172 for more details), but is broken again due - // to cl/356735700. We include workspace name here so action results from different workspaces - // on Windows won't be shared. - boolean isWindows = System.getProperty("os.name").startsWith("Windows"); - if (isWindows) { + if (Spawns.shouldDifferentiateWorkspaceCache(spawn)) { return ImmutableList - .of(Property.newBuilder().setName("workspace").setValue(execRoot.getBaseName()).build()); + .of(Property.newBuilder().setName("bazel-workspace").setValue(execRoot.getBaseName()) + .build()); } return ImmutableList.of(); diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java index 466d603e36f180..65d2e1bc591d56 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java +++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java @@ -130,7 +130,7 @@ public Path resolveLocalPath(ActionInput actionInput) { } /** - * A {@link RemotePathResolver} used when {@code --experimental_sibling_external_layout} is set. + * A {@link RemotePathResolver} used when {@code --experimental_sibling_repository_layout} is set. * Use parent directory of {@code execRoot} and set {@code workingDirectory} to the base name of * {@code execRoot}. * @@ -138,12 +138,12 @@ public Path resolveLocalPath(ActionInput actionInput) { * {@code --incompatible_remote_output_paths_relative_to_input_root} is not set, otherwise, * relative to input root. */ - class SiblingExternalLayoutResolver implements RemotePathResolver { + class SiblingRepositoryLayoutResolver implements RemotePathResolver { private final Path execRoot; private final boolean incompatibleRemoteOutputPathsRelativeToInputRoot; - public SiblingExternalLayoutResolver(Path execRoot, + public SiblingRepositoryLayoutResolver(Path execRoot, boolean incompatibleRemoteOutputPathsRelativeToInputRoot) { this.execRoot = execRoot; this.incompatibleRemoteOutputPathsRelativeToInputRoot = incompatibleRemoteOutputPathsRelativeToInputRoot; diff --git a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java index 43b12b51ec420b..c290a534f8a37c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java @@ -62,13 +62,6 @@ static DirectoryTree fromActionInputs( throws IOException { Map tree = new HashMap<>(); int numFiles = buildFromActionInputs(inputs, metadataProvider, execRoot, digestUtil, tree); - // // Make sure working directory is exists - // PathFragment workingDirectory = PathFragment.create(execRoot.getBaseName()); - // if (!tree.containsKey(workingDirectory)) { - // DirectoryNode dir = new DirectoryNode(workingDirectory.toString()); - // tree.put(workingDirectory, dir); - // createParentDirectoriesIfNotExist(workingDirectory, dir, tree); - // } return new DirectoryTree(tree, numFiles); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index 8afcf9c0b33ae4..55fde109880651 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -1477,6 +1477,19 @@ protected Spawn createSpawn(Map clientEnv) throws ActionExecutio .build(); } + if (featureConfiguration.isEnabled(CppRuleClasses.PARSE_SHOWINCLUDES)) { + // Hack on Windows. The included headers dumped by cl.exe in stdout contain absolute paths. + // When compiling the file from different workspace, the shared cache will cause header + // dependency checking to fail. This was initially fixed by a hack (see + // https://github.com/bazelbuild/bazel/issues/9172 for more details), but is broken again due + // to cl/356735700. We require execution service to ignore caches from other workspace. + executionInfo = + ImmutableMap.builderWithExpectedSize(executionInfo.size() + 1) + .putAll(executionInfo) + .put(ExecutionRequirements.DIFFERENTIATE_WORKSPACE_CACHE, "") + .build(); + } + try { return new SimpleSpawn( this, diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java index 40ed1f4a251e36..ac42266c81a7f5 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcCacheClientTest.java @@ -65,7 +65,7 @@ import com.google.devtools.build.lib.remote.common.RemoteActionExecutionContext; import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey; import com.google.devtools.build.lib.remote.common.RemotePathResolver; -import com.google.devtools.build.lib.remote.common.RemotePathResolver.SiblingExternalLayoutResolver; +import com.google.devtools.build.lib.remote.common.RemotePathResolver.SiblingRepositoryLayoutResolver; import com.google.devtools.build.lib.remote.grpc.ChannelConnectionFactory; import com.google.devtools.build.lib.remote.merkletree.MerkleTree; import com.google.devtools.build.lib.remote.options.RemoteOptions; @@ -402,7 +402,7 @@ public void testDownloadAllResultsForSiblingLayoutAndRelativeToInputRoot() throw RemoteOptions remoteOptions = Options.getDefaults(RemoteOptions.class); GrpcCacheClient client = newClient(remoteOptions); RemoteCache remoteCache = new RemoteCache(client, remoteOptions, DIGEST_UTIL); - RemotePathResolver remotePathResolver = new SiblingExternalLayoutResolver(execRoot, true); + RemotePathResolver remotePathResolver = new SiblingRepositoryLayoutResolver(execRoot, true); Digest fooDigest = DIGEST_UTIL.computeAsUtf8("foo-contents"); Digest barDigest = DIGEST_UTIL.computeAsUtf8("bar-contents");