From d3d86440e90a8cef8483dec83f06c8a6f644603e Mon Sep 17 00:00:00 2001 From: felly Date: Fri, 8 Jun 2018 18:29:43 -0700 Subject: Unify path resolution codepaths. RELNOTES: None PiperOrigin-RevId: 199880252 --- .../build/lib/actions/ActionExecutionContext.java | 32 +++++--- .../build/lib/actions/ArtifactPathResolver.java | 88 ++++++++++++++++++++++ .../build/lib/analysis/test/TestResult.java | 3 +- .../build/lib/analysis/test/TestRunnerAction.java | 18 ++--- .../build/lib/exec/StandaloneTestStrategy.java | 5 +- .../lib/skyframe/ActionExecutionFunction.java | 17 +---- .../build/lib/skyframe/ActionMetadataHandler.java | 1 + .../build/lib/skyframe/ArtifactPathResolver.java | 38 ---------- 8 files changed, 124 insertions(+), 78 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/actions/ArtifactPathResolver.java delete mode 100644 src/main/java/com/google/devtools/build/lib/skyframe/ArtifactPathResolver.java (limited to 'src/main/java/com/google/devtools') diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java index 1db7c23e43..2aff66330f 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java @@ -59,6 +59,8 @@ public class ActionExecutionContext implements Closeable { @Nullable private ImmutableList outputSymlinks; + private final ArtifactPathResolver pathResolver; + private ActionExecutionContext( Executor executor, MetadataProvider actionInputFileCache, @@ -82,6 +84,9 @@ public class ActionExecutionContext implements Closeable { this.artifactExpander = artifactExpander; this.env = env; this.actionFileSystem = actionFileSystem; + this.pathResolver = createPathResolver(actionFileSystem, + // executor is only ever null in testing. + executor == null ? null : executor.getExecRoot()); } public ActionExecutionContext( @@ -168,22 +173,25 @@ public class ActionExecutionContext implements Closeable { * {@link Artifact.getRoot}. */ public Path getInputPath(ActionInput input) { - if (input instanceof Artifact) { - Artifact artifact = (Artifact) input; - if (actionFileSystem != null) { - return actionFileSystem.getPath(artifact.getPath().getPathString()); - } - return artifact.getPath(); - } - return getExecRoot().getRelative(input.getExecPath()); + return pathResolver.toPath(input); } public Root getRoot(Artifact artifact) { - if (actionFileSystem != null) { - return Root.fromPath( - actionFileSystem.getPath(artifact.getRoot().getRoot().asPath().getPathString())); + return pathResolver.transformRoot(artifact.getRoot().getRoot()); + } + + private static ArtifactPathResolver createPathResolver(FileSystem actionFileSystem, + Path execRoot) { + if (actionFileSystem == null) { + return ArtifactPathResolver.forExecRoot(execRoot); + } else { + return ArtifactPathResolver.withTransformedFileSystem( + actionFileSystem.getPath(execRoot.asFragment())); } - return artifact.getRoot().getRoot(); + } + + public ArtifactPathResolver getPathResolver() { + return pathResolver; } /** diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactPathResolver.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactPathResolver.java new file mode 100644 index 0000000000..7c28d1e72d --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactPathResolver.java @@ -0,0 +1,88 @@ +// Copyright 2018 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.actions; + +import com.google.common.base.Preconditions; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.Root; + +/** + * An indirection layer on Path resolution of {@link Artifact} and {@link Root}. + */ +public interface ArtifactPathResolver { + Path toPath(ActionInput actionInput); + Root transformRoot(Root root); + + ArtifactPathResolver IDENTITY = new IdentityResolver(null); + + static ArtifactPathResolver forExecRoot(Path execRoot) { + return new IdentityResolver(execRoot); + } + + static ArtifactPathResolver withTransformedFileSystem(Path execRoot) { + return new TransformResolver(execRoot); + } + + /** + * Path resolution that uses an Artifact's path directly, or looks up the input execPath relative + * to the given execRoot. + */ + class IdentityResolver implements ArtifactPathResolver { + private final Path execRoot; + + private IdentityResolver(Path execRoot) { + this.execRoot = execRoot; + } + + @Override + public Path toPath(ActionInput actionInput) { + if (actionInput instanceof Artifact) { + return ((Artifact) actionInput).getPath(); + } + return execRoot.getRelative(actionInput.getExecPath()); + } + + @Override + public Root transformRoot(Root root) { + return Preconditions.checkNotNull(root); + } + }; + + /** + * A resolver that transforms all results to the same filesystem as the given execRoot. + */ + class TransformResolver implements ArtifactPathResolver { + private final FileSystem fileSystem; + private final Path execRoot; + + private TransformResolver(Path execRoot) { + this.execRoot = execRoot; + this.fileSystem = Preconditions.checkNotNull(execRoot.getFileSystem()); + } + + @Override + public Path toPath(ActionInput input) { + if (input instanceof Artifact) { + return fileSystem.getPath(((Artifact) input).getPath().getPathString()); + } + return execRoot.getRelative(input.getExecPath()); + } + + @Override + public Root transformRoot(Root root) { + return Root.toFileSystem(Preconditions.checkNotNull(root), fileSystem); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java index 1a386ab847..c6a46e0b4e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestResult.java @@ -17,6 +17,7 @@ package com.google.devtools.build.lib.analysis.test; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; @@ -167,6 +168,6 @@ public class TestResult { */ private Collection> getFiles() { // TODO(ulfjack): Cache the set of generated files in the TestResultData. - return testAction.getTestOutputsMapping(null, execRoot); + return testAction.getTestOutputsMapping(ArtifactPathResolver.forExecRoot(execRoot), execRoot); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java index f03dcaff33..3548b9aafc 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java @@ -29,6 +29,7 @@ import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.ActionResult; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit; @@ -240,14 +241,14 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa */ // TODO(ulfjack): Instead of going to local disk here, use SpawnResult (add list of files there). public ImmutableList> getTestOutputsMapping( - @Nullable ActionExecutionContext context, Path execRoot) { + ArtifactPathResolver resolver, Path execRoot) { ImmutableList.Builder> builder = ImmutableList.builder(); - if (convertPath(context, getTestLog()).exists()) { - builder.add(Pair.of(TestFileNameConstants.TEST_LOG, convertPath(context, getTestLog()))); + if (resolver.toPath(getTestLog()).exists()) { + builder.add(Pair.of(TestFileNameConstants.TEST_LOG, resolver.toPath(getTestLog()))); } - if (getCoverageData() != null && convertPath(context, getCoverageData()).exists()) { + if (getCoverageData() != null && resolver.toPath(getCoverageData()).exists()) { builder.add(Pair.of(TestFileNameConstants.TEST_COVERAGE, - convertPath(context, getCoverageData()))); + resolver.toPath(getCoverageData()))); } if (execRoot != null) { ResolvedPaths resolvedPaths = resolve(execRoot); @@ -295,13 +296,6 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa return builder.build(); } - private static Path convertPath(@Nullable ActionExecutionContext actionContext, - Artifact artifact) { - return actionContext == null - ? artifact.getPath() - : actionContext.getInputPath(artifact); - } - @Override protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) throws CommandLineExpansionException { diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java index 5f714bd281..6c034d97b9 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java @@ -162,7 +162,7 @@ public class StandaloneTestStrategy extends TestStrategy { } processLastTestAttempt(attempt, dataBuilder, standaloneTestResult.testResultData()); ImmutableList> testOutputs = - action.getTestOutputsMapping(actionExecutionContext, execRoot); + action.getTestOutputsMapping(actionExecutionContext.getPathResolver(), execRoot); actionExecutionContext .getEventHandler() .post( @@ -211,7 +211,8 @@ public class StandaloneTestStrategy extends TestStrategy { // Get the normal test output paths, and then update them to use "attempt_N" names, and // attemptDir, before adding them to the outputs. ImmutableList> testOutputs = - action.getTestOutputsMapping(actionExecutionContext, actionExecutionContext.getExecRoot()); + action.getTestOutputsMapping(actionExecutionContext.getPathResolver(), + actionExecutionContext.getExecRoot()); for (Pair testOutput : testOutputs) { // e.g. /testRoot/test.dir/file, an example we follow throughout this loop's comments. Path testOutputPath = testOutput.getSecond(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index 2ffe103975..63ddb692d6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -31,6 +31,7 @@ import com.google.devtools.build.lib.actions.ActionLookupValue; import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey; import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FilesetOutputSymlink; import com.google.devtools.build.lib.actions.MissingInputFileException; @@ -46,7 +47,6 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.rules.cpp.IncludeScannable; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; -import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.skyframe.SkyFunction; @@ -550,19 +550,10 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver private ArtifactPathResolver pathResolver(ActionFileSystem actionFileSystem) { if (actionFileSystem != null) { - return new ArtifactPathResolver() { - @Override - public Path toPath(Artifact artifact) { - return actionFileSystem.getPath(artifact.getPath().getPathString()); - } - - @Override - public Root transformRoot(Root root) { - return Root.toFileSystem(root, actionFileSystem); - } - }; + return ArtifactPathResolver.withTransformedFileSystem( + actionFileSystem.getPath(skyframeActionExecutor.getExecRoot().asFragment())); } - return ArtifactPathResolver.IDENTITY; + return ArtifactPathResolver.forExecRoot(skyframeActionExecutor.getExecRoot()); } private static final Function TO_NONMANDATORY_SKYKEY = diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index af2a14347b..1d4012c60e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -26,6 +26,7 @@ import com.google.devtools.build.lib.actions.ActionInputMap; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; +import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileValue; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactPathResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactPathResolver.java deleted file mode 100644 index 9d0bf7651b..0000000000 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactPathResolver.java +++ /dev/null @@ -1,38 +0,0 @@ -// Copyright 2018 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.skyframe; - -import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.vfs.Path; -import com.google.devtools.build.lib.vfs.Root; - -/** - * An indirection layer on Path resolution of {@link Artifact} and {@link Root}. - */ -interface ArtifactPathResolver { - Path toPath(Artifact artifact); - Root transformRoot(Root root); - - ArtifactPathResolver IDENTITY = new ArtifactPathResolver() { - @Override - public Path toPath(Artifact artifact) { - return artifact.getPath(); - } - - @Override - public Root transformRoot(Root root) { - return root; - } - }; -} -- cgit v1.2.3