Skip to content

Commit

Permalink
Roll Forward: Remove on-demand runfile tree creation from test strategy
Browse files Browse the repository at this point in the history
RELNOTES: None

PiperOrigin-RevId: 266741971
  • Loading branch information
buchgr authored and copybara-github committed Sep 2, 2019
1 parent 1afb510 commit 580038e
Show file tree
Hide file tree
Showing 14 changed files with 341 additions and 148 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -469,4 +469,14 @@ private static CommandLine computeArgs(RuleContext ruleContext, CommandLine addi
return CommandLine.concat(
ruleContext.getExpander().withDataLocations().tokenized("args"), additionalArgs);
}

/** Returns the path of the input manifest of {@code runfilesDir}. */
public static Path inputManifestPath(Path runfilesDir) {
return FileSystemUtils.replaceExtension(runfilesDir, INPUT_MANIFEST_EXT);
}

/** Returns the path of the output manifest of {@code runfilesDir}. */
public static Path outputManifestPath(Path runfilesDir) {
return runfilesDir.getRelative(OUTPUT_MANIFEST_BASENAME);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
// Copyright 2019 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.exec;

import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.RunfilesSupplier;
import com.google.devtools.build.lib.analysis.RunfilesSupport;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.util.io.OutErr;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
import javax.annotation.concurrent.GuardedBy;
import javax.annotation.concurrent.ThreadSafe;

/**
* Utility used in local execution to create a runfiles tree if {@code --nobuild_runfile_links} has
* been specified.
*
* <p>It is safe to call {@link #updateRunfilesDirectory} concurrently.
*/
@ThreadSafe
public class RunfilesTreeUpdater {

public static final RunfilesTreeUpdater INSTANCE = new RunfilesTreeUpdater();

private final Object lock = new Object();

private static final class LockWithRefcnt {
int refcnt = 1;
}

/**
* Poor man's reference counted object pool.
*
* <p>Maintains a mapping of runfiles directory to a monitor. The monitor maintains a counter that
* tracks how many threads it is acquired by at the moment. If the count drops to zero the mapping
* is removed.
*/
@GuardedBy("lock")
private final Map<PathFragment, LockWithRefcnt> locksWithRefcnt = new HashMap<>();

private RunfilesTreeUpdater() {}

private static void updateRunfilesTree(
Path execRoot,
PathFragment runfilesDir,
BinTools binTools,
ImmutableMap<String, String> env,
OutErr outErr,
boolean enableRunfiles)
throws IOException, ExecException {
Path runfilesDirPath = execRoot.getRelative(runfilesDir);
Path inputManifest = RunfilesSupport.inputManifestPath(runfilesDirPath);
if (!inputManifest.exists()) {
return;
}

Path outputManifest = RunfilesSupport.outputManifestPath(runfilesDirPath);
try {
// Avoid rebuilding the runfiles directory if the manifest in it matches the input manifest,
// implying the symlinks exist and are already up to date. If the output manifest is a
// symbolic link, it is likely a symbolic link to the input manifest, so we cannot trust it as
// an up-to-date check.
if (!outputManifest.isSymbolicLink()
&& Arrays.equals(outputManifest.getDigest(), inputManifest.getDigest())) {
return;
}
} catch (IOException e) {
// Ignore it - we will just try to create runfiles directory.
}

if (!runfilesDirPath.exists()) {
runfilesDirPath.createDirectoryAndParents();
}

SymlinkTreeHelper helper =
new SymlinkTreeHelper(inputManifest, runfilesDirPath, /* filesetTree= */ false);
helper.createSymlinks(execRoot, outErr, binTools, env, enableRunfiles);
}

private LockWithRefcnt getLockAndIncrementRefcnt(PathFragment runfilesDirectory) {
synchronized (lock) {
LockWithRefcnt lock = locksWithRefcnt.get(runfilesDirectory);
if (lock != null) {
lock.refcnt++;
return lock;
}
lock = new LockWithRefcnt();
locksWithRefcnt.put(runfilesDirectory, lock);
return lock;
}
}

private void decrementRefcnt(PathFragment runfilesDirectory) {
synchronized (lock) {
LockWithRefcnt lock = locksWithRefcnt.get(runfilesDirectory);
lock.refcnt--;
if (lock.refcnt == 0) {
if (!locksWithRefcnt.remove(runfilesDirectory, lock)) {
throw new IllegalStateException(
String.format(
"Failed to remove lock for dir '%s'." + " This is a bug.", runfilesDirectory));
}
}
}
}

public void updateRunfilesDirectory(
Path execRoot,
RunfilesSupplier runfilesSupplier,
ArtifactPathResolver pathResolver,
BinTools binTools,
ImmutableMap<String, String> env,
OutErr outErr)
throws ExecException, IOException {
for (Map.Entry<PathFragment, Map<PathFragment, Artifact>> runfiles :
runfilesSupplier.getMappings(pathResolver).entrySet()) {
PathFragment runfilesDir = runfiles.getKey();
if (runfilesSupplier.isBuildRunfileLinks(runfilesDir)) {
continue;
}

try {
long startTime = Profiler.nanoTimeMaybe();
// Synchronize runfiles tree generation on the runfiles directory in order to prevent
// concurrent modifications of the runfiles tree. In particular this can happen for sharded
// tests and --runs_per_test > 1 in which case multiple actions use the same runfiles tree.
synchronized (getLockAndIncrementRefcnt(runfilesDir)) {
Profiler.instance()
.logSimpleTask(startTime, ProfilerTask.WAIT, "Waiting to create runfiles tree");
updateRunfilesTree(
execRoot,
runfilesDir,
binTools,
env,
outErr,
runfilesSupplier.isRunfileLinksEnabled(runfilesDir));
}
} finally {
decrementRefcnt(runfilesDir);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.ExecutionStrategy;
Expand All @@ -32,6 +33,7 @@
import com.google.devtools.build.lib.actions.SpawnActionContext;
import com.google.devtools.build.lib.actions.SpawnContinuation;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.TestExecException;
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
import com.google.devtools.build.lib.analysis.test.TestActionContext;
import com.google.devtools.build.lib.analysis.test.TestResult;
Expand Down Expand Up @@ -92,20 +94,14 @@ public StandaloneTestStrategy(

@Override
public TestRunnerSpawn createTestRunnerSpawn(
TestRunnerAction action, ActionExecutionContext actionExecutionContext)
throws ExecException, InterruptedException {
TestRunnerAction action, ActionExecutionContext actionExecutionContext) throws ExecException {
if (action.getExecutionSettings().getInputManifest() == null) {
throw new TestExecException("cannot run local tests with --nobuild_runfile_manifests");
}
Path execRoot = actionExecutionContext.getExecRoot();
Path runfilesDir =
getLocalRunfilesDirectory(
action,
actionExecutionContext,
binTools,
action.getLocalShellEnvironment(),
action.isEnableRunfiles());
Path tmpDir =
actionExecutionContext
.getPathResolver()
.convertPath(tmpDirRoot.getChild(TestStrategy.getTmpDirName(action)));
ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver();
Path runfilesDir = pathResolver.convertPath(action.getExecutionSettings().getRunfilesDir());
Path tmpDir = pathResolver.convertPath(tmpDirRoot.getChild(TestStrategy.getTmpDirName(action)));
Map<String, String> env = setupEnvironment(
action, actionExecutionContext.getClientEnv(), execRoot, runfilesDir, tmpDir);
if (executionOptions.splitXmlGeneration) {
Expand Down
106 changes: 0 additions & 106 deletions src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,13 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.io.ByteStreams;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.CommandLineExpansionException;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.TestExecException;
import com.google.devtools.build.lib.actions.UserExecException;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
Expand All @@ -38,8 +36,6 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventKind;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.io.FileWatcher;
Expand All @@ -54,7 +50,6 @@
import java.io.IOException;
import java.io.InputStream;
import java.time.Duration;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -392,107 +387,6 @@ public static Map<String, String> getMapping(
return result;
}

/**
* Returns the runfiles directory associated with the test executable, creating/updating it if
* necessary and --build_runfile_links is specified.
*/
protected static Path getLocalRunfilesDirectory(
TestRunnerAction testAction,
ActionExecutionContext actionExecutionContext,
BinTools binTools,
ImmutableMap<String, String> shellEnvironment,
boolean enableRunfiles)
throws ExecException, InterruptedException {
TestTargetExecutionSettings execSettings = testAction.getExecutionSettings();

if (execSettings.getInputManifest() == null) {
throw new TestExecException("cannot run local tests with --nobuild_runfile_manifests");
}

Path runfilesDir =
actionExecutionContext.getPathResolver().convertPath(execSettings.getRunfilesDir());

// If the symlink farm is already created then return the existing directory. If not we
// need to explicitly build it. This can happen when --nobuild_runfile_links is supplied
// as a flag to the build.
if (execSettings.getRunfilesSymlinksCreated()) {
return runfilesDir;
}

// Synchronize runfiles tree generation on the runfiles manifest artifact.
// This is necessary, because we might end up with multiple test runner actions
// trying to generate same runfiles tree in case of --runs_per_test > 1 or
// local test sharding.
long startTime = Profiler.nanoTimeMaybe();
synchronized (execSettings.getInputManifest()) {
Profiler.instance().logSimpleTask(startTime, ProfilerTask.WAIT, testAction.describe());
updateLocalRunfilesDirectory(
testAction,
runfilesDir,
actionExecutionContext,
binTools,
shellEnvironment,
enableRunfiles);
}

return runfilesDir;
}

/**
* Ensure the runfiles tree exists and is consistent with the TestAction's manifest
* ($0.runfiles_manifest), bringing it into consistency if not. The contents of the output file
* $0.runfiles/MANIFEST, if it exists, are used a proxy for the set of existing symlinks, to avoid
* the need for recursion.
*/
private static void updateLocalRunfilesDirectory(
TestRunnerAction testAction,
Path runfilesDir,
ActionExecutionContext actionExecutionContext,
BinTools binTools,
ImmutableMap<String, String> shellEnvironment,
boolean enableRunfiles)
throws ExecException, InterruptedException {
TestTargetExecutionSettings execSettings = testAction.getExecutionSettings();
Path outputManifest = runfilesDir.getRelative("MANIFEST");
try {
// Avoid rebuilding the runfiles directory if the manifest in it matches the input manifest,
// implying the symlinks exist and are already up to date. If the output manifest is a
// symbolic link, it is likely a symbolic link to the input manifest, so we cannot trust it as
// an up-to-date check.
if (!outputManifest.isSymbolicLink()
&& Arrays.equals(
outputManifest.getDigest(),
actionExecutionContext.getInputPath(execSettings.getInputManifest()).getDigest())) {
return;
}
} catch (IOException e1) {
// Ignore it - we will just try to create runfiles directory.
}

actionExecutionContext
.getEventHandler()
.handle(
Event.progress(
"Building runfiles directory for '"
+ execSettings.getExecutable().prettyPrint()
+ "'."));

new SymlinkTreeHelper(
actionExecutionContext.getInputPath(execSettings.getInputManifest()),
runfilesDir,
false)
.createSymlinks(
actionExecutionContext.getExecRoot(),
actionExecutionContext.getFileOutErr(),
binTools,
shellEnvironment,
enableRunfiles);

actionExecutionContext
.getEventHandler()
.handle(Event.progress(testAction.getProgressMessage()));
}

protected static void closeSuppressed(Throwable e, @Nullable Closeable c) {
if (c == null) {
return;
Expand Down
Loading

0 comments on commit 580038e

Please sign in to comment.