From 856662a87fc18e64fe5c94d9f359eb51f706cd65 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 10 Jul 2023 11:50:15 -0700 Subject: [PATCH 1/2] Adjust --top_level_targets_for_symlinks Adjust `--top_level_targets_for_symlinks`. - If all targets in `$ bazel build //...` have the top-level config, `bazel-bin`, etc. point to that config - If all targets in `$ bazel build //...` have the same transitioned config, `bazel-bin`, etc. point to that config - If targets in `$ bazel build //...` have mixed configs, `bazel-bin`, etc. are deleted - If all targets in `$ bazel build //...` have the top-level config, `bazel-bin`, etc. point to that config - If all targets in `$ bazel build //...` have the same transitioned config, `bazel-bin`, etc. point to that config - If targets in `$ bazel build //...` have mixed configs and at least one of them is the top-level config, `bazel-bin`, etc. point to the top-level config - If targets in `$ bazel build //...` have mixed configs and none are the top-level config, `bazel-bin`, etc. are deleted Fixes https://github.com/bazelbuild/bazel/issues/17081. Closes #18854. PiperOrigin-RevId: 546938509 Change-Id: I75adf0b8c2094522125c5e65d8c450eb2436d392 --- .../build/lib/buildtool/ExecutionTool.java | 46 ++++++++++++----- .../buildtool/OutputDirectoryLinksUtils.java | 4 +- .../lib/buildtool/ConvenienceSymlinkTest.java | 49 +++++++++++++++++-- 3 files changed, 83 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java index f50a8982fdde73..058938c9f0125e 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java @@ -734,17 +734,39 @@ private ImmutableList createConvenienceSymlinks( Reporter reporter = env.getReporter(); // Gather configurations to consider. - Set targetConfigurations = - buildRequestOptions.useTopLevelTargetsForSymlinks() - && !analysisResult.getTargetsToBuild().isEmpty() - ? analysisResult.getTargetsToBuild().stream() - .map(ConfiguredTarget::getActual) - .map(ConfiguredTarget::getConfigurationKey) - .filter(Objects::nonNull) - .distinct() - .map((key) -> executor.getConfiguration(reporter, key)) - .collect(toImmutableSet()) - : ImmutableSet.of(analysisResult.getConfigurationCollection().getTargetConfiguration()); + ImmutableSet targetConfigs; + if (buildRequestOptions.useTopLevelTargetsForSymlinks() && !targetsToBuild.isEmpty()) { + // Collect the configuration of each top-level requested target. These may be different than + // the build's top-level configuration because of self-transitions. + ImmutableSet requestedTargetConfigs = + targetsToBuild.stream() + .map(ConfiguredTarget::getActual) + .map(ConfiguredTarget::getConfigurationKey) + .filter(Objects::nonNull) + .distinct() + .map((key) -> executor.getConfiguration(reporter, key)) + .collect(toImmutableSet()); + if (requestedTargetConfigs.size() == 1) { + // All top-level targets have the same configuration, so use that one. + targetConfigs = requestedTargetConfigs; + } else if (requestedTargetConfigs.stream() + .anyMatch( + c -> c.getOutputDirectoryName().equals(configuration.getOutputDirectoryName()))) { + // Mixed configs but at least one matches the top-level config's output path (this doesn't + // mean it's the same as the top-level config: --trim_test_configuration means non-test + // targets use the default output path but lack the top-level config's TestOptions). Set + // symlinks to the top-level config so at least non-transitioned targets resolve. See + // https://github.com/bazelbuild/bazel/issues/17081. + targetConfigs = ImmutableSet.of(configuration); + } else { + // Mixed configs, none of which include the top-level config. Delete the symlinks because + // they won't contain any relevant data. This is handled in the + // createOutputDirectorySymlinks call below. + targetConfigs = requestedTargetConfigs; + } + } else { + targetConfigs = ImmutableSet.of(configuration); + } String productName = runtime.getProductName(); try (SilentCloseable c = @@ -756,7 +778,7 @@ private ImmutableList createConvenienceSymlinks( env.getWorkspace(), env.getDirectories(), getReporter(), - targetConfigurations, + targetConfigs, options -> getConfiguration(executor, reporter, options), productName); } diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/OutputDirectoryLinksUtils.java b/src/main/java/com/google/devtools/build/lib/buildtool/OutputDirectoryLinksUtils.java index ac93bf2059f9a8..ad1fd849942232 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/OutputDirectoryLinksUtils.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/OutputDirectoryLinksUtils.java @@ -169,7 +169,9 @@ static ImmutableList createOutputDirectoryLinks( eventHandler.handle( Event.warn( String.format( - "cleared convenience symlink(s) %s because their destinations would be ambiguous", + "cleared convenience symlink(s) %s because they wouldn't contain " + + "requested targets' outputs. Those targets self-transition to multiple " + + "distinct configurations", Joiner.on(", ").join(ambiguousLinks)))); } return convenienceSymlinksBuilder.build(); diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java index 0f73d29ab80f9a..3090658802b5cb 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java @@ -417,7 +417,8 @@ public void buildingOnlyTargetsWithNullConfigurations_unsetsSymlinks() throws Ex } @Test - public void buildingTargetsWithDifferentOutputDirectories_unsetsSymlinks() throws Exception { + public void buildingTargetsWithDifferentOutputDirectories_unsetsSymlinksIfNoneAreTopLevel() + throws Exception { addOptions("--symlink_prefix=ambiguous-", "--incompatible_skip_genfiles_symlink=false"); Path config = getOutputPath().getRelative("some-imaginary-config"); @@ -431,10 +432,9 @@ public void buildingTargetsWithDifferentOutputDirectories_unsetsSymlinks() throw write( "targets/BUILD", - "basic_rule(name='default')", "incoming_transition_rule(name='config1', path='set_from_config1')", "incoming_transition_rule(name='config2', path='set_from_config2')"); - buildTarget("//targets:default", "//targets:config1", "//targets:config2"); + buildTarget("//targets:config1", "//targets:config2"); // there should be nothing at any of the convenience symlinks which depend on configuration - // the symlinks put there during the simulated prior build should have been deleted @@ -454,6 +454,49 @@ public void buildingTargetsWithDifferentOutputDirectories_unsetsSymlinks() throw getOutputPath()); } + @Test + public void buildingTargetsWithDifferentOutputDirectories_setsSymlinksIfAnyAreTopLevel() + throws Exception { + addOptions( + "--symlink_prefix=ambiguous-", + "--incompatible_skip_genfiles_symlink=false", + "--incompatible_merge_genfiles_directory=false", + "--incompatible_skip_genfiles_symlink=false"); + + Path config = getOutputPath().getRelative("some-imaginary-config"); + // put symlinks at the convenience symlinks spots to simulate a prior build + Path binLink = getWorkspace().getChild("ambiguous-bin"); + binLink.createSymbolicLink(config.getChild("bin")); + Path genfilesLink = getWorkspace().getChild("ambiguous-genfiles"); + genfilesLink.createSymbolicLink(config.getChild("genfiles")); + Path testlogsLink = getWorkspace().getChild("ambiguous-testlogs"); + testlogsLink.createSymbolicLink(config.getChild("testlogs")); + + write( + "targets/BUILD", + "basic_rule(name='default')", + "incoming_transition_rule(name='config1', path='set_from_config1')"); + buildTarget("//targets:default", "//targets:config1"); + + assertThat(getConvenienceSymlinks()) + .containsExactly( + "ambiguous-bin", + getOutputPath() + .getRelative("default-" + getTargetConfiguration().getCpu() + "-fastbuild/bin"), + "ambiguous-genfiles", + getOutputPath() + .getRelative( + "default-" + getTargetConfiguration().getCpu() + "-fastbuild/genfiles"), + "ambiguous-testlogs", + getOutputPath() + .getRelative( + "default-" + getTargetConfiguration().getCpu() + "-fastbuild/testlogs"), + "ambiguous-" + TestConstants.WORKSPACE_NAME, + getExecRoot(), + "ambiguous-out", + getOutputPath()); + } + @Test public void buildingTargetsWithSameConfiguration_setsSymlinks() throws Exception { addOptions( From 069a50cf9e05da1d0c88a78bfd91a851e1999db1 Mon Sep 17 00:00:00 2001 From: Greg Estren Date: Wed, 12 Jul 2023 13:42:32 -0400 Subject: [PATCH 2/2] Merge with 6.3.0 baseline. Particularly manually merge relevant signature changes from https://github.com/bazelbuild/bazel/commit/1cd35886cf2233050e2ed14fb3c98e5ec1d756b5. Thsi is different than https://github.com/bazelbuild/bazel/commit/1cd35886cf2233050e2ed14fb3c98e5ec1d756b5 itself because that also uses a baseline too new for 6.3.0. --- .../devtools/build/lib/buildtool/ExecutionTool.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java index 058938c9f0125e..4ba1473c618ef7 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java @@ -707,7 +707,10 @@ public void handleConvenienceSymlinks(AnalysisResult analysisResult) { ImmutableList convenienceSymlinks = ImmutableList.of(); if (request.getBuildOptions().experimentalConvenienceSymlinks != ConvenienceSymlinksMode.IGNORE) { - convenienceSymlinks = createConvenienceSymlinks(request.getBuildOptions(), analysisResult); + convenienceSymlinks = createConvenienceSymlinks( + request.getBuildOptions(), + analysisResult.getTargetsToBuild(), + analysisResult.getConfigurationCollection().getTargetConfiguration()); } if (request.getBuildOptions().experimentalConvenienceSymlinksBepEvent) { env.getEventBus().post(new ConvenienceSymlinksIdentifiedEvent(convenienceSymlinks)); @@ -729,7 +732,9 @@ public void handleConvenienceSymlinks(AnalysisResult analysisResult) { * in fact gets removed if it was already present from a previous invocation. */ private ImmutableList createConvenienceSymlinks( - BuildRequestOptions buildRequestOptions, AnalysisResult analysisResult) { + BuildRequestOptions buildRequestOptions, + ImmutableSet targetsToBuild, + BuildConfigurationValue configuration) { SkyframeExecutor executor = env.getSkyframeExecutor(); Reporter reporter = env.getReporter();