Skip to content

Commit

Permalink
Properly account for symlinks and root symlinks in repo mapping manifest
Browse files Browse the repository at this point in the history
A runfiles symlink entry `path/to/pkg/file -> path/to/artifact` is implicitly prefixed with the workspace name and should thus result in repo mappings to the main repository being added to the repo mapping manifest, not mappings for the owner of the artifact.

A runfiles root symlink entry `first_segment/path/to/pkg/file -> path/to/artifact` should result result in repo mappings to the repository with canonical name equal to the first segment being added to the repo mapping manifest, if such a repository exists.

Along the way clarifies in the documentation of `ctx.runfiles(symlinks = ...)` that the workspace name will be prefixed implicitly.

Closes #18381.

PiperOrigin-RevId: 541934743
Change-Id: I2931772c3b337d76e65fd81e4a6b783ed80ac05b
  • Loading branch information
fmeum authored and copybara-github committed Jun 20, 2023
1 parent 11a9deb commit 73ed46c
Show file tree
Hide file tree
Showing 12 changed files with 256 additions and 88 deletions.
3 changes: 2 additions & 1 deletion site/en/extending/rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -884,7 +884,8 @@ need to be different for some reason, you can specify the `root_symlinks` or
`symlinks` arguments. The `root_symlinks` is a dictionary mapping paths to
files, where the paths are relative to the root of the runfiles directory. The
`symlinks` dictionary is the same, but paths are implicitly prefixed with the
name of the workspace.
name of the main workspace (*not* the name of the repository containing the
current target).

```python
...
Expand Down
14 changes: 14 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,7 @@ java_library(
":starlark/starlark_toolchain_context",
":starlark/starlark_transition",
":starlark/template_dict",
":symlink_entry",
":target_and_configuration",
":template_variable_info",
":test/analysis_failure",
Expand Down Expand Up @@ -1041,6 +1042,7 @@ java_library(
deps = [
":actions/abstract_file_write_action",
":actions/deterministic_writer",
":symlink_entry",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
Expand Down Expand Up @@ -1143,6 +1145,18 @@ java_library(
],
)

java_library(
name = "symlink_entry",
srcs = ["SymlinkEntry.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/net/starlark/java/eval",
"//third_party:guava",
],
)

java_library(
name = "target_and_configuration",
srcs = ["TargetAndConfiguration.java"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.analysis;

import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap;
import static java.nio.charset.StandardCharsets.ISO_8859_1;
import static java.util.Comparator.comparing;
Expand All @@ -30,6 +29,7 @@
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction;
import com.google.devtools.build.lib.analysis.actions.DeterministicWriter;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
Expand Down Expand Up @@ -63,19 +63,35 @@ public final class RepoMappingManifestAction extends AbstractFileWriteAction {
});
};

private static final MapFn<Artifact> OWNER_REPO_FN =
(artifact, args) -> {
args.accept(
artifact.getOwner() != null ? artifact.getOwner().getRepository().getName() : "");
};

private static final MapFn<SymlinkEntry> FIRST_SEGMENT_FN =
(symlink, args) -> args.accept(symlink.getPath().getSegment(0));

private final NestedSet<Package> transitivePackages;
private final NestedSet<Artifact> runfilesArtifacts;
private final boolean hasRunfilesSymlinks;
private final NestedSet<SymlinkEntry> runfilesRootSymlinks;
private final String workspaceName;

public RepoMappingManifestAction(
ActionOwner owner,
Artifact output,
NestedSet<Package> transitivePackages,
NestedSet<Artifact> runfilesArtifacts,
NestedSet<SymlinkEntry> runfilesSymlinks,
NestedSet<SymlinkEntry> runfilesRootSymlinks,
String workspaceName) {
super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output, /*makeExecutable=*/ false);
super(
owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output, /* makeExecutable= */ false);
this.transitivePackages = transitivePackages;
this.runfilesArtifacts = runfilesArtifacts;
this.hasRunfilesSymlinks = !runfilesSymlinks.isEmpty();
this.runfilesRootSymlinks = runfilesRootSymlinks;
this.workspaceName = workspaceName;
}

Expand All @@ -97,7 +113,9 @@ protected void computeKey(
throws CommandLineExpansionException, EvalException, InterruptedException {
fp.addUUID(MY_UUID);
actionKeyContext.addNestedSetToFingerprint(REPO_AND_MAPPING_DIGEST_FN, fp, transitivePackages);
actionKeyContext.addNestedSetToFingerprint(fp, runfilesArtifacts);
actionKeyContext.addNestedSetToFingerprint(OWNER_REPO_FN, fp, runfilesArtifacts);
fp.addBoolean(hasRunfilesSymlinks);
actionKeyContext.addNestedSetToFingerprint(FIRST_SEGMENT_FN, fp, runfilesRootSymlinks);
fp.addString(workspaceName);
}

Expand All @@ -107,11 +125,28 @@ public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx)
return out -> {
PrintWriter writer = new PrintWriter(out, /* autoFlush= */ false, ISO_8859_1);

ImmutableSet<RepositoryName> reposContributingRunfiles =
runfilesArtifacts.toList().stream()
.filter(a -> a.getOwner() != null)
.map(a -> a.getOwner().getRepository())
.collect(toImmutableSet());
var reposInRunfilePaths = ImmutableSet.<String>builder();

// The runfiles paths of symlinks are always prefixed with the main workspace name, *not* the
// name of the repository adding the symlink.
if (hasRunfilesSymlinks) {
reposInRunfilePaths.add(RepositoryName.MAIN.getName());
}

// Since root symlinks are the only way to stage a runfile at a specific path under the
// current repository's runfiles directory, recognize canonical repository names that appear
// as the first segment of their runfiles paths.
for (SymlinkEntry symlink : runfilesRootSymlinks.toList()) {
reposInRunfilePaths.add(symlink.getPath().getSegment(0));
}

for (Artifact artifact : runfilesArtifacts.toList()) {
Label owner = artifact.getOwner();
if (owner != null) {
reposInRunfilePaths.add(owner.getRepository().getName());
}
}

transitivePackages.toList().stream()
.collect(
toImmutableSortedMap(
Expand All @@ -123,14 +158,14 @@ public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx)
(first, second) -> first))
.forEach(
(repoName, mapping) ->
writeRepoMapping(writer, reposContributingRunfiles, repoName, mapping));
writeRepoMapping(writer, reposInRunfilePaths.build(), repoName, mapping));
writer.flush();
};
}

private void writeRepoMapping(
PrintWriter writer,
ImmutableSet<RepositoryName> reposContributingRunfiles,
ImmutableSet<String> reposInRunfilesPaths,
RepositoryName repoName,
RepositoryMapping repoMapping) {
for (Entry<String, RepositoryName> mappingEntry :
Expand All @@ -140,8 +175,8 @@ private void writeRepoMapping(
// Rlocation paths can't reference an empty apparent name anyway.
continue;
}
if (!reposContributingRunfiles.contains(mappingEntry.getValue())) {
// We only write entries for repos that actually contribute runfiles.
if (!reposInRunfilesPaths.contains(mappingEntry.getValue().getName())) {
// We only write entries for repos whose canonical names appear in runfiles paths.
continue;
}
// The canonical name of the main repo is the empty string, which is not a valid name for a
Expand Down
69 changes: 0 additions & 69 deletions src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
import com.google.devtools.build.lib.starlarkbuildapi.RunfilesApi;
import com.google.devtools.build.lib.starlarkbuildapi.SymlinkEntryApi;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
Expand All @@ -56,7 +55,6 @@
import java.util.UUID;
import javax.annotation.Nullable;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Printer;
import net.starlark.java.eval.Sequence;
import net.starlark.java.eval.Starlark;
import net.starlark.java.eval.StarlarkSemantics;
Expand Down Expand Up @@ -93,73 +91,6 @@ public void fingerprint(Fingerprint fp) {
@SerializationConstant @AutoCodec.VisibleForSerialization
static final EmptyFilesSupplier DUMMY_EMPTY_FILES_SUPPLIER = new DummyEmptyFilesSupplier();

/**
* An entry in the runfiles map.
*
* <p>build-runfiles.cc enforces the following constraints: The PathFragment must not be an
* absolute path, nor contain "..". Overlapping runfiles links are also refused. This is the case
* where you ask to create a link to "foo" and also "foo/bar.txt". I.e. you're asking it to make
* "foo" both a file (symlink) and a directory.
*
* <p>Links to directories are heavily discouraged.
*/
//
// O intrepid fixer or bugs and implementor of features, dare not to add a .equals() method
// to this class, lest you condemn yourself, or a fellow other developer to spending two
// delightful hours in a fancy hotel on a Chromebook that is utterly unsuitable for Java
// development to figure out what went wrong, just like I just did.
//
// The semantics of the symlinks nested set dictates that later entries overwrite earlier
// ones. However, the semantics of nested sets dictate that if there are duplicate entries, they
// are only returned once in the iterator.
//
// These two things, innocent when taken alone, result in the effect that when there are three
// entries for the same path, the first one and the last one the same, and the middle one
// different, the *middle* one will take effect: the middle one overrides the first one, and the
// first one prevents the last one from appearing on the iterator.
//
// The lack of a .equals() method prevents this by making the first entry in the above case not
// equal to the third one if they are not the same instance (which they almost never are).
//
// Goodnight, prince(ss)?, and sweet dreams.
public static final class SymlinkEntry implements SymlinkEntryApi {
private final PathFragment path;
private final Artifact artifact;

SymlinkEntry(PathFragment path, Artifact artifact) {
this.path = Preconditions.checkNotNull(path);
this.artifact = Preconditions.checkNotNull(artifact);
}

@Override
public String getPathString() {
return path.getPathString();
}

public PathFragment getPath() {
return path;
}

@Override
public Artifact getArtifact() {
return artifact;
}

@Override
public boolean isImmutable() {
return true;
}

@Override
public void repr(Printer printer) {
printer.append("SymlinkEntry(path = ");
printer.repr(getPathString());
printer.append(", target_file = ");
artifact.repr(printer);
printer.append(")");
}
}

// It is important to declare this *after* the DUMMY_SYMLINK_EXPANDER to avoid NPEs
public static final Runfiles EMPTY = new Builder().build();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,9 @@ private static Artifact createRepoMappingManifestAction(
ruleContext.getActionOwner(),
repoMappingManifest,
ruleContext.getTransitivePackagesForRunfileRepoMappingManifest(),
runfiles.getAllArtifacts(),
runfiles.getArtifacts(),
runfiles.getSymlinks(),
runfiles.getRootSymlinks(),
ruleContext.getWorkspaceName()));
return repoMappingManifest;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
// Copyright 2014 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package com.google.devtools.build.lib.analysis;

import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.starlarkbuildapi.SymlinkEntryApi;
import com.google.devtools.build.lib.vfs.PathFragment;
import net.starlark.java.eval.Printer;

/**
* An entry in the runfiles map.
*
* <p>build-runfiles.cc enforces the following constraints: The PathFragment must not be an absolute
* path, nor contain "..". Overlapping runfiles links are also refused. This is the case where you
* ask to create a link to "foo" and also "foo/bar.txt". I.e. you're asking it to make "foo" both a
* file (symlink) and a directory.
*
* <p>Links to directories are heavily discouraged.
*/
//
// O intrepid fixer or bugs and implementor of features, dare not to add a .equals() method
// to this class, lest you condemn yourself, or a fellow other developer to spending two
// delightful hours in a fancy hotel on a Chromebook that is utterly unsuitable for Java
// development to figure out what went wrong, just like I just did.
//
// The semantics of the symlinks nested set dictates that later entries overwrite earlier
// ones. However, the semantics of nested sets dictate that if there are duplicate entries, they
// are only returned once in the iterator.
//
// These two things, innocent when taken alone, result in the effect that when there are three
// entries for the same path, the first one and the last one the same, and the middle one
// different, the *middle* one will take effect: the middle one overrides the first one, and the
// first one prevents the last one from appearing on the iterator.
//
// The lack of a .equals() method prevents this by making the first entry in the above case not
// equal to the third one if they are not the same instance (which they almost never are).
//
// Goodnight, prince(ss)?, and sweet dreams.
public final class SymlinkEntry implements SymlinkEntryApi {
private final PathFragment path;
private final Artifact artifact;

SymlinkEntry(PathFragment path, Artifact artifact) {
this.path = Preconditions.checkNotNull(path);
this.artifact = Preconditions.checkNotNull(artifact);
}

@Override
public String getPathString() {
return path.getPathString();
}

public PathFragment getPath() {
return path;
}

@Override
public Artifact getArtifact() {
return artifact;
}

@Override
public boolean isImmutable() {
return true;
}

@Override
public void repr(Printer printer) {
printer.append("SymlinkEntry(path = ");
printer.repr(getPathString());
printer.append(", target_file = ");
artifact.repr(printer);
printer.append(")");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@
import com.google.devtools.build.lib.analysis.ResolvedToolchainContext;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.Runfiles;
import com.google.devtools.build.lib.analysis.Runfiles.SymlinkEntry;
import com.google.devtools.build.lib.analysis.RunfilesProvider;
import com.google.devtools.build.lib.analysis.ShToolchain;
import com.google.devtools.build.lib.analysis.SymlinkEntry;
import com.google.devtools.build.lib.analysis.ToolchainCollection;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,11 @@ public static <T> String describedNestedSetFingerprint(
return "<empty>";
}
StringBuilder sb = new StringBuilder();
sb.append("order: ").append(nestedSet.getOrder()).append('\n');
sb.append("order: ")
.append(nestedSet.getOrder())
.append(
" (fingerprinting considers internal"
+ " nested set structure, which is not reflected in values reported below)\n");
ImmutableList<T> list = nestedSet.toList();
sb.append("size: ").append(list.size()).append('\n');
for (T item : list) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,9 @@ public void repoMappingAction(
ruleContext.getActionOwner(),
repoMappingManifest,
ruleContext.getTransitivePackagesForRunfileRepoMappingManifest(),
runfiles.getAllArtifacts(),
runfiles.getArtifacts(),
runfiles.getSymlinks(),
runfiles.getRootSymlinks(),
ruleContext.getWorkspaceName()));
}

Expand Down
Loading

0 comments on commit 73ed46c

Please sign in to comment.