Skip to content

Commit

Permalink
Do not force unresolved symlinks to be absolute
Browse files Browse the repository at this point in the history
The documentation of `ctx.actions.symlink(target_path = ...)` states
that the given path is used as the symlink target without any changes,
but in reality this path has so far always been converted into an
absolute path by prepending the exec root. This is changed by this
commit, which gets very close to the documented behavior by preserving
the target path as is except for the standard normalization applied by
`PathFragment`. Improving the situation even further would require
modifying or adding to Bazel's core file system API, which may be done
in a follow-up PR.

Since relative symlinks only resolve correctly at the location they were
originally created, they have to be handled specially when staged as
runfiles. This commit adds a new `declared_symlinks` parameter to the
rule context's `runfiles` method that takes in symlink artifacts
declared via `ctx.actions.declare_symlink`. These symlinks are staged at
their runfiles path directly, with no further processing of their target
and without any intermediate runfiles pointing back to the artifact's
location under the exec root. This has to main benefits:
* With local execution, symlinks are resolved within the runfiles tree,
  which is more hermetic than following the runfiles symlink back into
  the exec root and resolving the symlink artifact there.
* Actions can expect symlink artifacts to be stages as is without
  intermediate symlinks with local and sandboxed execution, both inside
  and outside the runfiles tree. This is important for packaging actions
  as well as rulesets sensitive to symlinks (e.g. rules_js).

As a side-effect of the switch to relative symlinks, this commit
resolves a non-hermeticity issue observed in
bazelbuild#10298 (comment)
Integration tests are added to verify that symlinks staged in the
sandbox are no longer resolved non-hermetically.

Fixes bazelbuild#14224
  • Loading branch information
fmeum committed Jul 26, 2022
1 parent 2f7d965 commit 3143575
Show file tree
Hide file tree
Showing 11 changed files with 371 additions and 19 deletions.
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ java_library(
"starlark/StarlarkRuleContext.java",
"starlark/StarlarkRuleTransitionProvider.java",
"starlark/StarlarkTransition.java",
"starlark/UnresolvedSymlinkAction.java",
"test/AnalysisTestActionBuilder.java",
"test/BaselineCoverageAction.java",
"test/CoverageCommon.java",
Expand Down
65 changes: 65 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,8 @@ public void repr(Printer printer) {
*/
private final NestedSet<Artifact> artifacts;

private final NestedSet<Artifact> symlinkArtifacts;

/**
* A map of symlinks that should be present in the runfiles directory. In general, the symlink can
* be determined from the artifact by using the output-dir-relative path, so this should only be
Expand Down Expand Up @@ -235,6 +237,7 @@ public enum ConflictPolicy {
private Runfiles(
PathFragment suffix,
NestedSet<Artifact> artifacts,
NestedSet<Artifact> symlinkArtifacts,
NestedSet<SymlinkEntry> symlinks,
NestedSet<SymlinkEntry> rootSymlinks,
NestedSet<Artifact> extraMiddlemen,
Expand All @@ -243,6 +246,7 @@ private Runfiles(
boolean legacyExternalRunfiles) {
this.suffix = suffix;
this.artifacts = Preconditions.checkNotNull(artifacts);
this.symlinkArtifacts = Preconditions.checkNotNull(symlinkArtifacts);
this.symlinks = Preconditions.checkNotNull(symlinks);
this.rootSymlinks = Preconditions.checkNotNull(rootSymlinks);
this.extraMiddlemen = Preconditions.checkNotNull(extraMiddlemen);
Expand Down Expand Up @@ -277,6 +281,15 @@ public NestedSet<Artifact> getArtifacts() {
return artifacts;
}

@Override
public Depset getSymlinkArtifactsForStarlark() {
return Depset.of(Artifact.TYPE, symlinkArtifacts);
}

public NestedSet<Artifact> getSymlinkArtifacts() {
return symlinkArtifacts;
}

/** Returns the symlinks. */
@Override
public Depset /*<SymlinkEntry>*/ getSymlinksForStarlark() {
Expand Down Expand Up @@ -395,8 +408,21 @@ public Map<PathFragment, Artifact> getRunfilesInputs(
Map<PathFragment, Artifact> manifest = getSymlinksAsMap(checker);
// Add artifacts (committed to inclusion on construction of runfiles).
for (Artifact artifact : artifacts.toList()) {
if (artifact.isSymlink()) {
eventHandler.handle(Event.of(EventKind.ERROR,
location,
String.format("symlink artifact unexpected in artifacts: %s", artifact)));
}
checker.put(manifest, artifact.getRunfilesPath(), artifact);
}
for (Artifact symlinkArtifact : symlinkArtifacts.toList()) {
if (!symlinkArtifact.isSymlink()) {
eventHandler.handle(Event.of(EventKind.ERROR, location,
String.format("non-symlink artifact unexpected in symlinkArtifacts: %s",
symlinkArtifact)));
}
checker.put(manifest, symlinkArtifact.getRunfilesPath(), symlinkArtifact);
}

manifest = filterListForObscuringSymlinks(eventHandler, location, manifest);

Expand Down Expand Up @@ -556,6 +582,7 @@ public NestedSet<Artifact> getAllArtifacts() {
NestedSetBuilder<Artifact> allArtifacts = NestedSetBuilder.stableOrder();
allArtifacts
.addTransitive(artifacts)
.addTransitive(symlinkArtifacts)
.addAll(Iterables.transform(symlinks.toList(), SymlinkEntry::getArtifact))
.addAll(Iterables.transform(rootSymlinks.toList(), SymlinkEntry::getArtifact));
return allArtifacts.build();
Expand All @@ -566,6 +593,7 @@ public NestedSet<Artifact> getAllArtifacts() {
*/
public boolean isEmpty() {
return artifacts.isEmpty()
&& symlinkArtifacts.isEmpty()
&& symlinks.isEmpty()
&& rootSymlinks.isEmpty()
&& extraMiddlemen.isEmpty();
Expand Down Expand Up @@ -677,6 +705,7 @@ public static final class Builder {
* entries with later ones, so we want a post-order iteration.
*/
private final NestedSetBuilder<Artifact> artifactsBuilder = NestedSetBuilder.compileOrder();
private final NestedSetBuilder<Artifact> symlinkArtifactsBuilder = NestedSetBuilder.compileOrder();

private final NestedSetBuilder<SymlinkEntry> symlinksBuilder = NestedSetBuilder.stableOrder();
private final NestedSetBuilder<SymlinkEntry> rootSymlinksBuilder =
Expand Down Expand Up @@ -736,6 +765,7 @@ public Runfiles build() {
return new Runfiles(
suffix,
artifactsBuilder.build(),
symlinkArtifactsBuilder.build(),
symlinksBuilder.build(),
rootSymlinksBuilder.build(),
extraMiddlemenBuilder.build(),
Expand All @@ -750,6 +780,8 @@ public Builder addArtifact(Artifact artifact) {
Preconditions.checkNotNull(artifact);
Preconditions.checkArgument(
!artifact.isMiddlemanArtifact(), "unexpected middleman artifact: %s", artifact);
Preconditions.checkArgument(
!artifact.isSymlink(), "unexpected symlink artifact: %s", artifact);
artifactsBuilder.add(artifact);
return this;
}
Expand Down Expand Up @@ -785,6 +817,26 @@ public Builder addTransitiveArtifactsWrappedInStableOrder(NestedSet<Artifact> ar
return this;
}

@CanIgnoreReturnValue
public Builder addSymlinkArtifact(Artifact symlinkArtifact) {
Preconditions.checkNotNull(symlinkArtifact);
Preconditions.checkArgument(
symlinkArtifact.isSymlink(), "expected symlink artifact, got: %s", symlinkArtifact);
symlinkArtifactsBuilder.add(symlinkArtifact);
return this;
}

/**
* Adds several symlink artifacts to the internal collection.
*/
@CanIgnoreReturnValue
public Builder addSymlinkArtifacts(Iterable<Artifact> symlinkArtifacts) {
for (Artifact artifact : symlinkArtifacts) {
addSymlinkArtifact(artifact);
}
return this;
}

/** Adds a symlink. */
@CanIgnoreReturnValue
public Builder addSymlink(PathFragment link, Artifact target) {
Expand Down Expand Up @@ -875,6 +927,7 @@ private Builder merge(Runfiles runfiles, boolean includeArtifacts) {
suffix.equals(runfiles.suffix), "%s != %s", suffix, runfiles.suffix);
if (includeArtifacts) {
artifactsBuilder.addTransitive(runfiles.getArtifacts());
symlinkArtifactsBuilder.addTransitive(runfiles.getSymlinkArtifacts());
}
symlinksBuilder.addTransitive(runfiles.getSymlinks());
rootSymlinksBuilder.addTransitive(runfiles.getRootSymlinks());
Expand Down Expand Up @@ -1143,11 +1196,18 @@ public void fingerprint(Fingerprint fp) {
fp.addPath(rootSymlink.getValue().getExecPath());
}

fp.addInt(artifacts.toList().size());
for (Artifact artifact : artifacts.toList()) {
fp.addPath(artifact.getRunfilesPath());
fp.addPath(artifact.getExecPath());
}

fp.addInt(symlinkArtifacts.toList().size());
for (Artifact symlinkArtifact : symlinkArtifacts.toList()) {
fp.addPath(symlinkArtifact.getRunfilesPath());
// Symlink targets are tracked as inputs of actions using this method.
}

for (String name : getEmptyFilenames().toList()) {
fp.addString(name);
}
Expand Down Expand Up @@ -1180,6 +1240,11 @@ public String describeFingerprint() {
"artifact: '%s' '%s'\n", artifact.getRunfilesPath(), artifact.getExecPath()));
}

for (Artifact symlinkArtifact : artifacts.toList()) {
sb.append(
String.format("symlinkArtifact: '%s'\n", symlinkArtifact.getRunfilesPath()));
}

for (String name : getEmptyFilenames().toList()) {
sb.append(String.format("emptyFilename: '%s'\n", name));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction;
import com.google.devtools.build.lib.analysis.actions.DeterministicWriter;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.util.Fingerprint;
Expand Down Expand Up @@ -129,7 +127,7 @@ public SourceManifestAction(
Artifact primaryOutput,
Runfiles runfiles,
boolean remotableSourceManifestActions) {
super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), primaryOutput, false);
super(owner, runfiles.getSymlinkArtifacts(), primaryOutput, false);
this.manifestWriter = manifestWriter;
this.runfiles = runfiles;
this.remotableSourceManifestActions = remotableSourceManifestActions;
Expand Down Expand Up @@ -227,7 +225,11 @@ public void writeEntry(Writer manifestWriter, PathFragment rootRelativePath, Art
// This trailing whitespace is REQUIRED to process the single entry line correctly.
manifestWriter.append(' ');
if (symlink != null) {
manifestWriter.append(symlink.getPath().getPathString());
if (symlink.isSymlink()) {
manifestWriter.append(symlink.getPath().readSymbolicLink().getPathString());
} else {
manifestWriter.append(symlink.getPath().getPathString());
}
}
manifestWriter.append('\n');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@
import java.io.IOException;
import javax.annotation.Nullable;

/** Action to create a symbolic link. */
/**
* Action to create a symlink to a known-to-exist target with alias semantics similar to a true copy
* of the input (if any).
*/
public final class SymlinkAction extends AbstractAction {
private static final String GUID = "7f4fab4d-d0a7-4f0f-8649-1d0337a21fee";

Expand Down Expand Up @@ -152,13 +155,6 @@ public static SymlinkAction toFileset(
owner, execPath, primaryInput, primaryOutput, progressMessage, TargetType.FILESET);
}

public static SymlinkAction createUnresolved(
ActionOwner owner, Artifact primaryOutput, PathFragment targetPath, String progressMessage) {
Preconditions.checkArgument(primaryOutput.isSymlink());
return new SymlinkAction(
owner, targetPath, null, primaryOutput, progressMessage, TargetType.OTHER);
}

/**
* Creates a new SymlinkAction instance, where an input artifact is not present. This is useful
* when dealing with special cases where input paths that are outside the exec root directory
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ public void symlink(
? (String) progressMessageUnchecked
: "Creating symlink " + outputArtifact.getExecPathString();

SymlinkAction action;
Action action;
if (targetFile != Starlark.NONE) {
Artifact inputArtifact = (Artifact) targetFile;
if (outputArtifact.isSymlink()) {
Expand Down Expand Up @@ -324,10 +324,10 @@ public void symlink(
}

action =
SymlinkAction.createUnresolved(
new UnresolvedSymlinkAction(
ruleContext.getActionOwner(),
outputArtifact,
PathFragment.create((String) targetPath),
(String) targetPath,
progressMessage);
}
registerAction(action);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -912,6 +912,7 @@ public String expandLocation(String input, Sequence<?> targets, StarlarkThread t
public Runfiles runfiles(
Sequence<?> files,
Object transitiveFiles,
Sequence<?> declaredSymlinks,
Boolean collectData,
Boolean collectDefault,
Object symlinks,
Expand All @@ -929,7 +930,15 @@ public Runfiles runfiles(
builder.addRunfiles(ruleContext, RunfilesProvider.DEFAULT_RUNFILES);
}
if (!files.isEmpty()) {
builder.addArtifacts(Sequence.cast(files, Artifact.class, "files"));
Sequence<Artifact> artifacts = Sequence.cast(files, Artifact.class, "files");
for (Artifact artifact : artifacts) {
if (artifact.isSymlink()) {
throw Starlark.errorf(
"declared symlink '%s' is invalid for files, add via declared_symlinks instead",
artifact);
}
}
builder.addArtifacts(artifacts);
}
if (transitiveFiles != Starlark.NONE) {
NestedSet<Artifact> transitiveArtifacts =
Expand All @@ -943,6 +952,22 @@ public Runfiles runfiles(
}
builder.addTransitiveArtifacts(transitiveArtifacts);
}
if (!declaredSymlinks.isEmpty()) {
if (!ruleContext.getConfiguration().allowUnresolvedSymlinks()) {
throw Starlark.errorf(
"declared_symlink is not allowed; "
+ "use the --experimental_allow_unresolved_symlinks command line option");
}
Sequence<Artifact> artifacts = Sequence.cast(declaredSymlinks, Artifact.class, "declared_symlinks");
for (Artifact artifact : artifacts) {
if (!artifact.isSymlink()) {
throw Starlark.errorf(
"file '%s' is invalid for declared_symlinks, add via files instead",
artifact);
}
}
builder.addSymlinkArtifacts(artifacts);
}
if (isDepset(symlinks)) {
builder.addSymlinks(((Depset) symlinks).getSet(SymlinkEntry.class));
} else if (isNonEmptyDict(symlinks)) {
Expand Down
Loading

0 comments on commit 3143575

Please sign in to comment.