Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adjust --top_level_targets_for_symlinks #18854

Closed
wants to merge 5 commits into from
Closed

Adjust --top_level_targets_for_symlinks #18854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8d0daaa
Adjust --top_level_targets_for_symlinks.
gregestren Jul 6, 2023
38a25d1
Fix message typo.
gregestren Jul 6, 2023
dbecfb5
Compare config output directories, not the configs themselves.
gregestren Jul 6, 2023
71a02bf
Remove merge markers
gregestren Jul 7, 2023
d1b66b0
Update tests.
gregestren Jul 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 34 additions & 11 deletions src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,16 +766,39 @@ private ImmutableList<ConvenienceSymlink> createConvenienceSymlinks(
Reporter reporter = env.getReporter();

// Gather configurations to consider.
Set<BuildConfigurationValue> targetConfigurations =
buildRequestOptions.useTopLevelTargetsForSymlinks() && !targetsToBuild.isEmpty()
? targetsToBuild.stream()
.map(ConfiguredTarget::getActual)
.map(ConfiguredTarget::getConfigurationKey)
.filter(Objects::nonNull)
.distinct()
.map((key) -> executor.getConfiguration(reporter, key))
.collect(toImmutableSet())
: ImmutableSet.of(configuration);
ImmutableSet<BuildConfigurationValue> 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<BuildConfigurationValue> 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we include this case?

Pro: the symlink has nothing useful
Con: this will trigger when $ bazel build //... doesn't include any tests, since --trim_test_configuration will remove test options from all targets. Users may be confused why there are no symlinks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate on the Con? Does --trim_test_configuration result in duplicate configs? If so, could we somehow recognize when a config differs from the top-level config only in test options and ignore those configs here to fix the Con?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a con in that it means bazel-bin will still be deleted more than users expect. If $ bazel build //:all expands to 10 binaries, where only 1 has a self-transition, bazel-bin will still be deleted. If $ bazel build //:all also includes a test, bazel-bin will show up.

Worse, the non-transitioning binaries would actually appear in bazel-bin if it wasn't deleted. Even though they lack TestOptions they still have the same bazel-out/foo-bar/bin path because that instance of config trimming doesn't affect paths.

I'd rather not get into explicitly examining the configuration diffs. Another approach is to not compare the configs but their output paths.

Latest commit implements that, and manual testing shows intuitive results (I tried every combo of test, non-transitioning non-test, transitioning non-test).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that change, comparing paths instead of configs makes sense as those are all we care about in the end.

// 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 =
Expand All @@ -787,7 +810,7 @@ private ImmutableList<ConvenienceSymlink> createConvenienceSymlinks(
env.getWorkspace(),
env.getDirectories(),
getReporter(),
targetConfigurations,
targetConfigs,
options -> getConfiguration(executor, reporter, options),
productName);
}
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/com/google/devtools/build/lib/buildtool/OutputDirectoryLinksUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,9 @@ static ImmutableList<ConvenienceSymlink> 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 "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that this is more detailed now, but I could see this message being intimidating for the end user who is usually not the author of the self transition. I have a hard time coming up with something simpler though that would still be precise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I hope this is at least a signal to ask for expert help with a clearly diagnosable message. But yes end-user friendly messages would always be great.

This is probably getting off-topic but I like the error pattern idea of a super-concise non-intimidating message with a link to a help page with more context. That page can contextualize as much as needed and distinguish user vs. expert content and not be so overwhelming at the CLI.

+ "requested targets' outputs. Those targets self-transition to multiple "
+ "distinct configurations",
Joiner.on(", ").join(ambiguousLinks))));
}
return convenienceSymlinksBuilder.build();
Expand Down
49 changes: 46 additions & 3 deletions src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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
Expand All @@ -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(
Expand Down