children) {
+ this.detailedExitCode = detailedExitCode;
+ this.exitCode = detailedExitCode.getExitCode();
+ this.finishTimeMillis = finishTimeMillis;
+ this.children = children;
+ }
+
public BuildCompletingEvent(ExitCode exitCode, long finishTimeMillis) {
this(exitCode, finishTimeMillis, ImmutableList.of());
}
@@ -65,13 +77,17 @@ public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext convert
.setCode(exitCode.getNumericExitCode())
.build();
- BuildEventStreamProtos.BuildFinished finished =
+ BuildEventStreamProtos.BuildFinished.Builder finished =
BuildEventStreamProtos.BuildFinished.newBuilder()
.setOverallSuccess(ExitCode.SUCCESS.equals(exitCode))
.setExitCode(protoExitCode)
.setFinishTime(Timestamps.fromMillis(finishTimeMillis))
- .setFinishTimeMillis(finishTimeMillis)
- .build();
- return GenericBuildEvent.protoChaining(this).setFinished(finished).build();
+ .setFinishTimeMillis(finishTimeMillis);
+
+ if (detailedExitCode != null && detailedExitCode.getFailureDetail() != null) {
+ finished.setFailureDetail(detailedExitCode.getFailureDetail());
+ }
+
+ return GenericBuildEvent.protoChaining(this).setFinished(finished.build()).build();
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto
index e9b88ab1a72c8c..12bfa920b006a2 100644
--- a/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto
+++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto
@@ -836,6 +836,9 @@ message BuildFinished {
google.protobuf.Timestamp finish_time = 5;
AnomalyReport anomaly_report = 4 [deprecated = true];
+
+ // Only populated if success = false, and sometimes not even then.
+ failure_details.FailureDetail failure_detail = 6;
}
message BuildMetrics {
@@ -965,6 +968,9 @@ message BuildMetrics {
PackageMetrics package_metrics = 4;
message TimingMetrics {
+ // For Skymeld,
+ // analysis_phase_time_in_ms + execution_phase_time_in_ms >= wall_time_in_ms
+ //
// The CPU time in milliseconds consumed during this build.
int64 cpu_time_in_ms = 1;
// The elapsed wall time in milliseconds during this build.
@@ -973,6 +979,10 @@ message BuildMetrics {
// When analysis and execution phases are interleaved, this measures the
// elapsed time from the first analysis work to the last.
int64 analysis_phase_time_in_ms = 3;
+ // The elapsed wall time in milliseconds during the execution phase.
+ // When analysis and execution phases are interleaved, this measures the
+ // elapsed time from the first action execution to the last.
+ int64 execution_phase_time_in_ms = 4;
}
TimingMetrics timing_metrics = 5;
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java
index 48842f71178282..197d944d1b183e 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java
@@ -310,36 +310,6 @@ public String getSymlinkPrefix(String productName) {
return symlinkPrefix == null ? productName + "-" : symlinkPrefix;
}
- // Transitional flag for safely rolling out new convenience symlink behavior.
- // To be made a no-op and deleted once new symlink behavior is battle-tested.
- @Option(
- name = "use_top_level_targets_for_symlinks",
- defaultValue = "true",
- documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
- effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
- help =
- "If enabled, the symlinks are based on the configurations of the top-level targets "
- + " rather than the top-level target configuration. If this would be ambiguous, "
- + " the symlinks will be deleted to avoid confusion.")
- public boolean useTopLevelTargetsForSymlinks;
-
- /**
- * Returns whether to use the output directories used by the top-level targets for convenience
- * symlinks.
- *
- * If true, then symlinks use the actual output directories of the top-level targets. The
- * symlinks will be created iff all top-level targets share the same output directory. Otherwise,
- * any stale symlinks from previous invocations will be deleted to avoid ambiguity.
- *
- *
If false, then symlinks use the output directory implied by command-line flags, regardless
- * of whether top-level targets have transitions which change them (or even have any output
- * directories at all, as in the case of a build with no targets or one which only builds source
- * files).
- */
- public boolean useTopLevelTargetsForSymlinks() {
- return useTopLevelTargetsForSymlinks;
- }
-
@Option(
name = "experimental_create_py_symlinks",
defaultValue = "false",
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
index 5d1771e6ee453d..91c74921e94ad2 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
@@ -743,15 +743,18 @@ public void handleConvenienceSymlinks(
/**
* Creates convenience symlinks based on the target configurations.
*
- *
Exactly what target configurations we consider depends on the value of {@code
- * --use_top_level_targets_for_symlinks}. If this flag is false, we use the top-level target
- * configuration as represented by the command line prior to processing any target. If the flag is
- * true, we instead use the configurations OF the top-level targets -- meaning that we account for
- * the effects of any rule transitions these targets may have.
+ *
Top-level targets may have different configurations than the top-level configuration. This
+ * is because targets may apply configuration transitions.
*
- *
For each type of convenience symlink, if all the considered configurations agree on what
- * path the symlink should point to, it gets created; otherwise, the symlink is not created, and
- * in fact gets removed if it was already present from a previous invocation.
+ *
If all top-level targets have the same configuration - even if that isn't the top-level
+ * configuration - symlinks point to that configuration.
+ *
+ *
If top-level targets have mixed configurations and at least one of them has the top-level
+ * configuration, symliks point to the top-level configuration.
+ *
+ *
If top-level targets have mixed configurations and none has the top-level configuration,
+ * symlinks aren't created. Furthermore, lingering symlinks from the last build are deleted. This
+ * is to prevent confusion by pointing to an outdated directory the current build never used.
*/
private ImmutableList createConvenienceSymlinks(
BuildRequestOptions buildRequestOptions,
@@ -762,7 +765,9 @@ private ImmutableList createConvenienceSymlinks(
// Gather configurations to consider.
ImmutableSet targetConfigs;
- if (buildRequestOptions.useTopLevelTargetsForSymlinks() && !targetsToBuild.isEmpty()) {
+ if (targetsToBuild.isEmpty()) {
+ targetConfigs = ImmutableSet.of(configuration);
+ } else {
// 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 requestedTargetConfigs =
@@ -791,8 +796,6 @@ private ImmutableList createConvenienceSymlinks(
// createOutputDirectorySymlinks call below.
targetConfigs = requestedTargetConfigs;
}
- } else {
- targetConfigs = ImmutableSet.of(configuration);
}
String productName = runtime.getProductName();
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/BuildCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/BuildCompleteEvent.java
index 2b77fa5b0d9b4d..ff631281bda0f1 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/BuildCompleteEvent.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/BuildCompleteEvent.java
@@ -32,7 +32,7 @@ public final class BuildCompleteEvent extends BuildCompletingEvent {
/** Construct the BuildCompleteEvent. */
public BuildCompleteEvent(BuildResult result, Collection children) {
- super(result.getDetailedExitCode().getExitCode(), result.getStopTime(), children);
+ super(result.getDetailedExitCode(), result.getStopTime(), children);
this.result = checkNotNull(result);
}
diff --git a/src/main/java/com/google/devtools/build/lib/concurrent/PriorityWorkerPool.java b/src/main/java/com/google/devtools/build/lib/concurrent/PriorityWorkerPool.java
index 6808fbf0928d06..59678bf48945cc 100644
--- a/src/main/java/com/google/devtools/build/lib/concurrent/PriorityWorkerPool.java
+++ b/src/main/java/com/google/devtools/build/lib/concurrent/PriorityWorkerPool.java
@@ -21,9 +21,7 @@
import static com.google.devtools.build.lib.concurrent.PriorityWorkerPool.NextWorkerActivity.DO_CPU_HEAVY_TASK;
import static com.google.devtools.build.lib.concurrent.PriorityWorkerPool.NextWorkerActivity.DO_TASK;
import static com.google.devtools.build.lib.concurrent.PriorityWorkerPool.NextWorkerActivity.IDLE;
-import static com.google.devtools.build.lib.concurrent.PriorityWorkerPool.NextWorkerActivity.QUIESCENT;
import static java.lang.Thread.currentThread;
-import static java.util.concurrent.TimeUnit.SECONDS;
import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;
@@ -131,15 +129,18 @@ final class PriorityWorkerPool {
this.pool = newForkJoinPool();
this.errorClassifier = errorClassifier;
- long baseAddress = createPaddedBaseAddress(4);
+ long baseAddress = createPaddedBaseAddress(5);
cleaner.register(this, new AddressFreer(baseAddress));
this.countersAddress = getAlignedAddress(baseAddress, /* offset= */ 0);
+
this.queue =
new TaskFifo(
/* sizeAddress= */ getAlignedAddress(baseAddress, /* offset= */ 1),
/* appendIndexAddress= */ getAlignedAddress(baseAddress, /* offset= */ 2),
/* takeIndexAddress= */ getAlignedAddress(baseAddress, /* offset= */ 3));
+ this.activeWorkerCountAddress = getAlignedAddress(baseAddress, /* offset= */ 4);
+
resetExecutionCounters();
}
@@ -157,29 +158,34 @@ void execute(Runnable rawTask) {
if (task.isCpuHeavy()) {
cpuHeavyQueue.add(task);
if (acquireThreadAndCpuPermitElseReleaseCpuHeavyTask()) {
+ UNSAFE.getAndAddInt(null, activeWorkerCountAddress, 1);
pool.execute(RUN_CPU_HEAVY_TASK);
}
return;
}
}
- while (!queue.tryAppend(rawTask)) {
- // If the queue is full, this thread donates some work to reduce the queue.
- if (!tryAcquireTask()) {
- // This should be very hard to reach if the queue is full except under cancellation. It's
- // possible to perform the cancellation check in advance, but we can save doing the check in
- // most cases by deferring it to this branch.
- if (isCancelled()) {
- return;
- }
- logger.atWarning().atMostEvery(5, SECONDS).log(
- "Queue is full but no tasks could be acquired: %s", this);
- continue;
+ if (!queue.tryAppend(rawTask)) {
+ if (!isCancelled()) {
+ // The task queue is full (and the pool is not cancelled). Enqueues the task directly in the
+ // ForkJoinPool. This should be rare in practice.
+ UNSAFE.getAndAddInt(null, activeWorkerCountAddress, 1);
+ pool.execute(
+ () -> {
+ try {
+ rawTask.run();
+ } catch (Throwable uncaught) {
+ handleUncaughtError(uncaught);
+ } finally {
+ workerBecomingIdle();
+ }
+ });
}
- dequeueTaskAndRun();
+ return;
}
if (acquireThreadElseReleaseTask()) {
+ UNSAFE.getAndAddInt(null, activeWorkerCountAddress, 1);
pool.execute(RUN_TASK);
}
}
@@ -328,12 +334,9 @@ private ForkJoinPool newForkJoinPool() {
*
* After completing a task, the worker checks if there are any available tasks that it may
* execute, subject to CPU permit constraints. On finding and reserving an appropriate task, the
- * worker returns its next planned activity, {@link #IDLE} or {@link #QUIESCENT} if it finds
- * nothing to do.
+ * worker returns its next planned activity, {@link #IDLE} if it finds nothing to do.
*/
enum NextWorkerActivity {
- /** The worker will stop and is the last worker working. */
- QUIESCENT,
/** The worker will stop. */
IDLE,
/** The worker will perform a non-CPU heavy task. */
@@ -386,12 +389,8 @@ private WorkerThread(ForkJoinPool pool, String name) {
private void runLoop(NextWorkerActivity nextActivity) {
while (true) {
switch (nextActivity) {
- case QUIESCENT:
- synchronized (quiescenceMonitor) {
- quiescenceMonitor.notifyAll();
- }
- return;
case IDLE:
+ workerBecomingIdle();
return;
case DO_TASK:
dequeueTaskAndRun();
@@ -404,14 +403,6 @@ private void runLoop(NextWorkerActivity nextActivity) {
}
}
}
-
- boolean tryDoQueuedWork() {
- if (!tryAcquireTask()) {
- return false;
- }
- dequeueTaskAndRun();
- return true;
- }
}
private void dequeueTaskAndRun() {
@@ -431,6 +422,14 @@ private void dequeueCpuHeavyTaskAndRun() {
}
}
+ private void workerBecomingIdle() {
+ if (UNSAFE.getAndAddInt(null, activeWorkerCountAddress, -1) == 1) {
+ synchronized (quiescenceMonitor) {
+ quiescenceMonitor.notifyAll();
+ }
+ }
+ }
+
// The constants below apply to the 64-bit execution counters value.
private static final long CANCEL_BIT = 0x8000_0000_0000_0000L;
@@ -479,10 +478,10 @@ private void dequeueCpuHeavyTaskAndRun() {
*/
private final long countersAddress;
+ private final long activeWorkerCountAddress;
+
boolean isQuiescent() {
- long snapshot = getExecutionCounters();
- int threadsSnapshot = (int) ((snapshot & THREADS_MASK) >> THREADS_BIT_OFFSET);
- return threadsSnapshot == poolSize;
+ return UNSAFE.getInt(null, activeWorkerCountAddress) == 0;
}
boolean isCancelled() {
@@ -505,6 +504,7 @@ private void resetExecutionCounters() {
countersAddress,
(((long) poolSize) << THREADS_BIT_OFFSET)
| (((long) cpuPermits) << CPU_PERMITS_BIT_OFFSET));
+ UNSAFE.putInt(null, activeWorkerCountAddress, 0);
}
private boolean acquireThreadElseReleaseTask() {
@@ -532,17 +532,6 @@ private boolean acquireThreadAndCpuPermitElseReleaseCpuHeavyTask() {
} while (true);
}
- private boolean tryAcquireTask() {
- long snapshot;
- do {
- snapshot = getExecutionCounters();
- if ((snapshot & TASKS_MASK) == 0 || snapshot < 0) {
- return false;
- }
- } while (!tryUpdateExecutionCounters(snapshot, snapshot - ONE_TASK));
- return true;
- }
-
/**
* Worker threads determine their next action after completing a task using this method.
*
@@ -564,7 +553,7 @@ private NextWorkerActivity getActivityFollowingTask() {
} else {
long target = snapshot + ONE_THREAD;
if (UNSAFE.compareAndSwapLong(null, countersAddress, snapshot, target)) {
- return quiescentOrIdle(target);
+ return IDLE;
}
}
snapshot = UNSAFE.getLong(null, countersAddress);
@@ -574,8 +563,8 @@ private NextWorkerActivity getActivityFollowingTask() {
/**
* Worker threads call this to determine their next action after completing a CPU heavy task.
*
- *
This releases a CPU permit when returning {@link NextWorkerActivity#QUIESCENT}, {@link
- * NextWorkerActivity#IDLE} or {@link NextWorkerActivity#DO_TASK}.
+ *
This releases a CPU permit when returning {@link NextWorkerActivity#IDLE} or {@link
+ * NextWorkerActivity#DO_TASK}.
*/
private NextWorkerActivity getActivityFollowingCpuHeavyTask() {
long snapshot = UNSAFE.getLongVolatile(null, countersAddress);
@@ -593,18 +582,13 @@ private NextWorkerActivity getActivityFollowingCpuHeavyTask() {
} else {
long target = snapshot + CPU_HEAVY_RESOURCES;
if (UNSAFE.compareAndSwapLong(null, countersAddress, snapshot, target)) {
- return quiescentOrIdle(target);
+ return IDLE;
}
}
snapshot = UNSAFE.getLong(null, countersAddress);
} while (true);
}
- private NextWorkerActivity quiescentOrIdle(long snapshot) {
- int snapshotThreads = (int) ((snapshot & THREADS_MASK) >> THREADS_BIT_OFFSET);
- return snapshotThreads == poolSize ? QUIESCENT : IDLE;
- }
-
// Throughout this class, the following wrappers are used where possible, but they are often not
// inlined by the JVM even though they show up on profiles, so they are inlined explicitly in
// numerous cases.
diff --git a/src/main/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutor.java b/src/main/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutor.java
index 1d1912a5759094..3aa135b6f565cc 100644
--- a/src/main/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutor.java
@@ -16,7 +16,6 @@
import static com.google.common.base.MoreObjects.toStringHelper;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Throwables.throwIfUnchecked;
-import static java.lang.Thread.currentThread;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.util.concurrent.ListenableFuture;
@@ -41,11 +40,6 @@
*
*
The queue for non-CPUHeavy tasks has a fixed capacity. When full, callers of execute assist
* with enqueued work.
- *
- *
Threads may voluntarily assist with queued work by calling {@link
- * TieredPriorityExecutor#tryDoQueuedWork} when a thread is about to block. If tasks are available,
- * the current thread may perform a task inside {@link TieredPriorityExecutor#tryDoQueuedWork}
- * before it returns. This may add latency to the donating thread but can reduce overhead.
*/
public final class TieredPriorityExecutor implements QuiescingExecutor {
/** A common cleaner shared by all executors. */
@@ -163,23 +157,6 @@ public CountDownLatch getInterruptionLatchForTestingOnly() {
throw new UnsupportedOperationException();
}
- /**
- * Attempts to donate work on the current thread.
- *
- *
Calling this method may be useful if the current thread is about to block. Subject to
- * scheduling constraints, attempts to poll work from the queue and execute it on the current
- * thread.
- *
- * @return true if work was donated, false otherwise.
- */
- public static boolean tryDoQueuedWork() {
- var thread = currentThread();
- if (!(thread instanceof PriorityWorkerPool.WorkerThread)) {
- return false;
- }
- return ((PriorityWorkerPool.WorkerThread) thread).tryDoQueuedWork();
- }
-
/**
* The parallelism target of the underlying thread pool.
*
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
index 379f41f28170c1..c95e51bf596ea8 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
@@ -138,12 +138,13 @@ public void createSymlinks(
/** Copies the input manifest to the output manifest. */
public void copyManifest() throws ExecException {
- // Pretend we created the runfiles tree by copying the manifest
+ // Pretend we created the runfiles tree by symlinking the output manifest to the input manifest.
+ Path outputManifest = getOutputManifest();
try {
symlinkTreeRoot.createDirectoryAndParents();
- FileSystemUtils.copyFile(inputManifest, getOutputManifest());
+ outputManifest.createSymbolicLink(inputManifest);
} catch (IOException e) {
- throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_MANIFEST_COPY_IO_EXCEPTION);
+ throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_MANIFEST_LINK_IO_EXCEPTION);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
index 3d9ce428385932..cbcc6e7d997ba5 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
@@ -425,7 +425,8 @@ private SpawnResult start()
TerminationStatus terminationStatus;
try (SilentCloseable c =
Profiler.instance()
- .profile(ProfilerTask.PROCESS_TIME, spawn.getResourceOwner().getMnemonic())) {
+ .profile(
+ ProfilerTask.REMOTE_PROCESS_TIME, spawn.getResourceOwner().getMnemonic())) {
needCleanup = true;
Subprocess subprocess = subprocessBuilder.start();
try {
diff --git a/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java b/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java
index 450f1eb4ef8335..f2b4471b05ae6c 100644
--- a/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java
+++ b/src/main/java/com/google/devtools/build/lib/metrics/MetricsCollector.java
@@ -99,6 +99,7 @@ class MetricsCollector {
// TopLevelTargetExecutionStartedEvent. This AtomicBoolean is so that we only account for the
// build once.
private final AtomicBoolean buildAccountedFor;
+ private long executionStartMillis;
@CanIgnoreReturnValue
private MetricsCollector(
@@ -157,6 +158,10 @@ public void onAnalysisPhaseComplete(AnalysisPhaseCompleteEvent event) {
}
}
+ private void markExecutionPhaseStarted() {
+ executionStartMillis = BlazeClock.instance().currentTimeMillis();
+ }
+
@SuppressWarnings("unused")
@Subscribe
public synchronized void logAnalysisGraphStats(AnalysisGraphStatsEvent event) {
@@ -175,13 +180,17 @@ public synchronized void logAnalysisGraphStats(AnalysisGraphStatsEvent event) {
@SuppressWarnings("unused")
@Subscribe
public synchronized void logExecutionStartingEvent(ExecutionStartingEvent event) {
+ markExecutionPhaseStarted();
numBuilds.getAndIncrement();
}
+ // Skymeld-specific: we don't have an ExecutionStartingEvent for skymeld, so we have to use
+ // TopLevelTargetExecutionStartedEvent
@Subscribe
- public synchronized void accountForBuild(
+ public synchronized void handleExecutionPhaseStart(
@SuppressWarnings("unused") TopLevelTargetPendingExecutionEvent event) {
if (buildAccountedFor.compareAndSet(/*expectedValue=*/ false, /*newValue=*/ true)) {
+ markExecutionPhaseStarted();
numBuilds.getAndIncrement();
}
}
@@ -255,6 +264,8 @@ public void actionResultReceived(ActionResultReceivedEvent event) {
@SuppressWarnings("unused")
@Subscribe
public void onExecutionComplete(ExecutionFinishedEvent event) {
+ timingMetrics.setExecutionPhaseTimeInMs(
+ BlazeClock.instance().currentTimeMillis() - executionStartMillis);
artifactMetrics
.setSourceArtifactsRead(event.sourceArtifactsRead())
.setOutputArtifactsSeen(event.outputArtifactsSeen())
diff --git a/src/main/java/com/google/devtools/build/lib/profiler/ProfilerTask.java b/src/main/java/com/google/devtools/build/lib/profiler/ProfilerTask.java
index f317a3ffe8a52d..8733b86cbe2477 100644
--- a/src/main/java/com/google/devtools/build/lib/profiler/ProfilerTask.java
+++ b/src/main/java/com/google/devtools/build/lib/profiler/ProfilerTask.java
@@ -38,7 +38,7 @@ public enum ProfilerTask {
Threshold.FIFTY_MILLIS,
/* collectsSlowestInstances= */ true),
UPLOAD_TIME("Remote execution upload time", Threshold.FIFTY_MILLIS),
- PROCESS_TIME("Remote execution process wall time", Threshold.FIFTY_MILLIS),
+ REMOTE_PROCESS_TIME("Remote execution process wall time", Threshold.FIFTY_MILLIS),
REMOTE_QUEUE("Remote execution queuing time", Threshold.FIFTY_MILLIS),
REMOTE_SETUP("Remote execution setup", Threshold.FIFTY_MILLIS),
FETCH("Remote execution file fetching", Threshold.FIFTY_MILLIS),
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
index 12949636e783f2..832199e4969b9a 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java
@@ -44,6 +44,7 @@
import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.vfs.Dirent;
import com.google.devtools.build.lib.vfs.FileStatus;
+import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -87,6 +88,16 @@ public class RemoteActionFileSystem extends AbstractFileSystemWithCustomStat {
@Nullable private ActionExecutionMetadata action = null;
@Nullable private MetadataInjector metadataInjector = null;
+ /** Describes how to handle symlinks when calling {@link #statUnchecked}. */
+ private enum FollowMode {
+ /** Dereference the entire path. This is equivalent to {@link Symlinks.FOLLOW}. */
+ FOLLOW_ALL,
+ /** Dereference the parent path. This is equivalent to {@link Symlinks.NOFOLLOW}. */
+ FOLLOW_PARENT,
+ /** Do not dereference. This is only used internally to resolve symlinks efficiently. */
+ FOLLOW_NONE
+ };
+
RemoteActionFileSystem(
FileSystem localFs,
PathFragment execRootFragment,
@@ -143,7 +154,9 @@ boolean isRemote(Path path) {
}
private boolean isRemote(PathFragment path) {
- return getRemoteMetadata(path) != null;
+ var status = statInMemory(path, FollowMode.FOLLOW_ALL);
+ return (status instanceof FileStatusWithMetadata)
+ && ((FileStatusWithMetadata) status).getMetadata().isRemote();
}
public void updateContext(ActionExecutionMetadata action, MetadataInjector metadataInjector) {
@@ -214,8 +227,11 @@ private void maybeInjectMetadataForSymlinkOrDownload(PathFragment linkPath, Arti
"non-symlink artifact materialized as symlink must point to absolute path");
if (output.isTreeArtifact()) {
- TreeArtifactValue metadata = getRemoteTreeMetadata(targetPath);
- if (metadata == null) {
+ TreeArtifactValue metadata =
+ inputArtifactData.getTreeMetadata(targetPath.relativeTo(execRoot));
+
+ // TODO: Handle partially remote tree artifacts.
+ if (metadata == null || !metadata.isEntirelyRemote()) {
return;
}
@@ -235,7 +251,14 @@ private void maybeInjectMetadataForSymlinkOrDownload(PathFragment linkPath, Arti
metadataInjector.injectTree(parent, injectedTree.build());
} else {
- RemoteFileArtifactValue metadata = getRemoteMetadata(targetPath);
+ RemoteFileArtifactValue metadata = null;
+
+ var status = statInMemory(targetPath, FollowMode.FOLLOW_ALL);
+ if (status instanceof FileStatusWithMetadata
+ && ((FileStatusWithMetadata) status).getMetadata().isRemote()) {
+ metadata = (RemoteFileArtifactValue) ((FileStatusWithMetadata) status).getMetadata();
+ }
+
if (metadata == null) {
return;
}
@@ -327,18 +350,18 @@ public byte[] getxattr(PathFragment path, String name, boolean followSymlinks)
@Override
protected byte[] getFastDigest(PathFragment path) throws IOException {
- RemoteFileArtifactValue m = getRemoteMetadata(path);
- if (m != null) {
- return m.getDigest();
+ var stat = statInMemory(path, FollowMode.FOLLOW_ALL);
+ if (stat instanceof FileStatusWithDigest) {
+ return ((FileStatusWithDigest) stat).getDigest();
}
return localFs.getPath(path).getFastDigest();
}
@Override
protected byte[] getDigest(PathFragment path) throws IOException {
- RemoteFileArtifactValue m = getRemoteMetadata(path);
- if (m != null) {
- return m.getDigest();
+ var status = statInMemory(path, FollowMode.FOLLOW_ALL);
+ if (status instanceof FileStatusWithDigest) {
+ return ((FileStatusWithDigest) status).getDigest();
}
return localFs.getPath(path).getDigest();
}
@@ -348,7 +371,7 @@ protected byte[] getDigest(PathFragment path) throws IOException {
// permissions.
private boolean existsInMemory(PathFragment path) {
- return remoteOutputTree.getPath(path).isDirectory() || getRemoteMetadata(path) != null;
+ return statInMemory(path, FollowMode.FOLLOW_ALL) != null;
}
@Override
@@ -430,10 +453,12 @@ protected void chmod(PathFragment path, int mode) throws IOException {
@Override
protected PathFragment readSymbolicLink(PathFragment path) throws IOException {
- RemoteFileArtifactValue m = getRemoteMetadata(path);
- if (m != null) {
- // We don't support symlinks as remote action outputs.
- throw new IOException(path + " is not a symbolic link");
+ if (isOutput(path)) {
+ try {
+ return remoteOutputTree.getPath(path).readSymbolicLink();
+ } catch (FileNotFoundException e) {
+ // Intentionally ignored.
+ }
}
return localFs.getPath(path).readSymbolicLink();
}
@@ -441,33 +466,32 @@ protected PathFragment readSymbolicLink(PathFragment path) throws IOException {
@Override
protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment)
throws IOException {
- localFs.getPath(linkPath).createSymbolicLink(targetFragment);
if (isOutput(linkPath)) {
remoteOutputTree.getPath(linkPath).createSymbolicLink(targetFragment);
}
+
+ localFs.getPath(linkPath).createSymbolicLink(targetFragment);
}
+ @Nullable
@Override
- protected Path resolveSymbolicLinks(PathFragment path) throws IOException {
- return localFs.getPath(path).resolveSymbolicLinks();
+ protected PathFragment resolveOneLink(PathFragment path) throws IOException {
+ // The base implementation attempts to readSymbolicLink first and falls back to stat, but that
+ // unnecessarily allocates a NotASymlinkException in the overwhelmingly likely non-symlink case.
+ // It's more efficient to stat unconditionally.
+ //
+ // The parent path has already been canonicalized, so FOLLOW_NONE is effectively the same as
+ // FOLLOW_PARENT, but much more efficient as it doesn't call stat recursively.
+ var stat = statUnchecked(path, FollowMode.FOLLOW_NONE);
+ return stat.isSymbolicLink() ? readSymbolicLink(path) : null;
}
// -------------------- Implementations based on stat() --------------------
@Override
protected long getLastModifiedTime(PathFragment path, boolean followSymlinks) throws IOException {
- try {
- // We can't get mtime for a remote file, use mtime of in-memory file node instead.
- return remoteOutputTree
- .getPath(path)
- .getLastModifiedTime(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW);
- } catch (FileNotFoundException e) {
- // Intentionally ignored
- }
-
- return localFs
- .getPath(path)
- .getLastModifiedTime(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW);
+ FileStatus stat = stat(path, followSymlinks);
+ return stat.getLastModifiedTime();
}
@Override
@@ -485,11 +509,6 @@ protected boolean exists(PathFragment path, boolean followSymlinks) {
}
}
- @Override
- public boolean exists(PathFragment path) {
- return exists(path, /* followSymlinks= */ true);
- }
-
@Nullable
@Override
protected FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws IOException {
@@ -512,14 +531,43 @@ protected FileStatus statNullable(PathFragment path, boolean followSymlinks) {
@Override
protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOException {
- RemoteFileArtifactValue m = getRemoteMetadata(path);
- if (m != null) {
- return statFromRemoteMetadata(m);
+ return statUnchecked(path, followSymlinks ? FollowMode.FOLLOW_ALL : FollowMode.FOLLOW_PARENT);
+ }
+
+ @Nullable
+ private FileStatus statUnchecked(PathFragment path, FollowMode followMode) throws IOException {
+ if (followMode == FollowMode.FOLLOW_ALL) {
+ path = resolveSymbolicLinks(path).asFragment();
+ } else if (followMode == FollowMode.FOLLOW_PARENT) {
+ PathFragment parent = path.getParentDirectory();
+ if (parent != null) {
+ path = resolveSymbolicLinks(parent).asFragment().getChild(path.getBaseName());
+ }
+ }
+
+ var status = statInMemory(path, followMode);
+ if (status != null) {
+ return status;
}
- return localFs.getPath(path).stat(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW);
+ // The path has already been canonicalized above.
+ return localFs.getPath(path).stat(Symlinks.NOFOLLOW);
}
- private static FileStatus statFromRemoteMetadata(RemoteFileArtifactValue m) {
+ @Nullable
+ private FileStatus statInMemory(PathFragment path, FollowMode followMode) {
+ if (path.startsWith(execRoot)) {
+ var execPath = path.relativeTo(execRoot);
+ var metadata = inputArtifactData.getMetadata(execPath);
+ if (metadata != null) {
+ return statFromMetadata(metadata);
+ }
+ }
+
+ return remoteOutputTree.statNullable(
+ path, /* followSymlinks= */ followMode == FollowMode.FOLLOW_ALL);
+ }
+
+ private static FileStatusWithMetadata statFromMetadata(FileArtifactValue m) {
return new FileStatusWithMetadata() {
@Override
public byte[] getDigest() {
@@ -567,7 +615,7 @@ public long getNodeId() {
}
@Override
- public RemoteFileArtifactValue getMetadata() {
+ public FileArtifactValue getMetadata() {
return m;
}
};
@@ -597,48 +645,6 @@ FileArtifactValue getInputMetadata(ActionInput input) {
return inputArtifactData.getMetadata(execPath);
}
- @Nullable
- private FileArtifactValue getMetadataByExecPath(PathFragment execPath) {
- FileArtifactValue m = inputArtifactData.getMetadata(execPath);
- if (m != null) {
- return m;
- }
-
- var stat =
- remoteOutputTree.statNullable(execRoot.getRelative(execPath), /* followSymlinks= */ true);
- if (stat instanceof FileStatusWithMetadata) {
- return ((FileStatusWithMetadata) stat).getMetadata();
- }
-
- return null;
- }
-
- @Nullable
- private RemoteFileArtifactValue getRemoteMetadata(PathFragment path) {
- if (!isOutput(path)) {
- return null;
- }
- FileArtifactValue m = getMetadataByExecPath(path.relativeTo(execRoot));
- if (m != null && m.isRemote()) {
- return (RemoteFileArtifactValue) m;
- }
- return null;
- }
-
- @Nullable
- private TreeArtifactValue getRemoteTreeMetadata(PathFragment path) {
- if (!isOutput(path)) {
- return null;
- }
- TreeArtifactValue m = inputArtifactData.getTreeMetadata(path.relativeTo(execRoot));
- // TODO: Handle partially remote tree artifacts.
- if (m != null && m.isEntirelyRemote()) {
- return m;
- }
- // TODO(tjgq): Synthesize TreeArtifactValue from remoteOutputTree.
- return null;
- }
-
private void downloadFileIfRemote(PathFragment path) throws IOException {
if (!isRemote(path)) {
return;
@@ -747,12 +753,12 @@ protected Collection readdir(PathFragment path, boolean followSymlinks)
boolean existsRemotely = false;
+ path = resolveSymbolicLinks(path).asFragment();
+
if (isOutput(path)) {
try {
- for (var entry :
- remoteOutputTree
- .getPath(path)
- .readdir(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW)) {
+ for (var entry : remoteOutputTree.getPath(path).readdir(Symlinks.NOFOLLOW)) {
+ entry = maybeFollowSymlinkForDirent(path, entry, followSymlinks);
entries.put(entry.getName(), entry);
}
existsRemotely = true;
@@ -762,8 +768,8 @@ protected Collection readdir(PathFragment path, boolean followSymlinks)
}
try {
- for (var entry :
- localFs.getPath(path).readdir(followSymlinks ? Symlinks.FOLLOW : Symlinks.NOFOLLOW)) {
+ for (var entry : localFs.getPath(path).readdir(Symlinks.NOFOLLOW)) {
+ entry = maybeFollowSymlinkForDirent(path, entry, followSymlinks);
entries.put(entry.getName(), entry);
}
} catch (FileNotFoundException e) {
@@ -776,6 +782,19 @@ protected Collection readdir(PathFragment path, boolean followSymlinks)
return ImmutableList.sortedCopyOf(entries.values());
}
+ private Dirent maybeFollowSymlinkForDirent(
+ PathFragment dirPath, Dirent entry, boolean followSymlinks) {
+ if (!followSymlinks || !entry.getType().equals(Dirent.Type.SYMLINK)) {
+ return entry;
+ }
+ PathFragment path = dirPath.getChild(entry.getName());
+ FileStatus st = statNullable(path, /* followSymlinks= */ true);
+ if (st == null) {
+ return new Dirent(entry.getName(), Dirent.Type.UNKNOWN);
+ }
+ return new Dirent(entry.getName(), direntFromStat(st));
+ }
+
/*
* -------------------- TODO(buchgr): Not yet implemented --------------------
*
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
index 4d82a88c60191c..fbc44c59550bff 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -15,9 +15,9 @@
package com.google.devtools.build.lib.remote;
import static com.google.common.base.Preconditions.checkNotNull;
-import static com.google.devtools.build.lib.profiler.ProfilerTask.PROCESS_TIME;
import static com.google.devtools.build.lib.profiler.ProfilerTask.REMOTE_DOWNLOAD;
import static com.google.devtools.build.lib.profiler.ProfilerTask.REMOTE_EXECUTION;
+import static com.google.devtools.build.lib.profiler.ProfilerTask.REMOTE_PROCESS_TIME;
import static com.google.devtools.build.lib.profiler.ProfilerTask.REMOTE_QUEUE;
import static com.google.devtools.build.lib.profiler.ProfilerTask.REMOTE_SETUP;
import static com.google.devtools.build.lib.profiler.ProfilerTask.UPLOAD_TIME;
@@ -342,7 +342,7 @@ private static void profileAccounting(ExecutedActionMetadata executedActionMetad
converter,
executedActionMetadata.getExecutionStartTimestamp(),
executedActionMetadata.getExecutionCompletedTimestamp(),
- PROCESS_TIME,
+ REMOTE_PROCESS_TIME,
"execute");
logProfileTask(
converter,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
index e96de8ac69c50e..ede028f3a25ce5 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
@@ -1330,7 +1330,8 @@ public ImmutableList getDexoptsSupportedInDexMerger() {
return dexoptsSupportedInDexMerger;
}
- /** dx flags supported in dexmerger actions. */
+ /** dx flags supported in dexsharder actions. */
+ @Override
public ImmutableList getDexoptsSupportedInDexSharder() {
return dexoptsSupportedInDexSharder;
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
index 1e4ca887ad431e..81c0f0bbf3940d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
@@ -18,7 +18,6 @@
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
-import com.google.common.base.Strings;
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
@@ -39,7 +38,6 @@
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
-import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.build.lib.rules.cpp.CcCompilationHelper.SourceCategory;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.CollidingProvidesException;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
@@ -53,7 +51,6 @@
import java.util.Map;
import java.util.Set;
import java.util.regex.Pattern;
-import java.util.regex.PatternSyntaxException;
import java.util.stream.Stream;
import javax.annotation.Nullable;
import net.starlark.java.annot.StarlarkMethod;
@@ -369,34 +366,6 @@ public boolean passesFilter(String flag) {
}
}
- @Nullable
- private static Pattern getNoCoptsPattern(RuleContext ruleContext) throws InterruptedException {
- if (!ruleContext.getRule().isAttrDefined(NO_COPTS_ATTRIBUTE, Type.STRING)) {
- return null;
- }
- String nocoptsValue = ruleContext.attributes().get(NO_COPTS_ATTRIBUTE, Type.STRING);
- if (Strings.isNullOrEmpty(nocoptsValue)) {
- return null;
- }
-
- if (ruleContext.getConfiguration().getFragment(CppConfiguration.class).disableNoCopts()) {
- ruleContext.attributeError(
- NO_COPTS_ATTRIBUTE,
- "This attribute was removed. See https://github.com/bazelbuild/bazel/issues/8706 for"
- + " details.");
- }
-
- String nocoptsAttr = ruleContext.getExpander().expand(NO_COPTS_ATTRIBUTE, nocoptsValue);
- try {
- return Pattern.compile(nocoptsAttr);
- } catch (PatternSyntaxException e) {
- ruleContext.attributeError(
- NO_COPTS_ATTRIBUTE,
- "invalid regular expression '" + nocoptsAttr + "': " + e.getMessage());
- return null;
- }
- }
-
private static final String DEFINES_ATTRIBUTE = "defines";
/**
@@ -753,6 +722,11 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException(
// Support implicit enabling of FSAFDO for AFDO unless it has been disabled.
if (!allUnsupportedFeatures.contains(CppRuleClasses.FSAFDO)) {
allFeatures.add(CppRuleClasses.ENABLE_FSAFDO);
+ // Support implicit enabling of MFS for FSAFDO unless it has been disabled.
+ // We are reusing the "ENABLE_FDO_SPLIT_FUNCTIONS" feature here.
+ if (!allUnsupportedFeatures.contains(CppRuleClasses.SPLIT_FUNCTIONS)) {
+ allFeatures.add(CppRuleClasses.ENABLE_FDO_SPLIT_FUNCTIONS);
+ }
}
}
if (branchFdoProvider.isAutoXBinaryFdo()) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java
index 472f20145f5d6f..46dabe1177004a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java
@@ -154,10 +154,10 @@ public JavaConfiguration(BuildOptions buildOptions) throws InvalidConfigurationE
this.disallowJavaImportEmptyJars = javaOptions.disallowJavaImportEmptyJars;
Map optimizers = javaOptions.bytecodeOptimizers;
- if (optimizers.size() > 1) {
+ if (optimizers.size() != 1) {
throw new InvalidConfigurationException(
String.format(
- "--experimental_bytecode_optimizers can only accept up to one mapping, but %d"
+ "--experimental_bytecode_optimizers can only accept exactly one mapping, but %d"
+ " mappings were provided.",
optimizers.size()));
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/GeneratedExtensionRegistryProvider.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/GeneratedExtensionRegistryProvider.java
index ef0386692d7f0f..f908f75e3fb88e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/GeneratedExtensionRegistryProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/GeneratedExtensionRegistryProvider.java
@@ -158,10 +158,6 @@ private Provider() {
super(NAME, GeneratedExtensionRegistryProvider.class);
}
- public String getName() {
- return NAME;
- }
-
@Override
public GeneratedExtensionRegistryProvider create(
Label generatingRuleLabel,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStarlarkCommon.java b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStarlarkCommon.java
index 4db5b02d96ebb7..a149f17500ca80 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStarlarkCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStarlarkCommon.java
@@ -252,7 +252,7 @@ public AppleExecutableBinaryInfo newExecutableBinaryProvider(
}
private Dict asDict(Object o) {
- return o == Starlark.UNBOUND ? Dict.empty() : (Dict) o;
+ return o == Starlark.NONE ? Dict.empty() : (Dict) o;
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
index 44129690677141..9657df3e263d68 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
@@ -360,18 +360,16 @@ public String getTypeDescription() {
+ " by 4 GCs with zero second pause")
public MemoryProfileStableHeapParameters memoryProfileStableHeapParameters;
-
@Option(
name = "heap_dump_on_oom",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.LOGGING,
effectTags = {OptionEffectTag.BAZEL_MONITORING},
help =
- "Whether to manually output a heap dump if an OOM is thrown (including OOMs due to"
- + " --experimental_oom_more_eagerly_threshold). The dump will be written to"
+ "Whether to manually output a heap dump if an OOM is thrown (including manual OOMs due to"
+ + " reaching --gc_thrashing_limits). The dump will be written to"
+ " /.heapdump.hprof. This option effectively replaces"
- + " -XX:+HeapDumpOnOutOfMemoryError, which has no effect because OOMs are caught and"
- + " redirected to Runtime#halt.")
+ + " -XX:+HeapDumpOnOutOfMemoryError, which has no effect for manual OOMs.")
public boolean heapDumpOnOom;
@Option(
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/GcThrashingDetector.java b/src/main/java/com/google/devtools/build/lib/runtime/GcThrashingDetector.java
index 5c6483922fc7f9..68d69890615462 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/GcThrashingDetector.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/GcThrashingDetector.java
@@ -66,12 +66,12 @@ static Limit of(Duration period, int count) {
/** If enabled in {@link MemoryPressureOptions}, creates a {@link GcThrashingDetector}. */
@Nullable
static GcThrashingDetector createForCommand(MemoryPressureOptions options) {
- if (options.gcThrashingLimits.isEmpty() || options.oomMoreEagerlyThreshold == 100) {
+ if (options.gcThrashingLimits.isEmpty() || options.gcThrashingThreshold == 100) {
return null;
}
return new GcThrashingDetector(
- options.oomMoreEagerlyThreshold,
+ options.gcThrashingThreshold,
options.gcThrashingLimits,
BlazeClock.instance(),
BugReporter.defaultInstance());
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureListener.java b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureListener.java
index b85eeff7bad084..500397882b92aa 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureListener.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureListener.java
@@ -42,19 +42,16 @@
final class MemoryPressureListener implements NotificationListener {
private final AtomicReference eventBus = new AtomicReference<>();
- private final RetainedHeapLimiter retainedHeapLimiter;
private final AtomicReference gcThrashingDetector = new AtomicReference<>();
private final Executor executor;
- private MemoryPressureListener(RetainedHeapLimiter retainedHeapLimiter, Executor executor) {
- this.retainedHeapLimiter = retainedHeapLimiter;
+ private MemoryPressureListener(Executor executor) {
this.executor = executor;
}
- static MemoryPressureListener create(RetainedHeapLimiter retainedHeapLimiter) {
+ static MemoryPressureListener create() {
return createFromBeans(
ManagementFactory.getGarbageCollectorMXBeans(),
- retainedHeapLimiter,
// Use a dedicated thread to broadcast memory pressure events. The service thread that calls
// handleNotification for GC events is not a typical Java thread - it doesn't work with
// debugger breakpoints and may not show up in thread dumps.
@@ -65,7 +62,6 @@ static MemoryPressureListener create(RetainedHeapLimiter retainedHeapLimiter) {
@VisibleForTesting
static MemoryPressureListener createFromBeans(
List gcBeans,
- RetainedHeapLimiter retainedHeapLimiter,
Executor executor) {
ImmutableList tenuredGcEmitters = findTenuredCollectorBeans(gcBeans);
if (tenuredGcEmitters.isEmpty()) {
@@ -79,8 +75,7 @@ static MemoryPressureListener createFromBeans(
"Unable to find tenured collector from %s: names were %s.", gcBeans, names));
}
- MemoryPressureListener memoryPressureListener =
- new MemoryPressureListener(retainedHeapLimiter, executor);
+ MemoryPressureListener memoryPressureListener = new MemoryPressureListener(executor);
tenuredGcEmitters.forEach(e -> e.addNotificationListener(memoryPressureListener, null, null));
return memoryPressureListener;
}
@@ -150,14 +145,10 @@ private void broadcast(MemoryPressureEvent event) {
}
// A null EventBus implies memory pressure event between commands with no active EventBus.
- // In such cases, notify RetainedHeapLimiter but do not publish event.
EventBus eventBus = this.eventBus.get();
if (eventBus != null) {
eventBus.post(event);
}
- // Post to EventBus first so memory pressure subscribers have a chance to make things
- // eligible for GC before RetainedHeapLimiter would trigger a full GC.
- this.retainedHeapLimiter.handle(event);
}
void setEventBus(@Nullable EventBus eventBus) {
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureModule.java b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureModule.java
index 0ba6a8bd0f3aa7..d1850ca6173b9f 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureModule.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureModule.java
@@ -17,7 +17,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.eventbus.EventBus;
import com.google.common.eventbus.Subscribe;
-import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.runtime.MemoryPressure.MemoryPressureStats;
import com.google.devtools.build.lib.skyframe.HighWaterMarkLimiter;
import com.google.devtools.common.options.OptionsBase;
@@ -28,18 +27,10 @@
* pressure events.
*/
public final class MemoryPressureModule extends BlazeModule {
- private RetainedHeapLimiter retainedHeapLimiter;
- private MemoryPressureListener memoryPressureListener;
+ private final MemoryPressureListener memoryPressureListener = MemoryPressureListener.create();
private HighWaterMarkLimiter highWaterMarkLimiter;
private EventBus eventBus;
- @Override
- public void workspaceInit(
- BlazeRuntime runtime, BlazeDirectories directories, WorkspaceBuilder builder) {
- retainedHeapLimiter = RetainedHeapLimiter.create(runtime.getBugReporter());
- memoryPressureListener = MemoryPressureListener.create(retainedHeapLimiter);
- }
-
@Override
public ImmutableList> getCommandOptions(Command command) {
return ImmutableList.of(MemoryPressureOptions.class);
@@ -54,7 +45,6 @@ public void beforeCommand(CommandEnvironment env) {
highWaterMarkLimiter =
new HighWaterMarkLimiter(env.getSkyframeExecutor(), env.getSyscallCache(), options);
memoryPressureListener.setGcThrashingDetector(GcThrashingDetector.createForCommand(options));
- retainedHeapLimiter.setOptions(options);
eventBus.register(this);
eventBus.register(highWaterMarkLimiter);
@@ -77,7 +67,6 @@ void onCrash(@SuppressWarnings("unused") CrashEvent event) {
private void postStats() {
MemoryPressureStats.Builder stats = MemoryPressureStats.newBuilder();
- retainedHeapLimiter.addStatsAndReset(stats);
highWaterMarkLimiter.addStatsAndReset(stats);
eventBus.post(stats.build());
}
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureOptions.java
index aa7f138a8b9f29..d9222f746e67aa 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/MemoryPressureOptions.java
@@ -29,25 +29,6 @@
/** Options for responding to memory pressure. */
public final class MemoryPressureOptions extends OptionsBase {
- @Option(
- name = "experimental_oom_more_eagerly_threshold",
- defaultValue = "100",
- documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
- effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS},
- converter = PercentageConverter.class,
- help =
- "If this flag is set to a value less than 100, Bazel will OOM if, after two full GC's, "
- + "more than this percentage of the (old gen) heap is still occupied.")
- public int oomMoreEagerlyThreshold;
-
- @Option(
- name = "min_time_between_triggered_gc",
- defaultValue = "1m",
- documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
- effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
- help = "The minimum amount of time between GCs triggered by RetainedHeapLimiter.")
- public Duration minTimeBetweenTriggeredGc;
-
@Option(
name = "skyframe_high_water_mark_threshold",
defaultValue = "85",
@@ -97,30 +78,35 @@ public final class MemoryPressureOptions extends OptionsBase {
public int skyframeHighWaterMarkFullGcDropsPerInvocation;
@Option(
- name = "experimental_gc_thrashing_limits",
- defaultValue = "",
+ name = "gc_thrashing_limits",
+ oldName = "experimental_gc_thrashing_limits",
+ oldNameWarning = false,
+ defaultValue = "1s:2,20s:3,1m:5",
documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION,
effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS},
converter = GcThrashingLimitsConverter.class,
help =
"Limits which, if reached, cause GcThrashingDetector to crash Bazel with an OOM. Each"
+ " limit is specified as : where period is a duration and count is a"
- + " positive integer. If more than --experimental_oom_more_eagerly_threshold percent"
- + " of tenured space (old gen heap) remains occupied after consecutive full"
- + " GCs within , an OOM is triggered. Multiple limits can be specified"
- + " separated by commas.")
+ + " positive integer. If more than --gc_thrashing_threshold percent of tenured space"
+ + " (old gen heap) remains occupied after consecutive full GCs within"
+ + " , an OOM is triggered. Multiple limits can be specified separated by"
+ + " commas.")
public ImmutableList gcThrashingLimits;
@Option(
- name = "gc_thrashing_limits_retained_heap_limiter_mutually_exclusive",
- defaultValue = "true",
- documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION,
+ name = "gc_thrashing_threshold",
+ oldName = "experimental_oom_more_eagerly_threshold",
+ oldNameWarning = false,
+ defaultValue = "100",
+ documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
effectTags = {OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS},
+ converter = PercentageConverter.class,
help =
- "If true, specifying non-empty --experimental_gc_thrashing_limits deactivates"
- + " RetainedHeapLimiter to make it mutually exclusive with GcThrashingDetector."
- + " Setting to false permits both to be active for the same command.")
- public boolean gcThrashingLimitsRetainedHeapLimiterMutuallyExclusive;
+ "The percent of tenured space occupied (0-100) above which GcThrashingDetector considers"
+ + " memory pressure events against its limits (--gc_thrashing_limits). If set to 100,"
+ + " GcThrashingDetector is disabled.")
+ public int gcThrashingThreshold;
static final class NonNegativeIntegerConverter extends RangeConverter {
NonNegativeIntegerConverter() {
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiter.java b/src/main/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiter.java
deleted file mode 100644
index a59b54e5fb02ea..00000000000000
--- a/src/main/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiter.java
+++ /dev/null
@@ -1,183 +0,0 @@
-// Copyright 2016 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.runtime;
-
-import static com.google.common.base.Preconditions.checkNotNull;
-
-import com.google.common.annotations.VisibleForTesting;
-import com.google.common.flogger.GoogleLogger;
-import com.google.devtools.build.lib.bugreport.BugReporter;
-import com.google.devtools.build.lib.bugreport.Crash;
-import com.google.devtools.build.lib.bugreport.CrashContext;
-import com.google.devtools.build.lib.clock.BlazeClock;
-import com.google.devtools.build.lib.clock.Clock;
-import com.google.devtools.build.lib.concurrent.ThreadSafety;
-import com.google.devtools.build.lib.runtime.MemoryPressure.MemoryPressureStats;
-import com.google.devtools.common.options.Options;
-import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.atomic.AtomicInteger;
-import java.util.concurrent.atomic.AtomicLong;
-
-/**
- * Monitors the size of the retained heap and exit promptly if it grows too large.
- *
- * Specifically, checks the size of the tenured space after each major GC; if it exceeds {@link
- * MemoryPressureOptions#oomMoreEagerlyThreshold}%, call {@link System#gc()} to trigger a
- * stop-the-world collection; if it's still more than {@link
- * MemoryPressureOptions#oomMoreEagerlyThreshold}% full, exit with an {@link OutOfMemoryError}.
- */
-final class RetainedHeapLimiter implements MemoryPressureStatCollector {
-
- private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
-
- private final BugReporter bugReporter;
- private final Clock clock;
-
- private volatile MemoryPressureOptions options = inactiveOptions();
-
- private final AtomicBoolean throwingOom = new AtomicBoolean(false);
- private final AtomicBoolean heapLimiterTriggeredGc = new AtomicBoolean(false);
- private final AtomicInteger consecutiveIgnoredFullGcsOverThreshold = new AtomicInteger(0);
- private final AtomicBoolean loggedIgnoreWarningSinceLastGc = new AtomicBoolean(false);
- private final AtomicLong lastTriggeredGcMillis = new AtomicLong();
- private final AtomicInteger gcsTriggered = new AtomicInteger(0);
- private final AtomicInteger maxConsecutiveIgnoredFullGcsOverThreshold = new AtomicInteger(0);
-
- static RetainedHeapLimiter create(BugReporter bugReporter) {
- return new RetainedHeapLimiter(bugReporter, BlazeClock.instance());
- }
-
- @VisibleForTesting
- static RetainedHeapLimiter createForTest(BugReporter bugReporter, Clock clock) {
- return new RetainedHeapLimiter(bugReporter, clock);
- }
-
- private RetainedHeapLimiter(BugReporter bugReporter, Clock clock) {
- this.bugReporter = checkNotNull(bugReporter);
- this.clock = checkNotNull(clock);
- }
-
- @ThreadSafety.ThreadCompatible // Can only be called on the logical main Bazel thread.
- void setOptions(MemoryPressureOptions options) {
- if (options.gcThrashingLimitsRetainedHeapLimiterMutuallyExclusive
- && !options.gcThrashingLimits.isEmpty()) {
- this.options = inactiveOptions();
- } else {
- this.options = options;
- }
- }
-
- // Can be called concurrently, handles concurrent calls with #setThreshold gracefully.
- @ThreadSafety.ThreadSafe
- public void handle(MemoryPressureEvent event) {
- if (throwingOom.get()) {
- return; // Do nothing if a crash is already in progress.
- }
-
- boolean wasHeapLimiterTriggeredGc = false;
- boolean wasGcLockerDeferredHeapLimiterTriggeredGc = false;
- if (event.wasManualGc()) {
- wasHeapLimiterTriggeredGc = heapLimiterTriggeredGc.getAndSet(false);
- if (!wasHeapLimiterTriggeredGc) {
- // This was a manually triggered GC, but not from us earlier: short-circuit.
- logger.atInfo().log("Ignoring manual GC from other source");
- return;
- }
- } else if (event.wasGcLockerInitiatedGc() && heapLimiterTriggeredGc.getAndSet(false)) {
- // If System.gc() is called was while there are JNI thread(s) in the critical region, GCLocker
- // defers the GC until those threads exit the critical region. However, all GCLocker initiated
- // GCs are minor evacuation pauses, so we won't get the full GC we requested. Cancel the
- // timeout so we can attempt System.gc() again if we're still over the threshold. See full
- // explanation in b/263405096#comment14.
- logger.atWarning().log(
- "Observed a GCLocker initiated GC without observing a manual GC since the last call to"
- + " System.gc(), cancelling timeout to permit a retry");
- wasGcLockerDeferredHeapLimiterTriggeredGc = true;
- lastTriggeredGcMillis.set(0);
- }
-
- // Get a local reference to guard against concurrent modifications.
- MemoryPressureOptions options = this.options;
- int threshold = options.oomMoreEagerlyThreshold;
-
- if (threshold == 100) {
- return; // Inactive.
- }
-
- int actual = event.percentTenuredSpaceUsed();
- if (actual < threshold) {
- if (wasHeapLimiterTriggeredGc || wasGcLockerDeferredHeapLimiterTriggeredGc) {
- logger.atInfo().log("Back under threshold (%s%% of tenured space)", actual);
- }
- consecutiveIgnoredFullGcsOverThreshold.set(0);
- return;
- }
-
- if (wasHeapLimiterTriggeredGc) {
- if (!throwingOom.getAndSet(true)) {
- // We got here from a GC initiated by the other branch.
- OutOfMemoryError oom =
- new OutOfMemoryError(
- String.format(
- "RetainedHeapLimiter forcing exit due to GC thrashing: After back-to-back full"
- + " GCs, the tenured space is more than %s%% occupied (%s out of a tenured"
- + " space size of %s).",
- threshold, event.tenuredSpaceUsedBytes(), event.tenuredSpaceMaxBytes()));
- logger.atInfo().log("Calling handleCrash");
- // Exits the runtime.
- bugReporter.handleCrash(Crash.from(oom), CrashContext.halt());
- }
- } else if (clock.currentTimeMillis() - lastTriggeredGcMillis.get()
- > options.minTimeBetweenTriggeredGc.toMillis()) {
- logger.atInfo().log(
- "Triggering a full GC (%s%% of tenured space after %s GC)",
- actual, event.wasFullGc() ? "full" : "minor");
- heapLimiterTriggeredGc.set(true);
- gcsTriggered.incrementAndGet();
- // Force a full stop-the-world GC and see if it can get us below the threshold.
- System.gc();
- lastTriggeredGcMillis.set(clock.currentTimeMillis());
- consecutiveIgnoredFullGcsOverThreshold.set(0);
- loggedIgnoreWarningSinceLastGc.set(false);
- } else if (event.wasFullGc()) {
- int consecutiveIgnored = consecutiveIgnoredFullGcsOverThreshold.incrementAndGet();
- maxConsecutiveIgnoredFullGcsOverThreshold.accumulateAndGet(consecutiveIgnored, Math::max);
- logger.atWarning().log(
- "Ignoring possible GC thrashing x%s (%s%% of tenured space after full GC) because of"
- + " recently triggered GC",
- consecutiveIgnored, actual);
- } else if (!loggedIgnoreWarningSinceLastGc.getAndSet(true)) {
- logger.atWarning().log(
- "Ignoring possible GC thrashing (%s%% of tenured space after minor GC) because of"
- + " recently triggered GC",
- actual);
- }
- }
-
- @Override
- public void addStatsAndReset(MemoryPressureStats.Builder stats) {
- stats
- .setManuallyTriggeredGcs(gcsTriggered.getAndSet(0))
- .setMaxConsecutiveIgnoredGcsOverThreshold(
- maxConsecutiveIgnoredFullGcsOverThreshold.getAndSet(0));
- consecutiveIgnoredFullGcsOverThreshold.set(0);
- }
-
- private static MemoryPressureOptions inactiveOptions() {
- var options = Options.getDefaults(MemoryPressureOptions.class);
- options.oomMoreEagerlyThreshold = 100;
- return options;
- }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
index 1b991588db4a2b..188ec40acf8bf5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionArtifactCycleReporter.java
@@ -18,7 +18,6 @@
import com.google.devtools.build.lib.actions.ActionLookupData;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.collect.nestedset.ArtifactNestedSetKey;
import com.google.devtools.build.lib.pkgcache.PackageProvider;
import com.google.devtools.build.lib.skyframe.TestCompletionValue.TestCompletionKey;
import com.google.devtools.build.skyframe.CycleInfo;
@@ -50,6 +49,10 @@ protected String prettyPrint(Object untypedKey) {
return prettyPrint(key.functionName(), key.argument());
}
+ /**
+ * Should be kept consistent with {@link #ACTION_OR_ARTIFACT_OR_TRANSITIVE_RDEP} and {@link
+ * #shouldSkipOnPathToCycle}
+ */
private static String prettyPrint(SkyFunctionName skyFunctionName, Object arg) {
if (arg instanceof Artifact) {
return prettyPrintArtifact(((Artifact) arg));
@@ -77,7 +80,9 @@ private static String prettyPrintArtifact(Artifact artifact) {
@Override
protected boolean shouldSkipOnPathToCycle(SkyKey key) {
// BuildDriverKeys don't provide any relevant info for the end user.
- return SkyFunctions.BUILD_DRIVER.equals(key.functionName());
+ return SkyFunctions.BUILD_DRIVER.equals(key.functionName())
+ // ArtifactNestedSetKeys are just an implementation detail.
+ || SkyFunctions.ARTIFACT_NESTED_SET.equals(key.functionName());
}
@Override
@@ -108,6 +113,6 @@ protected boolean shouldSkipIntermediateKeyOnCycle(SkyKey key) {
// ArtifactNestedSetKey isn't worth reporting to the user - it is just an optimization, and will
// always be an intermediate member of a cycle. It may contain artifacts irrelevant to the
// cycle, and may be nested several layers deep.
- return key instanceof ArtifactNestedSetKey;
+ return SkyFunctions.ARTIFACT_NESTED_SET.equals(key.functionName());
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
index 502f742d6004c4..f1fa6db0a13ca9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -310,15 +310,15 @@ public SkyValue compute(SkyKey skyKey, Environment env)
starlarkExecTransition.orElse(null),
env,
env.getListener());
- if (depValueMap == null) {
- return null;
- }
if (!computeDependenciesState.transitiveRootCauses().isEmpty()) {
NestedSet causes = computeDependenciesState.transitiveRootCauses().build();
throw new AspectFunctionException(
new AspectCreationException(
"Loading failed", causes, getPrioritizedDetailedExitCode(causes)));
}
+ if (depValueMap == null) {
+ return null;
+ }
// Load the requested toolchains into the ToolchainContext, now that we have dependencies.
ToolchainCollection unloadedToolchainContexts =
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index 9982ba7b7a530c..64b14d683244ea 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -402,7 +402,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/actions:action_lookup_data",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/cmdline",
- "//src/main/java/com/google/devtools/build/lib/collect/nestedset:artifact_nested_set_key",
"//src/main/java/com/google/devtools/build/lib/pkgcache",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
@@ -3019,6 +3018,7 @@ java_library(
deps = [
":node_dropping_inconsistency_receiver",
":sky_functions",
+ "//src/main/java/com/google/devtools/build/lib/bugreport",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:graph_inconsistency_java_proto",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index 0557529677f4dd..9fccf1efebdb42 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -66,6 +66,7 @@
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiff;
import com.google.devtools.build.lib.analysis.config.ConfigConditions;
+import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
import com.google.devtools.build.lib.analysis.config.StarlarkTransitionCache;
import com.google.devtools.build.lib.analysis.test.AnalysisFailurePropagationException;
import com.google.devtools.build.lib.analysis.test.CoverageActionFinishedEvent;
@@ -89,6 +90,7 @@
import com.google.devtools.build.lib.packages.TargetUtils;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.SilentCloseable;
+import com.google.devtools.build.lib.server.FailureDetails;
import com.google.devtools.build.lib.skyframe.ArtifactConflictFinder.ConflictException;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.TopLevelAspectsKey;
@@ -279,7 +281,11 @@ private ImmutableSet getNativeCacheInvalidatingDifferences(
/** Sets the configuration. Not thread-safe. DO NOT CALL except from tests! */
@VisibleForTesting
public void setConfiguration(
- EventHandler eventHandler, BuildConfigurationValue configuration, int maxDifferencesToShow) {
+ EventHandler eventHandler,
+ BuildConfigurationValue configuration,
+ int maxDifferencesToShow,
+ boolean allowAnalysisCacheDiscards)
+ throws InvalidConfigurationException {
if (skyframeAnalysisWasDiscarded) {
eventHandler.handle(
Event.warn(
@@ -290,6 +296,12 @@ public void setConfiguration(
} else {
String diff = describeConfigurationDifference(configuration, maxDifferencesToShow);
if (diff != null) {
+ if (!allowAnalysisCacheDiscards) {
+ String message = String.format("%s, analysis cache would have been discarded.", diff);
+ throw new InvalidConfigurationException(
+ message,
+ FailureDetails.BuildConfiguration.Code.CONFIGURATION_DISCARDED_ANALYSIS_CACHE);
+ }
eventHandler.handle(
Event.warn(
diff
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java
index 4c3c15d50c484a..c25984e98f064f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkymeldInconsistencyReceiver.java
@@ -13,9 +13,9 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import static com.google.common.base.Preconditions.checkState;
import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.skyframe.GraphInconsistencyReceiver;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
@@ -30,7 +30,9 @@
public class SkymeldInconsistencyReceiver implements GraphInconsistencyReceiver {
private static final ImmutableMap
SKYMELD_EXPECTED_MISSING_CHILDREN =
- ImmutableMap.of(SkyFunctions.ACTION_EXECUTION, SkyFunctions.GLOB);
+ ImmutableMap.of(
+ SkyFunctions.ACTION_EXECUTION, SkyFunctions.GLOB,
+ SkyFunctions.GLOB, SkyFunctions.GLOB);
private final boolean heuristicallyDropNodes;
@@ -49,12 +51,12 @@ public void noteInconsistencyAndMaybeThrow(
return;
}
- checkState(
- NodeDroppingInconsistencyReceiver.isExpectedInconsistency(
- key, otherKeys, inconsistency, SKYMELD_EXPECTED_MISSING_CHILDREN),
- "Unexpected inconsistency: %s, %s, %s",
- key,
- otherKeys,
- inconsistency);
+ if (!NodeDroppingInconsistencyReceiver.isExpectedInconsistency(
+ key, otherKeys, inconsistency, SKYMELD_EXPECTED_MISSING_CHILDREN)) {
+ // Instead of crashing, simply send a bug report here so we can evaluate whether this is an
+ // actual bug or just something else to be added to the expected list.
+ BugReport.logUnexpected(
+ "Unexpected inconsistency: %s, %s, %s", key, otherKeys, inconsistency);
+ }
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java
index d30bf88a9922af..159822b31f51bc 100644
--- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java
+++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java
@@ -94,6 +94,13 @@ public interface AndroidConfigurationApi extends StarlarkValue {
documented = false)
ImmutableList getDexoptsSupportedInDexMerger();
+ @StarlarkMethod(
+ name = "get_dexopts_supported_in_dex_sharder",
+ structField = true,
+ doc = "",
+ documented = false)
+ ImmutableList getDexoptsSupportedInDexSharder();
+
@StarlarkMethod(
name = "get_target_dexopts_that_prevent_incremental_dexing",
structField = true,
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/objc/AppleCommonApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/objc/AppleCommonApi.java
index b09857d5695e58..8312046092b981 100644
--- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/objc/AppleCommonApi.java
+++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/objc/AppleCommonApi.java
@@ -420,8 +420,11 @@ AppleExecutableBinaryApi newExecutableBinaryProvider(
positional = false,
named = true,
documented = false,
- allowedTypes = {@ParamType(type = Dict.class)},
- defaultValue = "unbound"),
+ allowedTypes = {
+ @ParamType(type = Dict.class),
+ @ParamType(type = NoneType.class),
+ },
+ defaultValue = "None"),
},
useStarlarkThread = true)
// TODO(b/70937317): Iterate on, improve, and solidify this API.
diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkAttrModuleApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkAttrModuleApi.java
index 76ac8c3ea1930c..e1f94ce67c9cf3 100644
--- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkAttrModuleApi.java
+++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkAttrModuleApi.java
@@ -21,6 +21,7 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import net.starlark.java.eval.Dict;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Module;
@@ -30,9 +31,7 @@
import net.starlark.java.eval.StarlarkInt;
import net.starlark.java.eval.StarlarkThread;
-/**
- * Fake implementation of {@link StarlarkAttrModuleApi}.
- */
+/** Fake implementation of {@link StarlarkAttrModuleApi}. */
public class FakeStarlarkAttrModuleApi implements StarlarkAttrModuleApi {
@Override
@@ -44,11 +43,7 @@ public Descriptor intAttribute(
StarlarkThread thread)
throws EvalException {
return new FakeDescriptor(
- AttributeType.INT,
- Starlark.toJavaOptional(doc, String.class),
- mandatory,
- ImmutableList.of(),
- defaultInt);
+ AttributeType.INT, toTrimmedString(doc), mandatory, ImmutableList.of(), defaultInt);
}
@Override
@@ -61,7 +56,7 @@ public Descriptor stringAttribute(
throws EvalException {
return new FakeDescriptor(
AttributeType.STRING,
- Starlark.toJavaOptional(doc, String.class),
+ toTrimmedString(doc),
mandatory,
ImmutableList.of(),
defaultString != null ? "\"" + defaultString + "\"" : null);
@@ -87,11 +82,7 @@ public Descriptor labelAttribute(
allNameGroups = allProviderNameGroups(providers, thread);
}
return new FakeDescriptor(
- AttributeType.LABEL,
- Starlark.toJavaOptional(doc, String.class),
- mandatory,
- allNameGroups,
- defaultO);
+ AttributeType.LABEL, toTrimmedString(doc), mandatory, allNameGroups, defaultO);
}
@Override
@@ -100,7 +91,7 @@ public Descriptor stringListAttribute(
throws EvalException {
return new FakeDescriptor(
AttributeType.STRING_LIST,
- Starlark.toJavaOptional(doc, String.class),
+ toTrimmedString(doc),
mandatory,
ImmutableList.of(),
defaultList);
@@ -115,11 +106,7 @@ public Descriptor intListAttribute(
StarlarkThread thread)
throws EvalException {
return new FakeDescriptor(
- AttributeType.INT_LIST,
- Starlark.toJavaOptional(doc, String.class),
- mandatory,
- ImmutableList.of(),
- defaultList);
+ AttributeType.INT_LIST, toTrimmedString(doc), mandatory, ImmutableList.of(), defaultList);
}
@Override
@@ -141,11 +128,7 @@ public Descriptor labelListAttribute(
allNameGroups = allProviderNameGroups(providers, thread);
}
return new FakeDescriptor(
- AttributeType.LABEL_LIST,
- Starlark.toJavaOptional(doc, String.class),
- mandatory,
- allNameGroups,
- defaultList);
+ AttributeType.LABEL_LIST, toTrimmedString(doc), mandatory, allNameGroups, defaultList);
}
@Override
@@ -168,7 +151,7 @@ public Descriptor labelKeyedStringDictAttribute(
}
return new FakeDescriptor(
AttributeType.LABEL_STRING_DICT,
- Starlark.toJavaOptional(doc, String.class),
+ toTrimmedString(doc),
mandatory,
allNameGroups,
defaultList);
@@ -179,7 +162,7 @@ public Descriptor boolAttribute(
Boolean defaultO, Object doc, Boolean mandatory, StarlarkThread thread) throws EvalException {
return new FakeDescriptor(
AttributeType.BOOLEAN,
- Starlark.toJavaOptional(doc, String.class),
+ toTrimmedString(doc),
mandatory,
ImmutableList.of(),
Boolean.TRUE.equals(defaultO) ? "True" : "False");
@@ -189,11 +172,7 @@ public Descriptor boolAttribute(
public Descriptor outputAttribute(Object doc, Boolean mandatory, StarlarkThread thread)
throws EvalException {
return new FakeDescriptor(
- AttributeType.OUTPUT,
- Starlark.toJavaOptional(doc, String.class),
- mandatory,
- ImmutableList.of(),
- "");
+ AttributeType.OUTPUT, toTrimmedString(doc), mandatory, ImmutableList.of(), "");
}
@Override
@@ -201,11 +180,7 @@ public Descriptor outputListAttribute(
Boolean allowEmpty, Object doc, Boolean mandatory, StarlarkThread thread)
throws EvalException {
return new FakeDescriptor(
- AttributeType.OUTPUT_LIST,
- Starlark.toJavaOptional(doc, String.class),
- mandatory,
- ImmutableList.of(),
- "");
+ AttributeType.OUTPUT_LIST, toTrimmedString(doc), mandatory, ImmutableList.of(), "");
}
@Override
@@ -213,11 +188,7 @@ public Descriptor stringDictAttribute(
Boolean allowEmpty, Dict defaultO, Object doc, Boolean mandatory, StarlarkThread thread)
throws EvalException {
return new FakeDescriptor(
- AttributeType.STRING_DICT,
- Starlark.toJavaOptional(doc, String.class),
- mandatory,
- ImmutableList.of(),
- defaultO);
+ AttributeType.STRING_DICT, toTrimmedString(doc), mandatory, ImmutableList.of(), defaultO);
}
@Override
@@ -226,7 +197,7 @@ public Descriptor stringListDictAttribute(
throws EvalException {
return new FakeDescriptor(
AttributeType.STRING_LIST_DICT,
- Starlark.toJavaOptional(doc, String.class),
+ toTrimmedString(doc),
mandatory,
ImmutableList.of(),
defaultO);
@@ -236,11 +207,11 @@ public Descriptor stringListDictAttribute(
public Descriptor licenseAttribute(
Object defaultO, Object doc, Boolean mandatory, StarlarkThread thread) throws EvalException {
return new FakeDescriptor(
- AttributeType.STRING_LIST,
- Starlark.toJavaOptional(doc, String.class),
- mandatory,
- ImmutableList.of(),
- defaultO);
+ AttributeType.STRING_LIST, toTrimmedString(doc), mandatory, ImmutableList.of(), defaultO);
+ }
+
+ private static Optional toTrimmedString(Object doc) {
+ return Starlark.toJavaOptional(doc, String.class).map(Starlark::trimDocString);
}
@Override
diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java
index 1b8176e11711de..f3c3696e9bf34f 100644
--- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java
+++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java
@@ -106,8 +106,7 @@ public Object provider(Object doc, Object fields, Object init, StarlarkThread th
// fields is NONE, so there is no field information to add.
}
providerInfoList.add(
- forProviderInfo(
- fakeProvider, Starlark.toJavaOptional(doc, String.class), providerFieldInfos.build()));
+ forProviderInfo(fakeProvider, toTrimmedString(doc), providerFieldInfos.build()));
if (init == Starlark.NONE) {
return fakeProvider;
} else {
@@ -167,7 +166,7 @@ public StarlarkCallable rule(
// Only the Builder is passed to RuleInfoWrapper as the rule name is not yet available.
RuleInfo.Builder ruleInfo = RuleInfo.newBuilder().addAllAttribute(attrInfos);
- Starlark.toJavaOptional(doc, String.class).ifPresent(ruleInfo::setDocString);
+ toTrimmedString(doc).ifPresent(ruleInfo::setDocString);
Location loc = thread.getCallerLocation();
ruleInfoList.add(new RuleInfoWrapper(functionIdentifier, loc, ruleInfo));
@@ -231,7 +230,7 @@ public StarlarkAspectApi aspect(
// Only the Builder is passed to AspectInfoWrapper as the aspect name is not yet available.
AspectInfo.Builder aspectInfo =
AspectInfo.newBuilder().addAllAttribute(attrInfos).addAllAspectAttribute(aspectAttrs);
- Starlark.toJavaOptional(doc, String.class).ifPresent(aspectInfo::setDocString);
+ toTrimmedString(doc).ifPresent(aspectInfo::setDocString);
aspectInfoList.add(new AspectInfoWrapper(fakeAspect, thread.getCallerLocation(), aspectInfo));
@@ -279,4 +278,8 @@ public int compare(AttributeInfo o1, AttributeInfo o2) {
}
}
}
+
+ private static Optional toTrimmedString(Object doc) {
+ return Starlark.toJavaOptional(doc, String.class).map(Starlark::trimDocString);
+ }
}
diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java
index 668d9adfb14c67..3ab502b98fd455 100644
--- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java
+++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java
@@ -97,7 +97,9 @@ public StarlarkCallable repositoryRule(
// Only the Builder is passed to RuleInfoWrapper as the rule name is not yet available.
RuleInfo.Builder ruleInfo = RuleInfo.newBuilder().addAllAttribute(attrInfos);
- Starlark.toJavaOptional(doc, String.class).ifPresent(ruleInfo::setDocString);
+ Starlark.toJavaOptional(doc, String.class)
+ .map(Starlark::trimDocString)
+ .ifPresent(ruleInfo::setDocString);
Location loc = thread.getCallerLocation();
ruleInfoList.add(new RuleInfoWrapper(functionIdentifier, loc, ruleInfo));
return functionIdentifier;
diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractInMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/AbstractInMemoryMemoizingEvaluator.java
index 0a9215a1b5e440..aab1ab2eddad06 100644
--- a/src/main/java/com/google/devtools/build/skyframe/AbstractInMemoryMemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/AbstractInMemoryMemoizingEvaluator.java
@@ -161,6 +161,6 @@ private static String canonicalizeKey(SkyKey key) {
@Override
public void cleanupInterningPools() {
- getInMemoryGraph().cleanupInterningPool();
+ getInMemoryGraph().cleanupInterningPools();
}
}
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java
index e2849dbd991570..83d28c414acbaa 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraph.java
@@ -92,10 +92,12 @@ default int valuesSize() {
void removeIfDone(SkyKey key);
/**
- * Cleans up the {@link com.google.devtools.build.lib.concurrent.PooledInterner.Pool} by moving
- * instances back to weak interner and uninstall current pool.
+ * Cleans up {@linkplain com.google.devtools.build.lib.concurrent.PooledInterner.Pool interning
+ * pools} by moving objects to weak interners and uninstalling the current pools.
+ *
+ * May destroy this graph. Only call when the graph is about to be thrown away.
*/
- void cleanupInterningPool();
+ void cleanupInterningPools();
/**
* Returns the {@link InMemoryNodeEntry} for a given {@link SkyKey} if present in the graph.
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
index e1ddd3bacd922f..ac1baf5397afdb 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryGraphImpl.java
@@ -233,17 +233,9 @@ public void parallelForEach(Consumer consumer) {
nodeMap.forEachValue(PARALLELISM_THRESHOLD, consumer);
}
- /**
- * Re-interns {@link SkyKey} instances that use {@link SkyKeyInterner} in node map back to the
- * {@link SkyKeyInterner}'s weak interner.
- *
- * Also uninstalls current {@link SkyKeyPool} instance from being {@link SkyKeyInterner}'s
- * static global pool.
- */
@Override
- public void cleanupInterningPool() {
+ public void cleanupInterningPools() {
if (!usePooledInterning) {
- // No clean up is needed when `usePooledInterning` is false for shell integration tests.
return;
}
try (AutoProfiler ignored =
@@ -252,11 +244,13 @@ public void cleanupInterningPool() {
e -> {
weakInternSkyKey(e.getKey());
- if (!e.isDone() || !e.getKey().functionName().equals(SkyFunctions.PACKAGE)) {
- return;
+ if (e.isDone() && e.getKey().functionName().equals(SkyFunctions.PACKAGE)) {
+ weakInternPackageTargetsLabels((PackageValue) e.toValue());
}
- weakInternPackageTargetsLabels((PackageValue) e.toValue());
+ // The graph is about to be thrown away. Remove as we go to avoid temporarily storing
+ // everything in both the weak interner and the graph.
+ nodeMap.remove(e.getKey());
});
}
diff --git a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
index ccf1a15117686f..603a9ed7d62b52 100644
--- a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
@@ -203,8 +203,11 @@ public ProcessableGraph transform(ProcessableGraph graph) {
void dumpRdeps(PrintStream out, Predicate filter) throws InterruptedException;
/**
- * Cleans up the pool when {@link InMemoryGraph} serves as an alternative global pool to weak
- * interner for interning {@link SkyKey} and {@link com.google.devtools.build.lib.cmdline.Label}.
+ * Cleans up {@linkplain com.google.devtools.build.lib.concurrent.PooledInterner.Pool interning
+ * pools} by moving objects to weak interners and uninstalling the current pools.
+ *
+ * May destroy this evaluator's {@linkplain #getInMemoryGraph graph}. Only call when the graph
+ * is about to be thrown away.
*/
void cleanupInterningPools();
}
diff --git a/src/main/java/net/starlark/java/eval/Starlark.java b/src/main/java/net/starlark/java/eval/Starlark.java
index 545fb2816da974..722f37df51d43e 100644
--- a/src/main/java/net/starlark/java/eval/Starlark.java
+++ b/src/main/java/net/starlark/java/eval/Starlark.java
@@ -13,8 +13,12 @@
// limitations under the License.
package net.starlark.java.eval;
+import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Strings.isNullOrEmpty;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static java.lang.Math.min;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
@@ -540,6 +544,113 @@ public static String formatWithList(
return pr.toString();
}
+ /**
+ * Returns a Starlark doc string with each line trimmed and dedented to the minimal common
+ * indentation level (except for the first line, which is always fully trimmed), and with leading
+ * and trailing empty lines removed, following the PEP-257 algorithm. See
+ * https://peps.python.org/pep-0257/#handling-docstring-indentation
+ *
+ *
For whitespace trimming, we use the same definition of whitespace as the Starlark {@code
+ * string.strip} method.
+ *
+ *
Following PEP-257, we expand tabs in the doc string with tab size 8 before dedenting.
+ * Starlark does not use tabs for indentation, but Starlark string values may contain tabs, so we
+ * choose to expand them for consistency with Python.
+ *
+ *
The intent is to turn documentation strings like
+ *
+ *
+ * """Heading
+ *
+ * Details paragraph
+ * """
+ *
+ *
+ * and
+ *
+ *
+ * """
+ * Heading
+ *
+ * Details paragraph
+ * """
+ *
+ *
+ * into the desired "Heading\n\nDetails paragraph" form, and avoid the risk of documentation
+ * processors interpreting indented parts of the original string as special formatting (e.g. code
+ * blocks in the case of Markdown).
+ */
+ public static String trimDocString(String docString) {
+ ImmutableList lines = expandTabs(docString, 8).lines().collect(toImmutableList());
+ if (lines.isEmpty()) {
+ return "";
+ }
+ // First line is special: we fully strip it and ignore it for leading spaces calculation
+ String firstLineTrimmed = StringModule.INSTANCE.strip(lines.get(0), NONE);
+ Iterable subsequentLines = Iterables.skip(lines, 1);
+ int minLeadingSpaces = Integer.MAX_VALUE;
+ for (String line : subsequentLines) {
+ String strippedLeading = StringModule.INSTANCE.lstrip(line, NONE);
+ if (!strippedLeading.isEmpty()) {
+ int leadingSpaces = line.length() - strippedLeading.length();
+ minLeadingSpaces = min(leadingSpaces, minLeadingSpaces);
+ }
+ }
+ if (minLeadingSpaces == Integer.MAX_VALUE) {
+ minLeadingSpaces = 0;
+ }
+
+ StringBuilder result = new StringBuilder();
+ result.append(firstLineTrimmed);
+ for (String line : subsequentLines) {
+ // Length check ensures we ignore leading empty lines
+ if (result.length() > 0) {
+ result.append("\n");
+ }
+ if (line.length() > minLeadingSpaces) {
+ result.append(StringModule.INSTANCE.rstrip(line.substring(minLeadingSpaces), NONE));
+ }
+ }
+ // Remove trailing empty lines
+ return StringModule.INSTANCE.rstrip(result.toString(), NONE);
+ }
+
+ /**
+ * Expands tab characters to one or more spaces, producing the same indentation level at any given
+ * point on any given line as would be expected when rendering the string with a given tab size; a
+ * Java port of Python's {@code str.expandtabs}.
+ */
+ static String expandTabs(String line, int tabSize) {
+ if (!line.contains("\t")) {
+ // Don't alloc in the fast case.
+ return line;
+ }
+ checkArgument(tabSize > 0);
+ StringBuilder result = new StringBuilder();
+ int col = 0;
+ for (int i = 0; i < line.length(); i++) {
+ char c = line.charAt(i);
+ switch (c) {
+ case '\n':
+ case '\r':
+ result.append(c);
+ col = 0;
+ break;
+ case '\t':
+ int spaces = tabSize - col % tabSize;
+ for (int j = 0; j < spaces; j++) {
+ result.append(' ');
+ }
+ col += spaces;
+ break;
+ default:
+ result.append(c);
+ col++;
+ }
+ }
+ return result.toString();
+ }
+
/** Returns a slice of a sequence as if by the Starlark operation {@code x[start:stop:step]}. */
public static Object slice(
Mutability mu, Object x, Object startObj, Object stopObj, Object stepObj)
@@ -972,7 +1083,10 @@ public static Object execFileProgram(Program prog, Module module, StarlarkThread
int[] globalIndex = module.getIndicesOfGlobals(rfn.getGlobals());
if (module.getDocumentation() == null) {
- module.setDocumentation(rfn.getDocumentation());
+ String documentation = rfn.getDocumentation();
+ if (documentation != null) {
+ module.setDocumentation(Starlark.trimDocString(documentation));
+ }
}
StarlarkFunction toplevel =
diff --git a/src/main/java/net/starlark/java/eval/StarlarkFunction.java b/src/main/java/net/starlark/java/eval/StarlarkFunction.java
index 60f0af44390461..3136a3bc3e6de9 100644
--- a/src/main/java/net/starlark/java/eval/StarlarkFunction.java
+++ b/src/main/java/net/starlark/java/eval/StarlarkFunction.java
@@ -145,10 +145,14 @@ public String getName() {
return rfn.getName();
}
- /** Returns the value denoted by the function's doc string literal, or null if absent. */
+ /**
+ * Returns the value denoted by the function's doc string literal (trimmed if necessary), or null
+ * if absent.
+ */
@Nullable
public String getDocumentation() {
- return rfn.getDocumentation();
+ String documentation = rfn.getDocumentation();
+ return documentation != null ? Starlark.trimDocString(documentation) : null;
}
public Module getModule() {
diff --git a/src/main/java/net/starlark/java/eval/StringModule.java b/src/main/java/net/starlark/java/eval/StringModule.java
index b4cdc137fd9a09..302dbc1312b50c 100644
--- a/src/main/java/net/starlark/java/eval/StringModule.java
+++ b/src/main/java/net/starlark/java/eval/StringModule.java
@@ -184,9 +184,11 @@ public String upper(String self) {
* Note that this differs from Python 2.7, which uses ctype.h#isspace(), and from
* java.lang.Character#isWhitespace(), which does not recognize U+00A0.
*/
+ // TODO(https://github.com/bazelbuild/starlark/issues/112): use the Unicode definition of
+ // whitespace, matching Python 3.
private static final String LATIN1_WHITESPACE =
("\u0009" + "\n" + "\u000B" + "\u000C" + "\r" + "\u001C" + "\u001D" + "\u001E" + "\u001F"
- + "\u0020" + "\u0085" + "\u00A0");
+ + " " + "\u0085" + "\u00A0");
private static String stringLStrip(String self, String chars) {
CharMatcher matcher = CharMatcher.anyOf(chars);
diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto
index 17d313bce846b3..ca793a22f2eb0e 100644
--- a/src/main/protobuf/failure_details.proto
+++ b/src/main/protobuf/failure_details.proto
@@ -608,6 +608,7 @@ message BuildConfiguration {
// possibilities in Bazel, so we go with the more straightforward
// command-line error exit code 2.
INVALID_OUTPUT_DIRECTORY_MNEMONIC = 11 [(metadata) = { exit_code: 2 }];
+ CONFIGURATION_DISCARDED_ANALYSIS_CACHE = 12 [(metadata) = { exit_code: 2 }];
}
Code code = 1;
@@ -1197,6 +1198,7 @@ message Analysis {
CONFIGURED_VALUE_CREATION_FAILED = 18 [(metadata) = { exit_code: 1 }];
INCOMPATIBLE_TARGET_REQUESTED = 19 [(metadata) = { exit_code: 1 }];
ANALYSIS_FAILURE_PROPAGATION_FAILED = 20 [(metadata) = { exit_code: 1 }];
+ ANALYSIS_CACHE_DISCARDED = 21 [(metadata) = { exit_code: 1 }];
}
Code code = 1;
diff --git a/src/main/protobuf/memory_pressure.proto b/src/main/protobuf/memory_pressure.proto
index 97a8d20669d36b..63ec9b72433383 100644
--- a/src/main/protobuf/memory_pressure.proto
+++ b/src/main/protobuf/memory_pressure.proto
@@ -20,11 +20,12 @@ option java_package = "com.google.devtools.build.lib.runtime";
// Statistics about memory pressure handling.
message MemoryPressureStats {
- // The number of calls to System.gc() made by RetainedHeapLimiter.
- int32 manually_triggered_gcs = 1;
- // The maximum number of consecutive full GC events over the threshold ignored
- // by RetainedHeapLimiter because it recently triggered a GC.
- int32 max_consecutive_ignored_gcs_over_threshold = 2;
+ // Deprecated: An artifact of RetainedHeapLimiter. GcThrashingDetector (its
+ // replacement) does not manually trigger GC.
+ int32 manually_triggered_gcs = 1 [deprecated = true];
+ // Deprecated: An artifact of RetainedHeapLimiter. GcThrashingDetector (its
+ // replacement) does not ignore any non-manual full GC.
+ int32 max_consecutive_ignored_gcs_over_threshold = 2 [deprecated = true];
// Number of times HighWaterMarkLimiter dropped caches after minor GCs.
int32 minor_gc_drops = 3;
// Number of times HighWaterMarkLimiter dropped caches after full GCs.
diff --git a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl b/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl
index 1a5ef91e1605c3..3f1a3b85cc2c67 100644
--- a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl
+++ b/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl
@@ -21,7 +21,7 @@ load(":common/paths.bzl", "paths")
load(":common/java/java_info.bzl", "JavaInfo")
def _bazel_java_binary_impl(ctx):
- return _bazel_base_binary_impl(ctx, is_test_rule_class = False)
+ return _bazel_base_binary_impl(ctx, is_test_rule_class = False) + helper.executable_providers(ctx)
def _bazel_java_test_impl(ctx):
return _bazel_base_binary_impl(ctx, is_test_rule_class = True) + helper.test_providers(ctx)
diff --git a/src/main/starlark/builtins_bzl/common/java/java_helper.bzl b/src/main/starlark/builtins_bzl/common/java/java_helper.bzl
index 7c751f4e8ff3fd..2895829093ca75 100644
--- a/src/main/starlark/builtins_bzl/common/java/java_helper.bzl
+++ b/src/main/starlark/builtins_bzl/common/java/java_helper.bzl
@@ -288,6 +288,11 @@ def _test_providers(ctx):
return test_providers
+def _executable_providers(ctx):
+ if ctx.attr.create_executable:
+ return [_builtins.toplevel.RunEnvironmentInfo(cc_helper.get_expanded_env(ctx, {}))]
+ return []
+
def _resource_mapper(file):
return "%s:%s" % (
file.path,
@@ -366,6 +371,7 @@ helper = struct(
runfiles_enabled = _runfiles_enabled,
get_test_support = _get_test_support,
test_providers = _test_providers,
+ executable_providers = _executable_providers,
create_single_jar = _create_single_jar,
shell_quote = _shell_quote,
)
diff --git a/src/main/starlark/builtins_bzl/common/java/java_info.bzl b/src/main/starlark/builtins_bzl/common/java/java_info.bzl
index eb6faa4c9a924d..289540177a41c8 100644
--- a/src/main/starlark/builtins_bzl/common/java/java_info.bzl
+++ b/src/main/starlark/builtins_bzl/common/java/java_info.bzl
@@ -47,6 +47,13 @@ _ModuleFlagsInfo = provider(
"add_opens": "(depset[str]) Add-Opens configuration.",
},
)
+_EMPTY_MODULE_FLAGS_INFO = _ModuleFlagsInfo(add_exports = depset(), add_opens = depset())
+
+def _create_module_flags_info(*, add_exports, add_opens):
+ if add_exports or add_opens:
+ return _ModuleFlagsInfo(add_exports = add_exports, add_opens = add_opens)
+ return _EMPTY_MODULE_FLAGS_INFO
+
_JavaRuleOutputJarsInfo = provider(
doc = "Deprecated: use java_info.java_outputs. Information about outputs of a Java rule.",
fields = {
@@ -159,7 +166,7 @@ def merge(
"transitive_source_jars": depset(transitive = transitive_source_jars),
"java_outputs": java_outputs,
"outputs": _JavaRuleOutputJarsInfo(jars = java_outputs, jdeps = None, native_headers = None),
- "module_flags_info": _ModuleFlagsInfo(
+ "module_flags_info": _create_module_flags_info(
add_exports = depset(transitive = add_exports),
add_opens = depset(transitive = add_opens),
),
@@ -207,7 +214,7 @@ def to_java_binary_info(java_info):
"runtime_output_jars": [],
"plugins": _EMPTY_PLUGIN_DATA,
"api_generating_plugins": _EMPTY_PLUGIN_DATA,
- "module_flags_info": _ModuleFlagsInfo(add_exports = depset(), add_opens = depset()),
+ "module_flags_info": _EMPTY_MODULE_FLAGS_INFO,
"_neverlink": False,
"_constraints": [],
"annotation_processing": java_info.annotation_processing,
@@ -450,7 +457,7 @@ def java_info_for_compilation(
),
# the JavaInfo constructor does not add flags from runtime_deps nor support
# adding this target's exports/opens
- module_flags_info = _ModuleFlagsInfo(
+ module_flags_info = _create_module_flags_info(
add_exports = depset(add_exports, transitive = [
dep.module_flags_info.add_exports
for dep in concatenated_deps.runtimedeps_exports_deps
@@ -726,7 +733,7 @@ def _javainfo_init(
for dep in deps + runtime_deps + exports
],
),
- module_flags_info = _ModuleFlagsInfo(
+ module_flags_info = _create_module_flags_info(
add_exports = depset(transitive = [
dep.module_flags_info.add_exports
for dep in concatenated_deps.deps_exports
@@ -810,6 +817,16 @@ _EMPTY_PLUGIN_DATA = _JavaPluginDataInfo(
processor_data = depset(),
)
+def _create_plugin_data_info(*, processor_classes, processor_jars, processor_data):
+ if processor_classes or processor_jars or processor_data:
+ return _JavaPluginDataInfo(
+ processor_classes = processor_classes,
+ processor_jars = processor_jars,
+ processor_data = processor_data,
+ )
+ else:
+ return _EMPTY_PLUGIN_DATA
+
def disable_plugin_info_annotation_processing(plugin_info):
"""Returns a copy of the provided JavaPluginInfo without annotation processing info
@@ -820,7 +837,7 @@ def disable_plugin_info_annotation_processing(plugin_info):
(JavaPluginInfo) a new, transformed instance.
"""
return _new_javaplugininfo(
- plugins = _JavaPluginDataInfo(
+ plugins = _create_plugin_data_info(
processor_classes = depset(order = "preorder"),
# Preserve the processor path, since it may contain Error Prone plugins
# which will be service-loaded by JavaBuilder.
@@ -862,7 +879,7 @@ def _has_plugin_data(plugin_data):
)
def _merge_plugin_data(datas):
- return _JavaPluginDataInfo(
+ return _create_plugin_data_info(
processor_classes = depset(transitive = [p.processor_classes for p in datas]),
processor_jars = depset(transitive = [p.processor_jars for p in datas]),
processor_data = depset(transitive = [p.processor_data for p in datas]),
@@ -896,7 +913,7 @@ def _javaplugininfo_init(
java_infos = merge(runtime_deps)
processor_data = data if type(data) == "depset" else depset(data)
- plugins = _JavaPluginDataInfo(
+ plugins = _create_plugin_data_info(
processor_classes = depset([processor_class]) if processor_class else depset(),
processor_jars = java_infos.transitive_runtime_jars,
processor_data = processor_data,
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
index 9f0633d6ae041f..e9bf53cce69aa8 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
@@ -412,6 +412,90 @@ public void getInputMetadata_treeFileUnderFile_fails() {
assertThrows(IllegalArgumentException.class, () -> map.getInputMetadata(child));
}
+ @Test
+ public void getTreeMetadataForPrefix_nonTree() {
+ ActionInput file = ActionInputHelper.fromPath("some/file");
+ map.put(file, TestMetadata.create(1), /* depOwner= */ null);
+
+ assertThat(map.getTreeMetadataForPrefix(file.getExecPath())).isNull();
+ assertThat(map.getTreeMetadataForPrefix(file.getExecPath().getParentDirectory())).isNull();
+ assertThat(map.getTreeMetadataForPrefix(file.getExecPath().getChild("under"))).isNull();
+ }
+
+ @Test
+ public void getTreeMetadataForPrefix_emptyTree() {
+ SpecialArtifact tree = createTreeArtifact("a/tree");
+ TreeArtifactValue treeValue = TreeArtifactValue.newBuilder(tree).build();
+ map.putTreeArtifact(tree, treeValue, /* depOwner= */ null);
+
+ assertThat(map.getTreeMetadataForPrefix(tree.getExecPath().getParentDirectory())).isNull();
+ assertThat(map.getTreeMetadataForPrefix(tree.getExecPath())).isEqualTo(treeValue);
+ assertThat(map.getTreeMetadataForPrefix(tree.getExecPath().getChild("under")))
+ .isEqualTo(treeValue);
+ }
+
+ @Test
+ public void getTreeMetadataForPrefix_nonEmptyTree() {
+ SpecialArtifact tree = createTreeArtifact("a/tree");
+ TreeFileArtifact child = TreeFileArtifact.createTreeOutput(tree, "some/child");
+ TreeArtifactValue treeValue =
+ TreeArtifactValue.newBuilder(tree).putChild(child, TestMetadata.create(1)).build();
+ map.putTreeArtifact(tree, treeValue, /* depOwner= */ null);
+
+ assertThat(map.getTreeMetadataForPrefix(tree.getExecPath().getParentDirectory())).isNull();
+ assertThat(map.getTreeMetadataForPrefix(tree.getExecPath())).isEqualTo(treeValue);
+ assertThat(map.getTreeMetadataForPrefix(child.getExecPath())).isEqualTo(treeValue);
+ assertThat(map.getTreeMetadataForPrefix(child.getExecPath().getParentDirectory()))
+ .isEqualTo(treeValue);
+ assertThat(map.getTreeMetadataForPrefix(child.getExecPath().getChild("under")))
+ .isEqualTo(treeValue);
+ }
+
+ @Test
+ public void getTreeMetadataForPrefix_treeWithNestedFile() {
+ SpecialArtifact tree = createTreeArtifact("a/tree");
+ TreeFileArtifact child = TreeFileArtifact.createTreeOutput(tree, "some/dir/child");
+ TreeArtifactValue treeValue =
+ TreeArtifactValue.newBuilder(tree).putChild(child, TestMetadata.create(1)).build();
+ map.putTreeArtifact(tree, treeValue, /* depOwner= */ null);
+ map.put(
+ ActionInputHelper.fromPath(child.getExecPath()),
+ TestMetadata.create(1),
+ /* depOwner= */ null);
+
+ assertThat(map.getTreeMetadataForPrefix(tree.getExecPath().getParentDirectory())).isNull();
+ assertThat(map.getTreeMetadataForPrefix(tree.getExecPath())).isEqualTo(treeValue);
+ assertThat(map.getTreeMetadataForPrefix(child.getExecPath())).isEqualTo(treeValue);
+ assertThat(map.getTreeMetadataForPrefix(child.getExecPath().getParentDirectory()))
+ .isEqualTo(treeValue);
+ assertThat(map.getTreeMetadataForPrefix(child.getExecPath().getChild("under")))
+ .isEqualTo(treeValue);
+ }
+
+ @Test
+ public void getTreeMetadataForPrefix_treeWithNestedTree() {
+ SpecialArtifact tree = createTreeArtifact("a/tree");
+ TreeFileArtifact child = TreeFileArtifact.createTreeOutput(tree, "some/dir/child");
+ TreeArtifactValue treeValue =
+ TreeArtifactValue.newBuilder(tree).putChild(child, TestMetadata.create(1)).build();
+ map.putTreeArtifact(tree, treeValue, /* depOwner= */ null);
+ SpecialArtifact nestedTree = createTreeArtifact("a/tree/some/dir");
+ TreeFileArtifact nestedChild = TreeFileArtifact.createTreeOutput(nestedTree, "child");
+ TreeArtifactValue nestedTreeValue =
+ TreeArtifactValue.newBuilder(nestedTree)
+ .putChild(nestedChild, TestMetadata.create(1))
+ .build();
+ map.putTreeArtifact(nestedTree, nestedTreeValue, /* depOwner= */ null);
+
+ assertThat(map.getTreeMetadataForPrefix(tree.getExecPath().getParentDirectory())).isNull();
+ assertThat(map.getTreeMetadataForPrefix(tree.getExecPath())).isEqualTo(treeValue);
+ assertThat(map.getTreeMetadataForPrefix(child.getExecPath())).isEqualTo(treeValue);
+ assertThat(map.getTreeMetadataForPrefix(child.getExecPath().getParentDirectory()))
+ .isEqualTo(treeValue);
+ assertThat(map.getTreeMetadataForPrefix(child.getExecPath().getChild("under")))
+ .isEqualTo(treeValue);
+ }
+
@Test
public void getters_missingTree_returnNull() {
map.putTreeArtifact(createTreeArtifact("tree"), TreeArtifactValue.empty(), /*depOwner=*/ null);
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
index b0140032937192..e19cadaadb8ba6 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
@@ -25,6 +25,7 @@
import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
import com.google.devtools.build.lib.analysis.config.Fragment;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
+import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
import com.google.devtools.build.lib.analysis.config.RequiresOptions;
import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
@@ -1163,4 +1164,16 @@ public void cacheClearMessageAfterDiscardAnalysisCacheBuildWithRelevantOptionCha
assertDoesNotContainEvent("Build option");
assertContainsEvent("discarding analysis cache");
}
+
+ @Test
+ public void throwsIfAnalysisCacheIsDiscardedWhenOptionSet() throws Exception {
+ setupDiffResetTesting();
+ scratch.file("test/BUILD", "load(':lib.bzl', 'normal_lib')", "normal_lib(name='top')");
+ useConfiguration("--definitely_relevant=old");
+ update("//test:top");
+ useConfiguration("--noallow_analysis_cache_discard", "--definitely_relevant=new");
+
+ Throwable t = assertThrows(InvalidConfigurationException.class, () -> update("//test:top"));
+ assertThat(t.getMessage().contains("analysis cache would have been discarded")).isTrue();
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
index 6dc075d31b40f1..28fc819af2663b 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
@@ -85,6 +85,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_registry",
+ "//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception",
"//src/main/java/com/google/devtools/build/lib/analysis:config/per_label_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/run_under",
"//src/main/java/com/google/devtools/build/lib/analysis:config/run_under_converter",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
index 95ca620a76b7fe..ae8a327c634475 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
@@ -258,7 +258,18 @@ public AnalysisResult update(
/** Sets the configuration. Not thread-safe. */
public void setConfigurationForTesting(
EventHandler eventHandler, BuildConfigurationValue configuration) {
- skyframeBuildView.setConfiguration(eventHandler, configuration, /* maxDifferencesToShow= */ -1);
+ try {
+ skyframeBuildView.setConfiguration(
+ eventHandler,
+ configuration,
+ /* maxDifferencesToShow= */ -1, /* allowAnalysisCacheDiscards */
+ true);
+ } catch (InvalidConfigurationException e) {
+ throw new UnsupportedOperationException(
+ "InvalidConfigurationException was thrown and caught during a test, "
+ + "this case is not yet handled",
+ e);
+ }
}
public ArtifactFactory getArtifactFactory() {
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java
index b9a60c0eb866e1..ebe22607deb7f7 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java
@@ -1845,16 +1845,13 @@ public void extensionMetadata() throws Exception {
+ "Imported, but reported as indirect dependencies by the extension:\n"
+ " indirect_dep, indirect_dev_dep\n"
+ "\n"
- + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these"
+ + "\033[35m\033[1m ** You can use the following buildozer command to fix these"
+ " issues:\033[0m\n"
+ "\n"
+ "buildozer 'use_repo_add @ext//:defs.bzl ext missing_direct_dep non_dev_as_dev_dep'"
- + " //MODULE.bazel:all\n"
- + "buildozer 'use_repo_remove @ext//:defs.bzl ext dev_as_non_dev_dep"
- + " indirect_dep invalid_dep' //MODULE.bazel:all\n"
- + "buildozer 'use_repo_add dev @ext//:defs.bzl ext dev_as_non_dev_dep"
- + " missing_direct_dev_dep' //MODULE.bazel:all\n"
- + "buildozer 'use_repo_remove dev @ext//:defs.bzl ext indirect_dev_dep invalid_dev_dep"
+ + " 'use_repo_remove @ext//:defs.bzl ext dev_as_non_dev_dep indirect_dep invalid_dep'"
+ + " 'use_repo_add dev @ext//:defs.bzl ext dev_as_non_dev_dep missing_direct_dev_dep'"
+ + " 'use_repo_remove dev @ext//:defs.bzl ext indirect_dev_dep invalid_dev_dep"
+ " non_dev_as_dev_dep' //MODULE.bazel:all",
ImmutableSet.of(EventKind.WARNING));
}
@@ -1929,14 +1926,13 @@ public void extensionMetadata_all() throws Exception {
+ " extension (may cause the build to fail when used by other modules):\n"
+ " direct_dev_dep, indirect_dev_dep\n"
+ "\n"
- + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these"
+ + "\033[35m\033[1m ** You can use the following buildozer command to fix these"
+ " issues:\033[0m\n"
+ "\n"
+ "buildozer 'use_repo_add @ext//:defs.bzl ext direct_dev_dep indirect_dev_dep"
- + " missing_direct_dep missing_direct_dev_dep' //MODULE.bazel:all\n"
- + "buildozer 'use_repo_remove @ext//:defs.bzl ext invalid_dep' //MODULE.bazel:all\n"
- + "buildozer 'use_repo_remove dev @ext//:defs.bzl ext direct_dev_dep indirect_dev_dep"
- + " invalid_dev_dep' //MODULE.bazel:all",
+ + " missing_direct_dep missing_direct_dev_dep' 'use_repo_remove @ext//:defs.bzl ext"
+ + " invalid_dep' 'use_repo_remove dev @ext//:defs.bzl ext direct_dev_dep"
+ + " indirect_dev_dep invalid_dev_dep' //MODULE.bazel:all",
ImmutableSet.of(EventKind.WARNING));
}
@@ -2012,14 +2008,12 @@ public void extensionMetadata_allDev() throws Exception {
+ " extension (may cause the build to fail when used by other modules):\n"
+ " direct_dep, indirect_dep\n"
+ "\n"
- + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these"
+ + "\033[35m\033[1m ** You can use the following buildozer command to fix these"
+ " issues:\033[0m\n"
+ "\n"
+ "buildozer 'use_repo_remove @ext//:defs.bzl ext direct_dep indirect_dep invalid_dep'"
- + " //MODULE.bazel:all\n"
- + "buildozer 'use_repo_add dev @ext//:defs.bzl ext direct_dep indirect_dep"
- + " missing_direct_dep missing_direct_dev_dep' //MODULE.bazel:all\n"
- + "buildozer 'use_repo_remove dev @ext//:defs.bzl ext invalid_dev_dep'"
+ + " 'use_repo_add dev @ext//:defs.bzl ext direct_dep indirect_dep missing_direct_dep"
+ + " missing_direct_dev_dep' 'use_repo_remove dev @ext//:defs.bzl ext invalid_dev_dep'"
+ " //MODULE.bazel:all",
ImmutableSet.of(EventKind.WARNING));
}
@@ -2133,11 +2127,11 @@ public void extensionMetadata_isolated() throws Exception {
+ "Imported, but reported as indirect dependencies by the extension:\n"
+ " indirect_dep\n"
+ "\n"
- + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these"
+ + "\033[35m\033[1m ** You can use the following buildozer command to fix these"
+ " issues:\033[0m\n"
+ "\n"
- + "buildozer 'use_repo_add ext1 direct_dep missing_direct_dep' //MODULE.bazel:all\n"
- + "buildozer 'use_repo_remove ext1 indirect_dep' //MODULE.bazel:all",
+ + "buildozer 'use_repo_add ext1 direct_dep missing_direct_dep' 'use_repo_remove ext1"
+ + " indirect_dep' //MODULE.bazel:all",
ImmutableSet.of(EventKind.WARNING));
assertContainsEvent(
"WARNING /ws/MODULE.bazel:8:21: The module extension ext defined in @ext//:defs.bzl"
@@ -2147,7 +2141,7 @@ public void extensionMetadata_isolated() throws Exception {
+ " build to fail):\n"
+ " missing_direct_dep\n"
+ "\n"
- + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these"
+ + "\033[35m\033[1m ** You can use the following buildozer command to fix these"
+ " issues:\033[0m\n"
+ "\n"
+ "buildozer 'use_repo_add ext2 missing_direct_dep' //MODULE.bazel:all",
@@ -2217,11 +2211,11 @@ public void extensionMetadata_isolatedDev() throws Exception {
+ "Imported, but reported as indirect dependencies by the extension:\n"
+ " indirect_dep\n"
+ "\n"
- + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these"
+ + "\033[35m\033[1m ** You can use the following buildozer command to fix these"
+ " issues:\033[0m\n"
+ "\n"
- + "buildozer 'use_repo_add ext1 direct_dep missing_direct_dep' //MODULE.bazel:all\n"
- + "buildozer 'use_repo_remove ext1 indirect_dep' //MODULE.bazel:all",
+ + "buildozer 'use_repo_add ext1 direct_dep missing_direct_dep' 'use_repo_remove ext1"
+ + " indirect_dep' //MODULE.bazel:all",
ImmutableSet.of(EventKind.WARNING));
assertContainsEvent(
"WARNING /ws/MODULE.bazel:8:21: The module extension ext defined in @ext//:defs.bzl"
@@ -2231,7 +2225,7 @@ public void extensionMetadata_isolatedDev() throws Exception {
+ " build to fail):\n"
+ " missing_direct_dep\n"
+ "\n"
- + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these"
+ + "\033[35m\033[1m ** You can use the following buildozer command to fix these"
+ " issues:\033[0m\n"
+ "\n"
+ "buildozer 'use_repo_add ext2 missing_direct_dep' //MODULE.bazel:all",
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/PatchUtilTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/PatchUtilTest.java
index 763fe5541b6155..2d4a115df995fa 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/PatchUtilTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/PatchUtilTest.java
@@ -512,7 +512,8 @@ public void testChunkDoesNotMatch() throws IOException {
assertThat(expected)
.hasMessageThat()
.contains(
- "Incorrect Chunk: the chunk content doesn't match the target\n"
+ "in patch applied to /root/foo.cc: Incorrect Chunk: the chunk content doesn't match "
+ + "the target\n"
+ "**Original Position**: 2\n"
+ "\n"
+ "**Original Content**:\n"
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java
index 5de9acc764b3e1..2e57036c43036c 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryContextTest.java
@@ -375,8 +375,8 @@ public void testPatchErrorWasThrown() throws Exception {
.hasCauseThat()
.hasMessageThat()
.isEqualTo(
- "Error applying patch /outputDir/my.patch: Incorrect Chunk: the chunk content "
- + "doesn't match the target\n"
+ "Error applying patch /outputDir/my.patch: in patch applied to "
+ + "/outputDir/foo: Incorrect Chunk: the chunk content doesn't match the target\n"
+ "**Original Position**: 1\n"
+ "\n"
+ "**Original Content**:\n"
diff --git a/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java b/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java
index c34ff7469994fc..dfea5f77af83bd 100644
--- a/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java
@@ -729,7 +729,7 @@ public void oom_firstReportedViaHandleCrash() throws Exception {
testOom(
() -> {
OutOfMemoryError oom = new OutOfMemoryError();
- // Simulates an OOM coming from RetainedHeapLimiter, which reports the error by calling
+ // Simulates an OOM coming from GcThrashingDetector, which reports the error by calling
// handleCrash. Uses keepAlive() to avoid exiting the JVM and aborting the test, then
// throw the original oom to ensure control flow terminates.
BugReport.handleCrash(Crash.from(oom), CrashContext.keepAlive());
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java
index 3090658802b5cb..aa8c9a2271c8b3 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/ConvenienceSymlinkTest.java
@@ -238,10 +238,7 @@ public void initializeRuleClasses(ConfiguredRuleClassProvider.Builder builder) {
protected void setupOptions() throws Exception {
super.setupOptions();
- addOptions(
- // Turn on the new symlink behavior
- "--use_top_level_targets_for_symlinks",
- "--experimental_merged_skyframe_analysis_execution=" + mergedAnalysisExecution);
+ addOptions("--experimental_merged_skyframe_analysis_execution=" + mergedAnalysisExecution);
}
@Override
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java
index 18f06729ec0901..00abaf8a379d96 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/OutputArtifactConflictTest.java
@@ -661,10 +661,11 @@ public void testConflictAfterNullBuild(
assertThat(eventListener.eventIds.get(0).getAspect()).isEqualTo("//x:aspect.bzl%my_aspect");
}
- // There exists a discrepancy between --experimental_merged_skyframe_analysis_execution and
- // otherwise in case of --keep_going. The version with merged phases would still build one of the
- // 2 conflicting targets, while the one without would stop at the end of the analysis phase and
- // build nothing. The overall build would still fail.
+ // There exists a discrepancy between skymeld and noskymeld modes in case of --keep_going.
+ // noskymeld: bazel would stop at the end of the analysis phase and build nothing.
+ // skymeld: we either finish building one of the 2 conflicting artifacts, or none at all.
+ //
+ // The overall build would still fail in both cases.
@Test
public void testTwoConflictingTargets_keepGoing_behaviorDifferences(
@TestParameter boolean mergedAnalysisExecution) throws Exception {
@@ -679,8 +680,8 @@ public void testTwoConflictingTargets_keepGoing_behaviorDifferences(
Path outputXYY = Iterables.getOnlyElement(getArtifacts("//x/y:y")).getPath();
if (mergedAnalysisExecution) {
- // Verify that one and only one of the output artifacts from these 2 targets were built.
- assertThat((outputXY.isDirectory() && outputXYY.isFile()) ^ outputXY.isFile()).isTrue();
+ // Verify that these 2 conflicting artifacts can't both exist.
+ assertThat(outputXYY.isFile() && outputXY.isFile()).isFalse();
} else {
// Verify that none of the output artifacts were built.
assertThat(outputXY.exists()).isFalse();
diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java
index 41f0ef54aeaa29..eaee62da410146 100644
--- a/src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/cmdline/LabelInternerIntegrationTest.java
@@ -219,6 +219,6 @@ public void labelInterner_alwaysRespectWeakInternerWhenLabelInternerDisabled() t
@After
public void cleanup() {
- skyframeExecutor().getEvaluator().getInMemoryGraph().cleanupInterningPool();
+ skyframeExecutor().getEvaluator().getInMemoryGraph().cleanupInterningPools();
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutorTest.java b/src/test/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutorTest.java
index bac6320756053c..0809fe55875646 100644
--- a/src/test/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/concurrent/TieredPriorityExecutorTest.java
@@ -36,7 +36,6 @@
import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
-import java.util.concurrent.atomic.AtomicReference;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
@@ -493,94 +492,6 @@ public void criticalError_disposesPool() throws InterruptedException {
} while (true);
}
- @Test
- public void workDonation_processesAllTasks() throws InterruptedException {
- var holdAllThreads = new CountDownLatch(1);
- for (int i = 0; i < POOL_SIZE - 1; ++i) {
- executor.execute(() -> awaitUninterruptibly(holdAllThreads));
- }
-
- var donor = new AtomicReference();
- var donorSet = new CountDownLatch(1);
- var gate = new CountDownLatch(1);
- var donationDone = new CountDownLatch(1);
- executor.execute(
- () -> {
- donor.set(Thread.currentThread());
- donorSet.countDown();
- awaitUninterruptibly(gate);
- for (int i = 0; i < 100; ++i) {
- assertThat(TieredPriorityExecutor.tryDoQueuedWork()).isTrue();
- }
- donationDone.countDown();
- awaitUninterruptibly(holdAllThreads);
- });
-
- var receiver = new AtomicInteger();
- for (int i = 0; i < 100; ++i) {
- executor.execute(
- () -> {
- // This task is running from the donor's thread.
- assertThat(Thread.currentThread()).isEqualTo(donor.get());
- receiver.getAndIncrement();
- });
- }
-
- donorSet.await();
- gate.countDown();
- donationDone.await();
- assertThat(receiver.get()).isEqualTo(100);
- holdAllThreads.countDown();
- executor.awaitQuiescence(/* interruptWorkers= */ true);
- }
-
- @Test
- public void workDonation_handlesErrorsInDonatedWork() throws InterruptedException {
- var interruptedCount = new AtomicInteger();
- var holdAllThreads = new CountDownLatch(1);
- for (int i = 0; i < POOL_SIZE - 1; ++i) {
- executor.execute(
- () -> {
- try {
- while (!holdAllThreads.await(INTERRUPT_POLL_MS, MILLISECONDS)) {}
- } catch (InterruptedException e) {
- interruptedCount.getAndIncrement();
- }
- });
- }
-
- var donor = new AtomicReference();
- var donorSet = new CountDownLatch(1);
- var gate = new CountDownLatch(1);
- executor.execute(
- () -> {
- donor.set(Thread.currentThread());
- donorSet.countDown();
- awaitUninterruptibly(gate);
- assertThat(TieredPriorityExecutor.tryDoQueuedWork()).isTrue();
- try {
- while (!holdAllThreads.await(INTERRUPT_POLL_MS, MILLISECONDS)) {}
- } catch (InterruptedException e) {
- interruptedCount.getAndIncrement();
- }
- });
-
- executor.execute(
- () -> {
- assertThat(Thread.currentThread()).isEqualTo(donor.get());
- throw new AssertionError("critical error");
- });
-
- donorSet.await();
- gate.countDown();
-
- var error =
- assertThrows(
- AssertionError.class, () -> executor.awaitQuiescence(/* interruptWorkers= */ true));
- assertThat(error).hasMessageThat().contains("critical error");
- assertThat(interruptedCount.get()).isEqualTo(POOL_SIZE);
- }
-
@Test
public void settableFuture_respondsToInterrupt() throws InterruptedException {
var interruptedCount = new AtomicInteger();
@@ -682,31 +593,22 @@ public void taskQueueOverflow_executesTasks() throws InterruptedException {
// Waits for holders to start, otherwise they might race against the filling of the queue below.
allHoldersStarted.await();
- // Fills up the queue.
- var executed = new ArrayList();
+ // Over-fills the queue.
+ var executed = Sets.newConcurrentHashSet();
var expected = new ArrayList();
- for (int i = 0; i < PriorityWorkerPool.TASKS_MAX_VALUE; ++i) {
+ for (int i = 0; i < 2 * PriorityWorkerPool.TASKS_MAX_VALUE; ++i) {
expected.add(i);
final int index = i;
executor.execute(() -> executed.add(index));
}
- // Adds tasks that would overflow the queue. Since overflows consume tasks from the queue, this
- // causes all the tasks above to be executed.
- var donorValues = Sets.newConcurrentHashSet();
- for (int i = 0; i < PriorityWorkerPool.TASKS_MAX_VALUE; ++i) {
- final int index = i;
- executor.execute(() -> donorValues.add(index));
- }
-
- assertThat(executed).isEqualTo(expected);
- assertThat(donorValues).isEmpty();
+ assertThat(executed).isEmpty();
holdAllThreads.countDown();
executor.awaitQuiescence(/* interruptWorkers= */ true);
- assertThat(donorValues).containsExactlyElementsIn(expected);
+ assertThat(executed).containsExactlyElementsIn(expected);
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/metrics/MetricsCollectorTest.java b/src/test/java/com/google/devtools/build/lib/metrics/MetricsCollectorTest.java
index feeb64338dbb0b..14ca0f2cb013d7 100644
--- a/src/test/java/com/google/devtools/build/lib/metrics/MetricsCollectorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/metrics/MetricsCollectorTest.java
@@ -519,6 +519,13 @@ public void testAnalysisTimeInMs() throws Exception {
assertThat(buildMetrics.getTimingMetrics().getAnalysisPhaseTimeInMs()).isGreaterThan(0);
}
+ @Test
+ public void testExecutionTimeInMs() throws Exception {
+ buildTarget("//foo:foo");
+ BuildMetrics buildMetrics = buildMetricsEventListener.event.getBuildMetrics();
+ assertThat(buildMetrics.getTimingMetrics().getExecutionPhaseTimeInMs()).isGreaterThan(0);
+ }
+
@Test
public void testUsedHeapSizePostBuild() throws Exception {
// TODO(bazel-team): Fix recording used heap size on Windows.
diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD
index 58f0345d83c454..874d5a7500e8b9 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD
@@ -124,6 +124,7 @@ java_test(
"@googleapis//:google_rpc_code_java_proto",
"@googleapis//:google_rpc_error_details_java_proto",
"@googleapis//:google_rpc_status_java_proto",
+ "@maven//:com_google_testparameterinjector_test_parameter_injector",
"@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_grpc",
"@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto",
"@remoteapis//:build_bazel_semver_semver_java_proto",
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java
index ee714f9e54df67..9f4de20aba844d 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java
@@ -14,10 +14,13 @@
package com.google.devtools.build.lib.remote;
import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.Arrays.stream;
+import static org.junit.Assert.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doAnswer;
@@ -51,7 +54,11 @@
import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
import com.google.devtools.build.lib.vfs.DigestHashFunction;
+import com.google.devtools.build.lib.vfs.Dirent;
+import com.google.devtools.build.lib.vfs.FileStatus;
+import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystem.NotASymlinkException;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -60,17 +67,19 @@
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
+import java.io.FileNotFoundException;
import java.io.IOException;
import java.util.Map;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
import org.mockito.ArgumentCaptor;
import org.mockito.stubbing.Answer;
/** Tests for {@link RemoteActionFileSystem} */
-@RunWith(JUnit4.class)
+@RunWith(TestParameterInjector.class)
public final class RemoteActionFileSystemTest extends RemoteActionFileSystemTestBase {
private static final RemoteOutputChecker DUMMY_REMOTE_OUTPUT_CHECKER =
new RemoteOutputChecker(
@@ -88,6 +97,21 @@ public final class RemoteActionFileSystemTest extends RemoteActionFileSystemTest
private final ArtifactRoot outputRoot =
ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, RELATIVE_OUTPUT_PATH);
+ enum FilesystemTestParam {
+ LOCAL,
+ REMOTE;
+
+ FileSystem getFilesystem(RemoteActionFileSystem actionFs) {
+ switch (this) {
+ case LOCAL:
+ return actionFs.getLocalFileSystem();
+ case REMOTE:
+ return actionFs.getRemoteOutputTree();
+ }
+ throw new IllegalStateException();
+ }
+ };
+
@Before
public void setUp() throws IOException {
outputRoot.getRoot().asPath().createDirectoryAndParents();
@@ -340,6 +364,250 @@ public void getMetadata_notFound() throws Exception {
assertThat(actionFs.getInputMetadata(artifact)).isNull();
}
+ @Test
+ public void statAndExists_fromInputArtifactData() throws Exception {
+ ActionInputMap inputs = new ActionInputMap(1);
+ Artifact artifact = createLocalArtifact("local-file", "local contents", inputs);
+ PathFragment path = artifact.getPath().asFragment();
+ FileArtifactValue metadata = checkNotNull(inputs.getInputMetadata(artifact));
+ RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem(inputs);
+
+ assertThat(actionFs.exists(path, /* followSymlinks= */ true)).isTrue();
+
+ FileStatus st = actionFs.stat(path, /* followSymlinks= */ true);
+ assertThat(st.isFile()).isTrue();
+ assertThat(st).isInstanceOf(FileStatusWithDigest.class);
+ assertThat(((FileStatusWithDigest) st).getDigest()).isEqualTo(metadata.getDigest());
+ }
+
+ @Test
+ public void statAndExists_fromRemoteOutputTree() throws Exception {
+ RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+ Artifact artifact = ActionsTestUtil.createArtifact(outputRoot, "out");
+ PathFragment path = artifact.getPath().asFragment();
+ FileArtifactValue metadata =
+ injectRemoteFile(actionFs, artifact.getPath().asFragment(), "remote contents");
+
+ assertThat(actionFs.exists(path, /* followSymlinks= */ true)).isTrue();
+
+ FileStatus st = actionFs.stat(path, /* followSymlinks= */ true);
+ assertThat(st.isFile()).isTrue();
+ assertThat(st).isInstanceOf(FileStatusWithDigest.class);
+ assertThat(((FileStatusWithDigest) st).getDigest()).isEqualTo(metadata.getDigest());
+ }
+
+ @Test
+ public void statAndExists_fromLocalFilesystem() throws Exception {
+ RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+ Artifact artifact = ActionsTestUtil.createArtifact(outputRoot, "out");
+ PathFragment path = artifact.getPath().asFragment();
+ writeLocalFile(actionFs, artifact.getPath().asFragment(), "local contents");
+
+ assertThat(actionFs.exists(path)).isTrue();
+
+ FileStatus st = actionFs.stat(path, /* followSymlinks= */ true);
+ assertThat(st.isFile()).isTrue();
+ assertThat(st.getSize()).isEqualTo("local contents".getBytes(UTF_8).length);
+ }
+
+ @Test
+ public void statAndExists_followSymlinks(
+ @TestParameter FilesystemTestParam from, @TestParameter FilesystemTestParam to)
+ throws Exception {
+ RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+ FileSystem fromFs = from.getFilesystem(actionFs);
+ FileSystem toFs = to.getFilesystem(actionFs);
+
+ PathFragment linkPath = getOutputPath("sym");
+ PathFragment targetPath = getOutputPath("target");
+ fromFs.getPath(linkPath).createSymbolicLink(execRoot.getRelative(targetPath).asFragment());
+
+ assertThat(actionFs.exists(linkPath, /* followSymlinks= */ false)).isTrue();
+ assertThat(actionFs.exists(linkPath, /* followSymlinks= */ true)).isFalse();
+ assertThat(actionFs.stat(linkPath, /* followSymlinks= */ false).isSymbolicLink()).isTrue();
+ assertThrows(
+ FileNotFoundException.class, () -> actionFs.stat(linkPath, /* followSymlinks= */ true));
+
+ if (toFs.equals(actionFs.getLocalFileSystem())) {
+ writeLocalFile(actionFs, targetPath, "content");
+ } else {
+ injectRemoteFile(actionFs, targetPath, "content");
+ }
+
+ assertThat(actionFs.exists(linkPath, /* followSymlinks= */ false)).isTrue();
+ assertThat(actionFs.stat(linkPath, /* followSymlinks= */ false).isSymbolicLink()).isTrue();
+ assertThat(actionFs.exists(linkPath, /* followSymlinks= */ true)).isTrue();
+ assertThat(actionFs.stat(linkPath, /* followSymlinks= */ true).isFile()).isTrue();
+ }
+
+ @Test
+ public void statAndExists_notFound() throws Exception {
+ RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+ Artifact artifact = ActionsTestUtil.createArtifact(outputRoot, "out");
+ PathFragment path = artifact.getPath().asFragment();
+
+ assertThat(actionFs.exists(path)).isFalse();
+
+ assertThrows(
+ FileNotFoundException.class, () -> actionFs.stat(path, /* followSymlinks= */ true));
+ }
+
+ @Test
+ public void readdir_fromRemoteOutputTree() throws Exception {
+ RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+ Artifact a1 = ActionsTestUtil.createArtifact(outputRoot, "dir/out1");
+ Artifact a2 = ActionsTestUtil.createArtifact(outputRoot, "dir/out2");
+ Artifact a3 = ActionsTestUtil.createArtifact(outputRoot, "dir/subdir/out3");
+ injectRemoteFile(actionFs, a1.getPath().asFragment(), "contents1");
+ injectRemoteFile(actionFs, a2.getPath().asFragment(), "contents2");
+ injectRemoteFile(actionFs, a3.getPath().asFragment(), "contents3");
+ PathFragment dirPath = a1.getPath().getParentDirectory().asFragment();
+
+ assertReaddir(
+ actionFs,
+ dirPath,
+ /* followSymlinks= */ true,
+ new Dirent("out1", Dirent.Type.FILE),
+ new Dirent("out2", Dirent.Type.FILE),
+ new Dirent("subdir", Dirent.Type.DIRECTORY));
+ }
+
+ @Test
+ public void readdir_fromLocalFilesystem() throws Exception {
+ RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+ Artifact a1 = ActionsTestUtil.createArtifact(outputRoot, "dir/out1");
+ Artifact a2 = ActionsTestUtil.createArtifact(outputRoot, "dir/out2");
+ Artifact a3 = ActionsTestUtil.createArtifact(outputRoot, "dir/subdir/out3");
+ writeLocalFile(actionFs, a1.getPath().asFragment(), "contents1");
+ writeLocalFile(actionFs, a2.getPath().asFragment(), "contents2");
+ writeLocalFile(actionFs, a3.getPath().asFragment(), "contents3");
+ PathFragment dirPath = a1.getPath().getParentDirectory().asFragment();
+
+ assertReaddir(
+ actionFs,
+ dirPath,
+ /* followSymlinks= */ true,
+ new Dirent("out1", Dirent.Type.FILE),
+ new Dirent("out2", Dirent.Type.FILE),
+ new Dirent("subdir", Dirent.Type.DIRECTORY));
+ }
+
+ @Test
+ public void readdir_fromBothFilesystems() throws Exception {
+ RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+ Artifact a1 = ActionsTestUtil.createArtifact(outputRoot, "dir/out1");
+ Artifact a2 = ActionsTestUtil.createArtifact(outputRoot, "dir/out2");
+ Artifact a3 = ActionsTestUtil.createArtifact(outputRoot, "dir/subdir/out3");
+ Artifact a4 = ActionsTestUtil.createArtifact(outputRoot, "dir/subdir/out4");
+ writeLocalFile(actionFs, a1.getPath().asFragment(), "contents1");
+ writeLocalFile(actionFs, a2.getPath().asFragment(), "contents2");
+ injectRemoteFile(actionFs, a2.getPath().asFragment(), "contents2");
+ injectRemoteFile(actionFs, a3.getPath().asFragment(), "contents3");
+ injectRemoteFile(actionFs, a4.getPath().asFragment(), "contents4");
+ PathFragment dirPath = a1.getPath().getParentDirectory().asFragment();
+
+ assertReaddir(
+ actionFs,
+ dirPath,
+ /* followSymlinks= */ true,
+ new Dirent("out1", Dirent.Type.FILE),
+ new Dirent("out2", Dirent.Type.FILE),
+ new Dirent("subdir", Dirent.Type.DIRECTORY));
+ }
+
+ @Test
+ public void readdir_followSymlinks(
+ @TestParameter FilesystemTestParam from, @TestParameter FilesystemTestParam to)
+ throws Exception {
+ RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+ FileSystem fromFs = from.getFilesystem(actionFs);
+ FileSystem toFs = to.getFilesystem(actionFs);
+
+ PathFragment dirPath = getOutputPath("dir");
+ PathFragment linkPath = getOutputPath("dir/sym");
+ PathFragment targetPath = getOutputPath("target");
+
+ fromFs.getPath(dirPath).createDirectory();
+ fromFs.getPath(linkPath).createSymbolicLink(execRoot.getRelative(targetPath).asFragment());
+
+ assertReaddir(
+ actionFs, dirPath, /* followSymlinks= */ false, new Dirent("sym", Dirent.Type.SYMLINK));
+ assertReaddir(
+ actionFs, dirPath, /* followSymlinks= */ true, new Dirent("sym", Dirent.Type.UNKNOWN));
+
+ if (toFs.equals(actionFs.getLocalFileSystem())) {
+ writeLocalFile(actionFs, targetPath, "content");
+ } else {
+ injectRemoteFile(actionFs, targetPath, "content");
+ }
+
+ assertReaddir(
+ actionFs, dirPath, /* followSymlinks= */ false, new Dirent("sym", Dirent.Type.SYMLINK));
+ assertReaddir(
+ actionFs, dirPath, /* followSymlinks= */ true, new Dirent("sym", Dirent.Type.FILE));
+ }
+
+ @Test
+ public void readdir_notFound() throws Exception {
+ RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+ Artifact artifact = ActionsTestUtil.createArtifact(outputRoot, "dir/out");
+ PathFragment path = artifact.getPath().getParentDirectory().asFragment();
+
+ assertThrows(FileNotFoundException.class, () -> actionFs.getDirectoryEntries(path));
+ assertThrows(
+ FileNotFoundException.class, () -> actionFs.readdir(path, /* followSymlinks= */ true));
+ }
+
+ private void assertReaddir(
+ RemoteActionFileSystem actionFs,
+ PathFragment dirPath,
+ boolean followSymlinks,
+ Dirent... expected)
+ throws Exception {
+ assertThat(actionFs.readdir(dirPath, followSymlinks))
+ .containsExactlyElementsIn(expected)
+ .inOrder();
+ assertThat(actionFs.getDirectoryEntries(dirPath))
+ .containsExactlyElementsIn(stream(expected).map(Dirent::getName).collect(toImmutableList()))
+ .inOrder();
+ }
+
+ @Test
+ public void readSymbolicLink_fromLocalFilesystem() throws Exception {
+ RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+ PathFragment filePath = getOutputPath("file");
+ PathFragment linkPath = getOutputPath("sym");
+ PathFragment targetPath = PathFragment.create("/some/path");
+ actionFs.getLocalFileSystem().getPath(linkPath).createSymbolicLink(targetPath);
+ writeLocalFile(actionFs, filePath, "contents");
+
+ assertThat(actionFs.readSymbolicLink(linkPath)).isEqualTo(targetPath);
+
+ assertThrows(NotASymlinkException.class, () -> actionFs.readSymbolicLink(filePath));
+ }
+
+ @Test
+ public void readSymbolicLink_fromRemoteFilesystem() throws Exception {
+ RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+ PathFragment filePath = getOutputPath("file");
+ PathFragment linkPath = getOutputPath("sym");
+ PathFragment targetPath = PathFragment.create("/some/path");
+ actionFs.getRemoteOutputTree().getPath(linkPath).createSymbolicLink(targetPath);
+ injectRemoteFile(actionFs, filePath, "contents");
+
+ assertThat(actionFs.readSymbolicLink(linkPath)).isEqualTo(targetPath);
+
+ assertThrows(NotASymlinkException.class, () -> actionFs.readSymbolicLink(filePath));
+ }
+
+ @Test
+ public void readSymbolicLink_notFound() throws Exception {
+ RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+ PathFragment linkPath = getOutputPath("sym");
+
+ assertThrows(FileNotFoundException.class, () -> actionFs.readSymbolicLink(linkPath));
+ }
+
@Test
public void createSymbolicLink_localFileArtifact() throws Exception {
// arrange
@@ -508,6 +776,53 @@ public void createSymbolicLink_unresolvedSymlink() throws Exception {
verifyNoInteractions(metadataInjector);
}
+ @Test
+ public void resolveSymbolicLinks(
+ @TestParameter FilesystemTestParam a, @TestParameter FilesystemTestParam b) throws Exception {
+ RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
+ FileSystem aFs = a.getFilesystem(actionFs);
+ FileSystem bFs = b.getFilesystem(actionFs);
+
+ // /a
+ // |- asub
+ // | `- afile
+ // `- abssym -> /b/bsub
+ // `- relsym -> asub
+ // /b
+ // `- bsub
+ // `- bfile
+
+ aFs.getPath(getOutputPath("a/asub")).createDirectoryAndParents();
+ aFs.getPath(getOutputPath("a/abssym")).createSymbolicLink(getOutputPath("b/bsub"));
+ aFs.getPath(getOutputPath("a/relsym")).createSymbolicLink(PathFragment.create("asub"));
+ if (aFs.equals(actionFs.getLocalFileSystem())) {
+ writeLocalFile(actionFs, getOutputPath("a/asub/afile"), "content");
+ } else {
+ injectRemoteFile(actionFs, getOutputPath("a/asub/afile"), "content");
+ }
+
+ bFs.getPath(getOutputPath("b/bsub")).createDirectoryAndParents();
+ if (bFs.equals(actionFs.getLocalFileSystem())) {
+ writeLocalFile(actionFs, getOutputPath("b/bsub/bfile"), "content");
+ } else {
+ injectRemoteFile(actionFs, getOutputPath("b/bsub/bfile"), "content");
+ }
+
+ assertThat(actionFs.getPath(getOutputPath("a/relsym/afile")).resolveSymbolicLinks())
+ .isEqualTo(actionFs.getPath(getOutputPath("a/asub/afile")));
+
+ assertThrows(
+ FileNotFoundException.class,
+ () -> actionFs.getPath(getOutputPath("a/bsub/nofile")).resolveSymbolicLinks());
+
+ assertThat(actionFs.getPath(getOutputPath("a/abssym/bfile")).resolveSymbolicLinks())
+ .isEqualTo(actionFs.getPath(getOutputPath("b/bsub/bfile")));
+
+ assertThrows(
+ FileNotFoundException.class,
+ () -> actionFs.getPath(getOutputPath("b/bsub/nofile")).resolveSymbolicLinks());
+ }
+
@Test
public void renameTo_onlyLocalFile_renameLocalFile() throws Exception {
FileSystem actionFs = createActionFileSystem();
@@ -542,6 +857,7 @@ protected FileArtifactValue injectRemoteFile(
@Override
protected void writeLocalFile(FileSystem actionFs, PathFragment path, String content)
throws IOException {
+ actionFs.getPath(path).getParentDirectory().createDirectoryAndParents();
FileSystemUtils.writeContent(actionFs.getPath(path), UTF_8, content);
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinarySplitFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinarySplitFunctionsTest.java
index a1c1f07f966961..a93cdad1de5543 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinarySplitFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinarySplitFunctionsTest.java
@@ -51,7 +51,8 @@ private Action getPredecessorByInputName(Action action, String str) {
return null;
}
- private LtoBackendAction setupAndRunToolchainActions(String... config) throws Exception {
+ private LtoBackendAction setupAndRunToolchainActions(String fdoFlavor, String... config)
+ throws Exception {
AnalysisMock.get()
.ccSupport()
.setupCcToolchainConfig(
@@ -60,14 +61,16 @@ private LtoBackendAction setupAndRunToolchainActions(String... config) throws Ex
.withFeatures(
CppRuleClasses.THIN_LTO,
MockCcSupport.HOST_AND_NONHOST_CONFIGURATION_FEATURES,
- CppRuleClasses.FDO_OPTIMIZE,
CppRuleClasses.SUPPORTS_START_END_LIB,
+ CppRuleClasses.FDO_OPTIMIZE,
+ CppRuleClasses.FSAFDO,
CppRuleClasses.ENABLE_FDO_SPLIT_FUNCTIONS,
MockCcSupport.FDO_SPLIT_FUNCTIONS,
MockCcSupport.SPLIT_FUNCTIONS));
+ // "profile" affects whether FDO_OPTIMIZE or AUTOFDO is activated.
List testConfig =
- Lists.newArrayList("--fdo_optimize=/pkg/profile.zip", "--compilation_mode=opt");
+ Lists.newArrayList("--fdo_optimize=" + getProfile(fdoFlavor), "--compilation_mode=opt");
Collections.addAll(testConfig, config);
useConfiguration(Iterables.toArray(testConfig, String.class));
@@ -87,11 +90,19 @@ private LtoBackendAction setupAndRunToolchainActions(String... config) throws Ex
return backendAction;
}
+ /** Gets profile for fdoFlavor. The profile suffix differentiates FDO_OPTIMIZE and FSAFDO mode. */
+ private String getProfile(String fdoFlavor) {
+ if (fdoFlavor.equals(CppRuleClasses.FSAFDO) || fdoFlavor.equals(CppRuleClasses.AUTOFDO)) {
+ return "/pkg/profile.afdo";
+ }
+ return "/pkg/profile.zip";
+ }
+
/**
- * Tests that split_functions is enabled for FDO with LLVM with --features=fdo_split_functions.
+ * Helps check that split_functions is enabled for fdoFlavor with LLVM with
+ * --features=fdo_split_functions.
*/
- @Test
- public void fdoImplicitSplitFunctions() throws Exception {
+ private void implicitSplitFunctions(String fdoFlavor) throws Exception {
scratch.file(
"pkg/BUILD",
"package(features = ['thin_lto'])",
@@ -102,7 +113,8 @@ public void fdoImplicitSplitFunctions() throws Exception {
scratch.file("pkg/binfile.cc", "int main() {}");
scratch.file("pkg/profile.zip", "");
- LtoBackendAction backendAction = setupAndRunToolchainActions("--features=fdo_split_functions");
+ LtoBackendAction backendAction =
+ setupAndRunToolchainActions(fdoFlavor, "--features=fdo_split_functions");
assertThat(Joiner.on(" ").join(backendAction.getArguments()))
.containsMatch("-fsplit-machine-functions");
@@ -111,11 +123,50 @@ public void fdoImplicitSplitFunctions() throws Exception {
}
/**
- * Tests that split_functions is not enabled for FDO with LLVM without
+ * Tests that split_functions is enabled for FDO with LLVM with --features=fdo_split_functions.
+ */
+ @Test
+ public void fdoImplicitSplitFunctions() throws Exception {
+ implicitSplitFunctions(CppRuleClasses.FDO_OPTIMIZE);
+ }
+
+ /**
+ * Tests that split_functions is enabled for FSAFDO with LLVM with --features=fdo_split_functions.
+ */
+ @Test
+ public void fsafdoImplicitSplitFunctions() throws Exception {
+ implicitSplitFunctions(CppRuleClasses.FSAFDO);
+ }
+
+ /**
+ * Tests that split_functions is disabled for AutoFDO without FSAFDO with LLVM with
* --features=fdo_split_functions.
*/
@Test
- public void fdoNoImplicitSplitFunctions() throws Exception {
+ public void noFSAFDODisablesSplitFunction() throws Exception {
+ scratch.file(
+ "pkg/BUILD",
+ "package(features = ['thin_lto'])",
+ "",
+ "cc_binary(name = 'bin',",
+ " srcs = ['binfile.cc', ],",
+ " malloc = '//base:system_malloc')");
+ scratch.file("pkg/binfile.cc", "int main() {}");
+ scratch.file("pkg/profile.afdo", "");
+
+ LtoBackendAction backendAction =
+ setupAndRunToolchainActions(
+ CppRuleClasses.FSAFDO, "--features=-fsafdo", "--features=fdo_split_functions");
+
+ assertThat(Joiner.on(" ").join(backendAction.getArguments()))
+ .doesNotContain("-fsplit-machine-functions");
+ }
+
+ /**
+ * Helps check that split_functions is not enabled for fdoFlavor with LLVM without
+ * --features=fdo_split_functions.
+ */
+ private void noImplicitSplitFunctions(String fdoFlavor) throws Exception {
scratch.file(
"pkg/BUILD",
"package(features = ['thin_lto'])",
@@ -126,18 +177,35 @@ public void fdoNoImplicitSplitFunctions() throws Exception {
scratch.file("pkg/binfile.cc", "int main() {}");
scratch.file("pkg/profile.zip", "");
- LtoBackendAction backendAction = setupAndRunToolchainActions();
+ LtoBackendAction backendAction = setupAndRunToolchainActions(fdoFlavor);
assertThat(Joiner.on(" ").join(backendAction.getArguments()))
.doesNotContain("-fsplit-machine-functions");
}
/**
- * Tests that split_functions is not enabled for FDO with LLVM when --features=fdo_split_functions
- * is overridden by --features=-split_functions.
+ * Tests that split_functions is not enabled for FDO with LLVM without
+ * --features=fdo_split_functions.
*/
@Test
- public void fdoImplicitSplitFunctionsDisabledOption() throws Exception {
+ public void fdoNoImplicitSplitFunctions() throws Exception {
+ noImplicitSplitFunctions(CppRuleClasses.FDO_OPTIMIZE);
+ }
+
+ /**
+ * Tests that split_functions is not enabled for FSAFDO with LLVM without
+ * --features=fdo_split_functions.
+ */
+ @Test
+ public void fsafdoNoImplicitSplitFunctions() throws Exception {
+ noImplicitSplitFunctions(CppRuleClasses.FSAFDO);
+ }
+
+ /**
+ * Helps check that split_functions is not enabled for fdoFlavor with LLVM when
+ * --features=fdo_split_functions is overridden by --features=-split_functions.
+ */
+ private void implicitSplitFunctionsDisabledOption(String fdoFlavor) throws Exception {
scratch.file(
"pkg/BUILD",
"package(features = ['thin_lto'])",
@@ -150,7 +218,7 @@ public void fdoImplicitSplitFunctionsDisabledOption() throws Exception {
LtoBackendAction backendAction =
setupAndRunToolchainActions(
- "--features=fdo_split_functions", "--features=-split_functions");
+ fdoFlavor, "--features=fdo_split_functions", "--features=-split_functions");
assertThat(Joiner.on(" ").join(backendAction.getArguments()))
.doesNotContain("-fsplit-machine-functions");
@@ -158,10 +226,27 @@ public void fdoImplicitSplitFunctionsDisabledOption() throws Exception {
/**
* Tests that split_functions is not enabled for FDO with LLVM when --features=fdo_split_functions
- * is overridden by --features=-split_functions in the build rule.
+ * is overridden by --features=-split_functions.
*/
@Test
- public void fdoImplicitSplitFunctionsDisabledBuild() throws Exception {
+ public void fdoImplicitSplitFunctionsDisabledOption() throws Exception {
+ implicitSplitFunctionsDisabledOption(CppRuleClasses.FDO_OPTIMIZE);
+ }
+
+ /**
+ * Tests that split_functions is not enabled for FSAFDO with LLVM when
+ * --features=fdo_split_functions is overridden by --features=-split_functions.
+ */
+ @Test
+ public void fsafdoImplicitSplitFunctionsDisabledOption() throws Exception {
+ implicitSplitFunctionsDisabledOption(CppRuleClasses.FSAFDO);
+ }
+
+ /**
+ * Helps check that split_functions is not enabled for fdoFlavor with LLVM when
+ * --features=fdo_split_functions is overridden by --features=-split_functions in the build rule.
+ */
+ private void implicitSplitFunctionsDisabledBuild(String fdoFlavor) throws Exception {
scratch.file(
"pkg/BUILD",
"package(features = ['thin_lto'])",
@@ -173,15 +258,33 @@ public void fdoImplicitSplitFunctionsDisabledBuild() throws Exception {
scratch.file("pkg/binfile.cc", "int main() {}");
scratch.file("pkg/profile.zip", "");
- LtoBackendAction backendAction = setupAndRunToolchainActions("--features=fdo_split_functions");
+ LtoBackendAction backendAction =
+ setupAndRunToolchainActions(fdoFlavor, "--features=fdo_split_functions");
assertThat(Joiner.on(" ").join(backendAction.getArguments()))
.doesNotContain("-fsplit-machine-functions");
}
- /** Tests that using propeller_optimize automatically disables implicit split functions. */
+ /**
+ * Tests that split_functions is not enabled for FDO with LLVM when --features=fdo_split_functions
+ * is overridden by --features=-split_functions in the build rule.
+ */
+ @Test
+ public void fdoImplicitSplitFunctionsDisabledBuild() throws Exception {
+ implicitSplitFunctionsDisabledBuild(CppRuleClasses.FDO_OPTIMIZE);
+ }
+
+ /**
+ * Tests that split_functions is not enabled for FSAFDO with LLVM when
+ * --features=fdo_split_functions is overridden by --features=-split_functions in the build rule.
+ */
@Test
- public void propellerOptimizeDisablesImplicitSplitFunctions() throws Exception {
+ public void fsafdoImplicitSplitFunctionsDisabledBuild() throws Exception {
+ implicitSplitFunctionsDisabledBuild(CppRuleClasses.FSAFDO);
+ }
+
+ /** Helps check that using propeller_optimize automatically disables implicit split functions. */
+ private void propellerOptimizeDisablesImplicitSplitFunctions(String fdoFlavor) throws Exception {
scratch.file(
"pkg/BUILD",
"package(features = ['thin_lto'])",
@@ -194,6 +297,7 @@ public void propellerOptimizeDisablesImplicitSplitFunctions() throws Exception {
LtoBackendAction backendAction =
setupAndRunToolchainActions(
+ fdoFlavor,
"--features=fdo_split_functions",
"--propeller_optimize_absolute_cc_profile=/tmp/cc.txt");
@@ -207,12 +311,23 @@ public void propellerOptimizeDisablesImplicitSplitFunctions() throws Exception {
.doesNotMatch("-fsplit-machine-functions");
}
+ /** Tests that using propeller_optimize automatically disables implicit split functions. */
+ @Test
+ public void fdoPropellerOptimizeDisablesImplicitSplitFunctions() throws Exception {
+ propellerOptimizeDisablesImplicitSplitFunctions(CppRuleClasses.FDO_OPTIMIZE);
+ }
+
+ /** Tests that using propeller_optimize automatically disables implicit split functions. */
+ @Test
+ public void fsafdoPropellerOptimizeDisablesImplicitSplitFunctions() throws Exception {
+ propellerOptimizeDisablesImplicitSplitFunctions(CppRuleClasses.FSAFDO);
+ }
+
/**
- * Tests that split_functions is not enabled for FDO with LLVM when --features=fdo_split_functions
- * is overridden by --features=-split_functions in the package.
+ * Helps check that split_functions is not enabled for fdoFlavor with LLVM when
+ * --features=fdo_split_functions is overridden by --features=-split_functions in the package.
*/
- @Test
- public void fdoImplicitSplitFunctionsDisabledPackage() throws Exception {
+ private void implicitSplitFunctionsDisabledPackage(String fdoFlavor) throws Exception {
scratch.file(
"pkg/BUILD",
"package(features = ['thin_lto', '-split_functions'])",
@@ -223,9 +338,28 @@ public void fdoImplicitSplitFunctionsDisabledPackage() throws Exception {
scratch.file("pkg/binfile.cc", "int main() {}");
scratch.file("pkg/profile.zip", "");
- LtoBackendAction backendAction = setupAndRunToolchainActions("--features=fdo_split_functions");
+ LtoBackendAction backendAction =
+ setupAndRunToolchainActions(fdoFlavor, "--features=fdo_split_functions");
assertThat(Joiner.on(" ").join(backendAction.getArguments()))
.doesNotContain("-fsplit-machine-functions");
}
+
+ /**
+ * Tests that split_functions is not enabled for FDO with LLVM when --features=fdo_split_functions
+ * is overridden by --features=-split_functions in the package.
+ */
+ @Test
+ public void fdoImplicitSplitFunctionsDisabledPackage() throws Exception {
+ implicitSplitFunctionsDisabledPackage(CppRuleClasses.FDO_OPTIMIZE);
+ }
+
+ /**
+ * Tests that split_functions is not enabled for FSAFDO with LLVM when
+ * --features=fdo_split_functions is overridden by --features=-split_functions in the package.
+ */
+ @Test
+ public void fsafdoImplicitSplitFunctionsDisabledPackage() throws Exception {
+ implicitSplitFunctionsDisabledPackage(CppRuleClasses.FSAFDO);
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/java/JavaConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/rules/java/JavaConfigurationTest.java
index 05ad07b0f9dbe4..bad4f6dfe55799 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/java/JavaConfigurationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/java/JavaConfigurationTest.java
@@ -35,6 +35,6 @@ public void testExperimentalBytecodeOptimizersFlag() throws Exception {
assertThrows(
InvalidConfigurationException.class,
() -> create("--experimental_bytecode_optimizers=somekey,somevalue"));
- assertThat(thrown).hasMessageThat().contains("can only accept up to one mapping");
+ assertThat(thrown).hasMessageThat().contains("can only accept exactly one mapping");
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java b/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java
index 607ee5c3db8fd0..4c54059625c77c 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtractTest.java
@@ -824,7 +824,10 @@ public void repositoryRule() throws Exception {
"",
"my_repo_rule = repository_rule(",
" implementation = _impl,",
- " doc = 'My repository rule',",
+ " doc = '''My repository rule",
+ "",
+ " With details",
+ " ''',",
" attrs = {",
" 'a': attr.string(doc = 'My doc', default = 'foo'),",
" 'b': attr.string(mandatory = True),",
@@ -855,7 +858,7 @@ public void repositoryRule() throws Exception {
.setRuleName("foo.repo_rule")
.setOriginKey(
OriginKey.newBuilder().setName("my_repo_rule").setFile("//:dep.bzl").build())
- .setDocString("My repository rule")
+ .setDocString("My repository rule\n\nWith details")
.addAllAttribute(ModuleInfoExtractor.IMPLICIT_REPOSITORY_RULE_ATTRIBUTES)
.addAttribute(
AttributeInfo.newBuilder()
@@ -878,7 +881,9 @@ public void moduleExtension() throws Exception {
scratch.file(
"dep.bzl",
"_install = tag_class(",
- " doc = 'Install',",
+ " doc = '''Install",
+ " ",
+ " With details''',",
" attrs = {",
" 'artifacts': attr.string_list(doc = 'Artifacts'),",
" '_hidden': attr.bool(),",
@@ -896,7 +901,9 @@ public void moduleExtension() throws Exception {
" pass",
"",
"my_ext = module_extension(",
- " doc = 'My extension',",
+ " doc = '''My extension",
+ "",
+ " With details''',",
" tag_classes = {",
" 'install': _install,",
" 'artifact': _artifact,",
@@ -925,12 +932,12 @@ public void moduleExtension() throws Exception {
.containsExactly(
ModuleExtensionInfo.newBuilder()
.setExtensionName("foo.ext")
- .setDocString("My extension")
+ .setDocString("My extension\n\nWith details")
.setOriginKey(OriginKey.newBuilder().setFile("//:dep.bzl").build())
.addTagClass(
ModuleExtensionTagClassInfo.newBuilder()
.setTagName("install")
- .setDocString("Install")
+ .setDocString("Install\n\nWith details")
.addAttribute(
AttributeInfo.newBuilder()
.setName("artifacts")
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/MemoryPressureListenerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/MemoryPressureListenerTest.java
index 43405372996db2..eb50b463ceacb5 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/MemoryPressureListenerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/MemoryPressureListenerTest.java
@@ -28,11 +28,12 @@
import com.google.common.eventbus.EventBus;
import com.google.common.eventbus.Subscribe;
import com.google.devtools.build.lib.bugreport.BugReporter;
-import com.google.devtools.common.options.Options;
+import com.google.devtools.build.lib.clock.BlazeClock;
import com.sun.management.GarbageCollectionNotificationInfo;
import com.sun.management.GcInfo;
import java.lang.management.GarbageCollectorMXBean;
import java.lang.management.MemoryUsage;
+import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
import javax.management.Notification;
@@ -52,7 +53,6 @@ private interface NotificationBean extends GarbageCollectorMXBean, NotificationE
private final NotificationBean mockUselessBean = mock(NotificationBean.class);
private final BugReporter bugReporter = mock(BugReporter.class);
private final EventBus eventBus = new EventBus();
- private final RetainedHeapLimiter retainedHeapLimiter = RetainedHeapLimiter.create(bugReporter);
private final List events = new ArrayList<>();
@Before
@@ -87,14 +87,14 @@ public void createFromBeans_throwsIfNoTenuredSpaceBean() {
IllegalStateException.class,
() ->
MemoryPressureListener.createFromBeans(
- ImmutableList.of(mockUselessBean), retainedHeapLimiter, directExecutor()));
+ ImmutableList.of(mockUselessBean), directExecutor()));
}
@Test
public void simple() {
MemoryPressureListener underTest =
MemoryPressureListener.createFromBeans(
- ImmutableList.of(mockUselessBean, mockBean), retainedHeapLimiter, directExecutor());
+ ImmutableList.of(mockUselessBean, mockBean), directExecutor());
underTest.setEventBus(eventBus);
verify(mockBean).addNotificationListener(underTest, null, null);
verify(mockUselessBean, never()).addNotificationListener(any(), any(), any());
@@ -134,7 +134,7 @@ public void simple() {
public void nullEventBus_doNotPublishEvent() {
MemoryPressureListener underTest =
MemoryPressureListener.createFromBeans(
- ImmutableList.of(mockUselessBean, mockBean), retainedHeapLimiter, directExecutor());
+ ImmutableList.of(mockUselessBean, mockBean), directExecutor());
verify(mockBean).addNotificationListener(underTest, null, null);
verify(mockUselessBean, never()).addNotificationListener(any(), any(), any());
@@ -166,8 +166,7 @@ public void nullEventBus_doNotPublishEvent() {
@Test
public void manualGc() {
MemoryPressureListener underTest =
- MemoryPressureListener.createFromBeans(
- ImmutableList.of(mockBean), retainedHeapLimiter, directExecutor());
+ MemoryPressureListener.createFromBeans(ImmutableList.of(mockBean), directExecutor());
underTest.setEventBus(eventBus);
verify(mockBean).addNotificationListener(underTest, null, null);
@@ -198,8 +197,7 @@ public void manualGc() {
@Test
public void doesntInvokeHandlerWhenTenuredSpaceMaxSizeIsZero() {
MemoryPressureListener underTest =
- MemoryPressureListener.createFromBeans(
- ImmutableList.of(mockBean), retainedHeapLimiter, directExecutor());
+ MemoryPressureListener.createFromBeans(ImmutableList.of(mockBean), directExecutor());
underTest.setEventBus(eventBus);
verify(mockBean).addNotificationListener(underTest, null, null);
@@ -229,7 +227,7 @@ public void findsTenuredSpaceWithNonZeroMaxSize() {
MemoryPressureListener underTest =
MemoryPressureListener.createFromBeans(
- ImmutableList.of(mockBean, anotherMockBean), retainedHeapLimiter, directExecutor());
+ ImmutableList.of(mockBean, anotherMockBean), directExecutor());
underTest.setEventBus(eventBus);
verify(mockBean).addNotificationListener(underTest, null, null);
verify(anotherMockBean).addNotificationListener(underTest, null, null);
@@ -267,18 +265,21 @@ public void findsTenuredSpaceWithNonZeroMaxSize() {
}
@Test
- public void retainedHeapLimiter_aboveThreshold_handleCrash() {
- MemoryPressureOptions options = Options.getDefaults(MemoryPressureOptions.class);
- options.oomMoreEagerlyThreshold = 90;
- retainedHeapLimiter.setOptions(options);
-
+ public void gcThrashingDetector_limitMet_handleCrash() {
MemoryPressureListener underTest =
MemoryPressureListener.createFromBeans(
- ImmutableList.of(mockUselessBean, mockBean), retainedHeapLimiter, directExecutor());
- underTest.setEventBus(eventBus);
+ ImmutableList.of(mockUselessBean, mockBean), directExecutor());
verify(mockBean).addNotificationListener(underTest, null, null);
verify(mockUselessBean, never()).addNotificationListener(any(), any(), any());
+ underTest.setEventBus(eventBus);
+ underTest.setGcThrashingDetector(
+ new GcThrashingDetector(
+ /* threshold= */ 90,
+ ImmutableList.of(GcThrashingDetector.Limit.of(Duration.ofMinutes(1), 1)),
+ BlazeClock.instance(),
+ bugReporter));
+
GcInfo mockGcInfo = mock(GcInfo.class);
String nonTenuredSpaceName = "nope";
MemoryUsage mockMemoryUsageForNonTenuredSpace = mock(MemoryUsage.class);
@@ -296,6 +297,7 @@ public void retainedHeapLimiter_aboveThreshold_handleCrash() {
MemoryPressureEvent event =
MemoryPressureEvent.newBuilder()
.setWasManualGc(false)
+ .setWasFullGc(true)
.setTenuredSpaceUsedBytes(99)
.setTenuredSpaceMaxBytes(100)
.build();
@@ -303,27 +305,10 @@ public void retainedHeapLimiter_aboveThreshold_handleCrash() {
new Notification(
GarbageCollectionNotificationInfo.GARBAGE_COLLECTION_NOTIFICATION, "test", 123);
notification.setUserData(
- new GarbageCollectionNotificationInfo("gcName", "gcAction", "non-manual", mockGcInfo)
+ new GarbageCollectionNotificationInfo("gcName", "end of major GC", "non-manual", mockGcInfo)
.toCompositeData(null));
underTest.handleNotification(notification, null);
assertThat(events).containsExactly(event);
-
- MemoryPressureEvent manualGcEvent =
- MemoryPressureEvent.newBuilder()
- .setWasManualGc(true)
- .setTenuredSpaceUsedBytes(99)
- .setTenuredSpaceMaxBytes(100)
- .build();
- Notification manualGCNotification =
- new Notification(
- GarbageCollectionNotificationInfo.GARBAGE_COLLECTION_NOTIFICATION, "test", 123);
- manualGCNotification.setUserData(
- new GarbageCollectionNotificationInfo("gcName", "gcAction", "System.gc()", mockGcInfo)
- .toCompositeData(null));
- underTest.handleNotification(manualGCNotification, null);
-
- verify(bugReporter).handleCrash(any(), any());
- assertThat(events).containsExactly(event, manualGcEvent);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiterTest.java b/src/test/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiterTest.java
deleted file mode 100644
index 632b309e778cc3..00000000000000
--- a/src/test/java/com/google/devtools/build/lib/runtime/RetainedHeapLimiterTest.java
+++ /dev/null
@@ -1,346 +0,0 @@
-// 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.runtime;
-
-import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.truth.Truth.assertThat;
-import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.verifyNoInteractions;
-import static org.mockito.Mockito.verifyNoMoreInteractions;
-
-import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.bugreport.BugReporter;
-import com.google.devtools.build.lib.bugreport.Crash;
-import com.google.devtools.build.lib.runtime.GcThrashingDetector.Limit;
-import com.google.devtools.build.lib.runtime.MemoryPressure.MemoryPressureStats;
-import com.google.devtools.build.lib.testutil.ManualClock;
-import com.google.devtools.common.options.Options;
-import com.google.testing.junit.testparameterinjector.TestParameterInjector;
-import java.lang.ref.WeakReference;
-import java.time.Duration;
-import org.junit.After;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.mockito.ArgumentCaptor;
-import org.mockito.ArgumentMatchers;
-
-/** Tests for {@link RetainedHeapLimiter}. */
-@RunWith(TestParameterInjector.class)
-public final class RetainedHeapLimiterTest {
-
- private final BugReporter bugReporter = mock(BugReporter.class);
- private final ManualClock clock = new ManualClock();
- private final MemoryPressureOptions options = Options.getDefaults(MemoryPressureOptions.class);
-
- private final RetainedHeapLimiter underTest =
- RetainedHeapLimiter.createForTest(bugReporter, clock);
-
- @Before
- public void setClock() {
- clock.advanceMillis(100000);
- }
-
- @After
- public void verifyNoMoreBugReports() {
- verifyNoMoreInteractions(bugReporter);
- }
-
- @Test
- public void underThreshold_noOom() {
- options.oomMoreEagerlyThreshold = 99;
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(100));
- underTest.handle(percentUsedAfterManualGc(89));
-
- verifyNoInteractions(bugReporter);
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1));
- }
-
- @Test
- public void overThreshold_oom() {
- options.oomMoreEagerlyThreshold = 90;
- underTest.setOptions(options);
-
- // Triggers GC, and tells RetainedHeapLimiter to OOM if too much memory used next time.
- underTest.handle(percentUsedAfterOrganicFullGc(91));
-
- underTest.handle(percentUsedAfterManualGc(91));
-
- ArgumentCaptor crashArgument = ArgumentCaptor.forClass(Crash.class);
- verify(bugReporter).handleCrash(crashArgument.capture(), ArgumentMatchers.any());
- OutOfMemoryError oom = (OutOfMemoryError) crashArgument.getValue().getThrowable();
- assertThat(oom).hasMessageThat().contains("forcing exit due to GC thrashing");
- assertThat(oom).hasMessageThat().contains("tenured space is more than 90% occupied");
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1));
- }
-
- @Test
- public void inactiveAfterOom() {
- options.oomMoreEagerlyThreshold = 90;
- options.minTimeBetweenTriggeredGc = Duration.ZERO;
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterManualGc(91));
- verify(bugReporter).handleCrash(any(), any());
-
- // No more GC or bug reports even if notifications come in after an OOM is in progress.
- WeakReference ref = new WeakReference<>(new Object());
- clock.advanceMillis(Duration.ofMinutes(1).toMillis());
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterManualGc(91));
- assertThat(ref.get()).isNotNull();
- verifyNoMoreBugReports();
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1));
- }
-
- @Test
- public void externalGcNoTrigger() {
- options.oomMoreEagerlyThreshold = 90;
- underTest.setOptions(options);
-
- // No trigger because cause was "System.gc()".
- underTest.handle(percentUsedAfterManualGc(91));
-
- // Proof: no OOM.
- underTest.handle(percentUsedAfterManualGc(91));
- verifyNoInteractions(bugReporter);
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0));
- }
-
- @Test
- public void triggerReset() {
- options.oomMoreEagerlyThreshold = 90;
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
-
- // Got under the threshold, so no OOM.
- underTest.handle(percentUsedAfterManualGc(89));
-
- // No OOM this time since wasn't triggered.
- underTest.handle(percentUsedAfterManualGc(91));
- verifyNoInteractions(bugReporter);
- }
-
- @Test
- public void triggerRaceWithOtherGc() {
- options.oomMoreEagerlyThreshold = 90;
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterManualGc(91));
-
- ArgumentCaptor crashArgument = ArgumentCaptor.forClass(Crash.class);
- verify(bugReporter).handleCrash(crashArgument.capture(), ArgumentMatchers.any());
- assertThat(crashArgument.getValue().getThrowable()).isInstanceOf(OutOfMemoryError.class);
- }
-
- @Test
- public void minTimeBetweenGc_lessThan_noGc() {
- options.oomMoreEagerlyThreshold = 90;
- options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1);
- underTest.setOptions(options);
- WeakReference ref = new WeakReference<>(new Object());
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- assertThat(ref.get()).isNull();
- underTest.handle(percentUsedAfterManualGc(89));
-
- ref = new WeakReference<>(new Object());
- clock.advanceMillis(Duration.ofSeconds(59).toMillis());
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- assertThat(ref.get()).isNotNull();
-
- assertStats(
- MemoryPressureStats.newBuilder()
- .setManuallyTriggeredGcs(1)
- .setMaxConsecutiveIgnoredGcsOverThreshold(1));
- }
-
- @Test
- public void minTimeBetweenGc_greaterThan_gc() {
- options.oomMoreEagerlyThreshold = 90;
- options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1);
- underTest.setOptions(options);
- WeakReference ref = new WeakReference<>(new Object());
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- assertThat(ref.get()).isNull();
- underTest.handle(percentUsedAfterManualGc(89));
-
- ref = new WeakReference<>(new Object());
- clock.advanceMillis(Duration.ofSeconds(61).toMillis());
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- assertThat(ref.get()).isNull();
-
- assertStats(
- MemoryPressureStats.newBuilder()
- .setManuallyTriggeredGcs(2)
- .setMaxConsecutiveIgnoredGcsOverThreshold(0));
- }
-
- @Test
- public void gcLockerDefersManualGc_timeoutCancelled() {
- options.oomMoreEagerlyThreshold = 90;
- options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1);
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- WeakReference ref = new WeakReference<>(new Object());
- underTest.handle(percentUsedAfterGcLockerGc(91));
- assertThat(ref.get()).isNull();
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(2));
- }
-
- @Test
- public void gcLockerAfterSuccessfulManualGc_timeoutPreserved() {
- options.oomMoreEagerlyThreshold = 90;
- options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1);
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterManualGc(89));
- WeakReference ref = new WeakReference<>(new Object());
- underTest.handle(percentUsedAfterGcLockerGc(91));
- assertThat(ref.get()).isNotNull();
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1));
- }
-
- @Test
- public void reportsMaxConsecutiveIgnored() {
- options.oomMoreEagerlyThreshold = 90;
- options.minTimeBetweenTriggeredGc = Duration.ofMinutes(1);
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterManualGc(89));
- for (int i = 0; i < 6; i++) {
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- }
-
- clock.advanceMillis(Duration.ofMinutes(2).toMillis());
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterManualGc(89));
- for (int i = 0; i < 8; i++) {
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- }
- underTest.handle(percentUsedAfterOrganicFullGc(89)); // Breaks the streak of over threshold GCs.
- underTest.handle(percentUsedAfterOrganicFullGc(91));
-
- clock.advanceMillis(Duration.ofMinutes(2).toMillis());
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- underTest.handle(percentUsedAfterOrganicFullGc(89));
- for (int i = 0; i < 7; i++) {
- underTest.handle(percentUsedAfterOrganicFullGc(91));
- }
-
- assertStats(
- MemoryPressureStats.newBuilder()
- .setManuallyTriggeredGcs(3)
- .setMaxConsecutiveIgnoredGcsOverThreshold(8));
- }
-
- @Test
- public void threshold100_noGcTriggeredEvenWithNonsenseStats() {
- options.oomMoreEagerlyThreshold = 100;
- underTest.setOptions(options);
- WeakReference ref = new WeakReference<>(new Object());
-
- underTest.handle(percentUsedAfterOrganicFullGc(101));
- assertThat(ref.get()).isNotNull();
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0));
- }
-
- @Test
- public void optionsNotSet_disabled() {
- underTest.handle(percentUsedAfterOrganicFullGc(99));
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0));
- }
-
- @Test
- public void gcThrashingLimitsSet_mutuallyExclusive_disabled() {
- options.oomMoreEagerlyThreshold = 90;
- options.gcThrashingLimits = ImmutableList.of(Limit.of(Duration.ofMinutes(1), 2));
- options.gcThrashingLimitsRetainedHeapLimiterMutuallyExclusive = true;
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(99));
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0));
- }
-
- @Test
- public void gcThrashingLimitsSet_mutuallyInclusive_enabled() {
- options.oomMoreEagerlyThreshold = 90;
- options.gcThrashingLimits = ImmutableList.of(Limit.of(Duration.ofMinutes(1), 2));
- options.gcThrashingLimitsRetainedHeapLimiterMutuallyExclusive = false;
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(99));
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1));
- }
-
- @Test
- public void statsReset() {
- options.oomMoreEagerlyThreshold = 90;
- underTest.setOptions(options);
-
- underTest.handle(percentUsedAfterOrganicFullGc(91));
-
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(1));
- assertStats(MemoryPressureStats.newBuilder().setManuallyTriggeredGcs(0));
- }
-
- private static MemoryPressureEvent percentUsedAfterManualGc(int percentUsed) {
- return percentUsedAfterGc(percentUsed).setWasManualGc(true).setWasFullGc(true).build();
- }
-
- private static MemoryPressureEvent percentUsedAfterOrganicFullGc(int percentUsed) {
- return percentUsedAfterGc(percentUsed).setWasFullGc(true).build();
- }
-
- private static MemoryPressureEvent percentUsedAfterGcLockerGc(int percentUsed) {
- return percentUsedAfterGc(percentUsed).setWasGcLockerInitiatedGc(true).build();
- }
-
- private static MemoryPressureEvent.Builder percentUsedAfterGc(int percentUsed) {
- checkArgument(percentUsed >= 0, percentUsed);
- return MemoryPressureEvent.newBuilder()
- .setTenuredSpaceUsedBytes(percentUsed)
- .setTenuredSpaceMaxBytes(100L);
- }
-
- private void assertStats(MemoryPressureStats.Builder expected) {
- MemoryPressureStats.Builder stats = MemoryPressureStats.newBuilder();
- underTest.addStatsAndReset(stats);
- assertThat(stats.build()).isEqualTo(expected.build());
- }
-}
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
index 67e17504c6c1b3..dbbfc96f4b5e73 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
@@ -870,6 +870,11 @@ public void testAttrDoc(
Attribute documented =
buildAttribute("documented", String.format("attr.%s(doc='foo')", attrType));
assertThat(documented.getDoc()).isEqualTo("foo");
+ Attribute documentedNeedingDedent =
+ buildAttribute(
+ "documented",
+ String.format("attr.%s(doc='''foo\n\n More details.\n ''')", attrType));
+ assertThat(documentedNeedingDedent.getDoc()).isEqualTo("foo\n\nMore details.");
Attribute undocumented = buildAttribute("undocumented", String.format("attr.%s()", attrType));
assertThat(undocumented.getDoc()).isNull();
}
@@ -899,11 +904,21 @@ public void testRuleDoc() throws Exception {
ev,
"def impl(ctx):",
" return None",
- "documented_rule = rule(impl, doc='My doc string')",
+ "documented_rule = rule(impl, doc = 'My doc string')",
+ "long_documented_rule = rule(",
+ " impl,",
+ " doc = '''Long doc",
+ "",
+ " With details",
+ "''',",
+ ")",
"undocumented_rule = rule(impl)");
StarlarkRuleFunction documentedRule = (StarlarkRuleFunction) ev.lookup("documented_rule");
+ StarlarkRuleFunction longDocumentedRule =
+ (StarlarkRuleFunction) ev.lookup("long_documented_rule");
StarlarkRuleFunction undocumentedRule = (StarlarkRuleFunction) ev.lookup("undocumented_rule");
assertThat(documentedRule.getDocumentation()).hasValue("My doc string");
+ assertThat(longDocumentedRule.getDocumentation()).hasValue("Long doc\n\nWith details");
assertThat(undocumentedRule.getDocumentation()).isEmpty();
}
@@ -1720,10 +1735,13 @@ public void declaredProviderDocumentation() throws Exception {
evalAndExport(
ev,
"UndocumentedInfo = provider()",
- "DocumentedInfo = provider(doc = 'My documented provider')",
+ "DocumentedInfo = provider(doc = '''",
+ " My documented provider",
+ "",
+ " Details''')",
// Note fields below are not alphabetized
"SchemafulWithoutDocsInfo = provider(fields = ['b', 'a'])",
- "SchemafulWithDocsInfo = provider(fields = {'b': 'Field b', 'a': 'Field a'})");
+ "SchemafulWithDocsInfo = provider(fields = {'b': 'Field b', 'a': 'Field\\n a'})");
StarlarkProvider undocumentedInfo = (StarlarkProvider) ev.lookup("UndocumentedInfo");
StarlarkProvider documentedInfo = (StarlarkProvider) ev.lookup("DocumentedInfo");
@@ -1732,11 +1750,11 @@ public void declaredProviderDocumentation() throws Exception {
StarlarkProvider schemafulWithDocsInfo = (StarlarkProvider) ev.lookup("SchemafulWithDocsInfo");
assertThat(undocumentedInfo.getDocumentation()).isEmpty();
- assertThat(documentedInfo.getDocumentation()).hasValue("My documented provider");
+ assertThat(documentedInfo.getDocumentation()).hasValue("My documented provider\n\nDetails");
assertThat(schemafulWithoutDocsInfo.getSchema())
.containsExactly("b", Optional.empty(), "a", Optional.empty());
assertThat(schemafulWithDocsInfo.getSchema())
- .containsExactly("b", Optional.of("Field b"), "a", Optional.of("Field a"));
+ .containsExactly("b", Optional.of("Field b"), "a", Optional.of("Field\na"));
}
@Test
@@ -2199,10 +2217,20 @@ public void aspectDoc() throws Exception {
"def _impl(target, ctx):", //
" pass",
"documented_aspect = aspect(_impl, doc='My doc string')",
+ "long_documented_aspect = aspect(",
+ " _impl,",
+ " doc='''",
+ " My doc string",
+ " ",
+ " With details''',",
+ ")",
"undocumented_aspect = aspect(_impl)");
StarlarkDefinedAspect documentedAspect = (StarlarkDefinedAspect) ev.lookup("documented_aspect");
assertThat(documentedAspect.getDocumentation()).hasValue("My doc string");
+ StarlarkDefinedAspect longDocumentedAspect =
+ (StarlarkDefinedAspect) ev.lookup("long_documented_aspect");
+ assertThat(longDocumentedAspect.getDocumentation()).hasValue("My doc string\n\nWith details");
StarlarkDefinedAspect undocumentedAspect =
(StarlarkDefinedAspect) ev.lookup("undocumented_aspect");
assertThat(undocumentedAspect.getDocumentation()).isEmpty();
diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/function_wrap_multiple_lines_test/golden.textproto b/src/test/java/com/google/devtools/build/skydoc/testdata/function_wrap_multiple_lines_test/golden.textproto
index 20846fe4f06857..e3255e3c534f38 100644
--- a/src/test/java/com/google/devtools/build/skydoc/testdata/function_wrap_multiple_lines_test/golden.textproto
+++ b/src/test/java/com/google/devtools/build/skydoc/testdata/function_wrap_multiple_lines_test/golden.textproto
@@ -123,7 +123,7 @@ rule_info {
}
attribute {
name: "deps"
- doc_string: "\nThe dependencies to use. Defaults to the most recent ANTLR 3 release,\nbut if you need to use a different version, you can specify the\ndependencies here.\n"
+ doc_string: "The dependencies to use. Defaults to the most recent ANTLR 3 release,\nbut if you need to use a different version, you can specify the\ndependencies here."
type: LABEL_LIST
default_value: "[Label(\"@antlr3_runtimes//:tool\")]"
}
diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/misc_apis_test/golden.textproto b/src/test/java/com/google/devtools/build/skydoc/testdata/misc_apis_test/golden.textproto
index ff7dbce0f141f4..3a7f865d521330 100644
--- a/src/test/java/com/google/devtools/build/skydoc/testdata/misc_apis_test/golden.textproto
+++ b/src/test/java/com/google/devtools/build/skydoc/testdata/misc_apis_test/golden.textproto
@@ -9,7 +9,7 @@ rule_info {
}
attribute {
name: "deps"
- doc_string: "\nA list of dependencies.\n"
+ doc_string: "A list of dependencies."
type: LABEL_LIST
provider_name_group {
provider_name: "MyInfo"
diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/provider_basic_test/golden.textproto b/src/test/java/com/google/devtools/build/skydoc/testdata/provider_basic_test/golden.textproto
index 14c7b778a64460..0f82fdc11fb27a 100644
--- a/src/test/java/com/google/devtools/build/skydoc/testdata/provider_basic_test/golden.textproto
+++ b/src/test/java/com/google/devtools/build/skydoc/testdata/provider_basic_test/golden.textproto
@@ -15,7 +15,7 @@ provider_info {
}
provider_info {
provider_name: "MyVeryDocumentedInfo"
- doc_string: "\nA provider with some really neat documentation.\nLook on my works, ye mighty, and despair!\n"
+ doc_string: "A provider with some really neat documentation.\nLook on my works, ye mighty, and despair!"
field_info {
name: "favorite_food"
doc_string: "A string representing my favorite food"
diff --git a/src/test/java/com/google/devtools/build/skydoc/testdata/unknown_predefines_test/golden.textproto b/src/test/java/com/google/devtools/build/skydoc/testdata/unknown_predefines_test/golden.textproto
index 00e84866d2bbb7..6936c857c0484a 100644
--- a/src/test/java/com/google/devtools/build/skydoc/testdata/unknown_predefines_test/golden.textproto
+++ b/src/test/java/com/google/devtools/build/skydoc/testdata/unknown_predefines_test/golden.textproto
@@ -38,4 +38,4 @@ func_info {
mandatory: true
}
}
-module_docstring: "\nTests that the documentation is generated even when unknown modules, calls, or providers are used.\n"
+module_docstring: "Tests that the documentation is generated even when unknown modules, calls, or providers are used."
diff --git a/src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java b/src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java
index 31b534c67a75d9..1b5bade699565f 100644
--- a/src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java
+++ b/src/test/java/com/google/devtools/build/skyframe/DeterministicInMemoryGraph.java
@@ -80,8 +80,8 @@ public void parallelForEach(Consumer consumer) {
}
@Override
- public void cleanupInterningPool() {
- ((InMemoryGraph) delegate).cleanupInterningPool();
+ public void cleanupInterningPools() {
+ ((InMemoryGraph) delegate).cleanupInterningPools();
}
@Override
diff --git a/src/test/java/com/google/devtools/build/skyframe/InMemoryGraphTest.java b/src/test/java/com/google/devtools/build/skyframe/InMemoryGraphTest.java
index 8ec297fbd0af27..339b4d28bc6163 100644
--- a/src/test/java/com/google/devtools/build/skyframe/InMemoryGraphTest.java
+++ b/src/test/java/com/google/devtools/build/skyframe/InMemoryGraphTest.java
@@ -121,7 +121,7 @@ public void cleanupPool_weakInternerReintern() throws InterruptedException {
assertThat(graph.get(null, Reason.OTHER, cat)).isNotNull();
assertThat(graph).isInstanceOf(InMemoryGraphImpl.class);
- ((InMemoryGraphImpl) graph).cleanupInterningPool();
+ ((InMemoryGraphImpl) graph).cleanupInterningPools();
// When re-creating a cat SkyKeyWithSkyKeyInterner, we expect to get the original instance. Pool
// cleaning up re-interns the cat instance back to the weak interner, and thus, no new instance
diff --git a/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java b/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java
index e9ba9568b711fe..ad35a7f513d9b8 100644
--- a/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java
+++ b/src/test/java/com/google/devtools/build/skyframe/NotifyingInMemoryGraph.java
@@ -87,8 +87,8 @@ public void parallelForEach(Consumer consumer) {
}
@Override
- public void cleanupInterningPool() {
- ((InMemoryGraph) delegate).cleanupInterningPool();
+ public void cleanupInterningPools() {
+ ((InMemoryGraph) delegate).cleanupInterningPools();
}
@Override
diff --git a/src/test/java/net/starlark/java/eval/BUILD b/src/test/java/net/starlark/java/eval/BUILD
index 82386955da80e7..3249dc9e1dea0b 100644
--- a/src/test/java/net/starlark/java/eval/BUILD
+++ b/src/test/java/net/starlark/java/eval/BUILD
@@ -24,6 +24,7 @@ java_test(
"MutabilityTest.java",
"PrinterTest.java",
"StarlarkAnnotationsTest.java",
+ "StarlarkClassTest.java",
"StarlarkEvaluationTest.java",
"StarlarkFlagGuardingTest.java",
"StarlarkListTest.java",
diff --git a/src/test/java/net/starlark/java/eval/EvalTests.java b/src/test/java/net/starlark/java/eval/EvalTests.java
index ab364c13d6e83a..63105613cf7b09 100644
--- a/src/test/java/net/starlark/java/eval/EvalTests.java
+++ b/src/test/java/net/starlark/java/eval/EvalTests.java
@@ -26,6 +26,7 @@
MethodLibraryTest.class,
MutabilityTest.class,
PrinterTest.class,
+ StarlarkClassTest.class,
StarlarkEvaluationTest.class,
StarlarkFlagGuardingTest.class,
StarlarkAnnotationsTest.class,
diff --git a/src/test/java/net/starlark/java/eval/EvaluationTest.java b/src/test/java/net/starlark/java/eval/EvaluationTest.java
index 2f4c37fc8c8b7f..241110df864cf4 100644
--- a/src/test/java/net/starlark/java/eval/EvaluationTest.java
+++ b/src/test/java/net/starlark/java/eval/EvaluationTest.java
@@ -791,7 +791,8 @@ public void moduleWithDocString() throws Exception {
assertThat(module.getDocumentation()).isNull();
ParserInput input =
ParserInput.fromLines(
- "\"\"\"Module doc header", //
+ "\"\"\"",
+ "Module doc header", //
"",
"Module doc details",
"\"\"\"",
@@ -805,7 +806,7 @@ public void moduleWithDocString() throws Exception {
StarlarkThread thread = new StarlarkThread(mu, StarlarkSemantics.DEFAULT);
Starlark.execFile(input, FileOptions.DEFAULT, module, thread);
}
- assertThat(module.getDocumentation()).isEqualTo("Module doc header\n\nModule doc details\n");
+ assertThat(module.getDocumentation()).isEqualTo("Module doc header\n\nModule doc details");
}
@Test
diff --git a/src/test/java/net/starlark/java/eval/StarlarkClassTest.java b/src/test/java/net/starlark/java/eval/StarlarkClassTest.java
new file mode 100644
index 00000000000000..6acb968a1cdeb9
--- /dev/null
+++ b/src/test/java/net/starlark/java/eval/StarlarkClassTest.java
@@ -0,0 +1,93 @@
+// Copyright 2023 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 net.starlark.java.eval;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertThrows;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for static utility methods in the {@link Starlark} class. */
+@RunWith(JUnit4.class)
+public final class StarlarkClassTest {
+
+ @Test
+ public void trimDocString() {
+ // See https://peps.python.org/pep-0257/#handling-docstring-indentation
+ // Single line
+ assertThat(Starlark.trimDocString("")).isEmpty();
+ assertThat(Starlark.trimDocString(" ")).isEmpty();
+ assertThat(Starlark.trimDocString("\t\t\t")).isEmpty();
+ assertThat(Starlark.trimDocString("Hello world")).isEqualTo("Hello world");
+ assertThat(Starlark.trimDocString(" Hello world ")).isEqualTo("Hello world");
+ // First line is always fully trimmed, regardless of subsequent indentation levels.
+ assertThat(Starlark.trimDocString(" Hello\t\nworld")).isEqualTo("Hello\nworld");
+ assertThat(Starlark.trimDocString(" Hello \n world")).isEqualTo("Hello\nworld");
+ assertThat(Starlark.trimDocString(" Hello \n world")).isEqualTo("Hello\nworld");
+ // Subsequent lines are dedented to their minimal indentation level and fully right-trimmed
+ assertThat(Starlark.trimDocString(" Hello \n world \n and \n good-bye "))
+ .isEqualTo("Hello\n world\nand\n good-bye");
+ // ... and the first line's indentation does not affect minimal indentation level computation.
+ assertThat(Starlark.trimDocString(" Hello\n world\n and\n good-bye"))
+ .isEqualTo("Hello\n world\n and\ngood-bye");
+ // Blank lines are trimmed and do not affect indentation level computation
+ assertThat(
+ Starlark.trimDocString(
+ " Hello \n\n world \n\n\n \n and \n \n good-bye "))
+ .isEqualTo("Hello\n\n world\n\n\n\nand\n\n good-bye");
+ // Windows-style \r\n is simplified to \n
+ assertThat(Starlark.trimDocString("Hello\r\nworld")).isEqualTo("Hello\nworld");
+ assertThat(Starlark.trimDocString("Hello\r\n\r\nworld")).isEqualTo("Hello\n\nworld");
+ // Leading and trailing blank lines are removed
+ assertThat(Starlark.trimDocString("\n\n\n")).isEmpty();
+ assertThat(Starlark.trimDocString("\r\n\r\n\r\n")).isEmpty();
+ assertThat(Starlark.trimDocString("\n \n \n ")).isEmpty();
+ assertThat(Starlark.trimDocString("\n \r\n \r\n ")).isEmpty();
+ assertThat(Starlark.trimDocString("\n\r\nHello world\n\r\n")).isEqualTo("Hello world");
+ assertThat(Starlark.trimDocString("\n\n \nHello\nworld\n\n \n\t")).isEqualTo("Hello\nworld");
+ assertThat(Starlark.trimDocString("\n\n \t\nHello\n world\n \n")).isEqualTo("Hello\n world");
+ // Tabs are expanded to size 8 (following Python convention)
+ assertThat(Starlark.trimDocString("Hello\tworld")).isEqualTo("Hello world");
+ assertThat(Starlark.trimDocString("\n\tHello\n\t\tworld")).isEqualTo("Hello\n world");
+ assertThat(Starlark.trimDocString("\n \tHello\n\t\tworld")).isEqualTo("Hello\n world");
+ assertThat(Starlark.trimDocString("\n Hello\n\tworld")).isEqualTo("Hello\n world");
+ }
+
+ @Test
+ public void expandTabs() {
+ assertThat(Starlark.expandTabs("", 8)).isEmpty();
+ assertThat(Starlark.expandTabs("Hello\nworld", 8)).isEqualTo("Hello\nworld");
+
+ assertThat(Starlark.expandTabs("\t", 1)).isEqualTo(" ");
+ assertThat(Starlark.expandTabs("\t", 2)).isEqualTo(" ");
+ assertThat(Starlark.expandTabs(" \t", 2)).isEqualTo(" ");
+ assertThat(Starlark.expandTabs("\t", 8)).isEqualTo(" ");
+ assertThat(Starlark.expandTabs(" \t", 8)).isEqualTo(" ");
+
+ assertThat(Starlark.expandTabs("01\t012\t0123\t01234", 4)).isEqualTo("01 012 0123 01234");
+ assertThat(Starlark.expandTabs("01\t012\t0123\t01234", 8))
+ .isEqualTo("01 012 0123 01234");
+
+ assertThat(Starlark.expandTabs("01\t\n\t012\t0123\t01234", 4))
+ .isEqualTo("01 \n 012 0123 01234");
+ assertThat(Starlark.expandTabs("\r01\r\n\t012\r\n\t0123\t01234\n", 8))
+ .isEqualTo("\r01\r\n 012\r\n 0123 01234\n");
+
+ assertThrows(IllegalArgumentException.class, () -> Starlark.expandTabs("\t", 0));
+ assertThrows(IllegalArgumentException.class, () -> Starlark.expandTabs("\t", -1));
+ }
+}
diff --git a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py
index 5a65b9cd8f4960..c32311812fb4cc 100644
--- a/src/test/py/bazel/bzlmod/bazel_lockfile_test.py
+++ b/src/test/py/bazel/bzlmod/bazel_lockfile_test.py
@@ -763,6 +763,48 @@ def testChangeEnvVariableInErrorMode(self):
stderr,
)
+ def testModuleExtensionWithFile(self):
+ self.ScratchFile(
+ 'MODULE.bazel',
+ [
+ 'lockfile_ext = use_extension("extension.bzl", "lockfile_ext")',
+ 'use_repo(lockfile_ext, "hello")',
+ ],
+ )
+ self.ScratchFile('BUILD.bazel')
+ self.ScratchFile(
+ 'extension.bzl',
+ [
+ 'def _repo_rule_impl(ctx):',
+ ' ctx.file("WORKSPACE")',
+ ' ctx.file("BUILD", "filegroup(name=\'lala\')")',
+ '',
+ 'repo_rule = repository_rule(implementation=_repo_rule_impl)',
+ '',
+ 'def _module_ext_impl(ctx):',
+ ' print(ctx.read(Label("//:hello.txt")))',
+ ' repo_rule(name="hello")',
+ '',
+ 'lockfile_ext = module_extension(',
+ ' implementation=_module_ext_impl',
+ ')',
+ ],
+ )
+
+ self.ScratchFile('hello.txt', ['I will not stay the same.'])
+ _, _, stderr = self.RunBazel(['build', '@hello//:all'])
+ self.assertIn('I will not stay the same.', ''.join(stderr))
+
+ # Shutdown bazel to empty cache and run with no changes
+ self.RunBazel(['shutdown'])
+ _, _, stderr = self.RunBazel(['build', '@hello//:all'])
+ self.assertNotIn('I will not stay the same.', ''.join(stderr))
+
+ # Update file and rerun
+ self.ScratchFile('hello.txt', ['I have changed now!'])
+ _, _, stderr = self.RunBazel(['build', '@hello//:all'])
+ self.assertIn('I have changed now!', ''.join(stderr))
+
if __name__ == '__main__':
unittest.main()
diff --git a/src/test/shell/BUILD b/src/test/shell/BUILD
index 85de0e15be0842..780915b2416ff8 100644
--- a/src/test/shell/BUILD
+++ b/src/test/shell/BUILD
@@ -45,7 +45,10 @@ sh_library(
"unittest.bash",
"unittest_utils.sh",
],
- data = [":testenv.sh"],
+ data = [
+ ":testenv.sh",
+ "@bazel_tools//tools/bash/runfiles",
+ ],
visibility = ["//visibility:public"],
)
diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD
index 670a46d64af614..b1c7b338ad0773 100644
--- a/src/test/shell/bazel/BUILD
+++ b/src/test/shell/bazel/BUILD
@@ -22,14 +22,6 @@ gen_workspace_stanza(
],
)
-gen_workspace_stanza(
- name = "rules_proto_stanza",
- out = "rules_proto_stanza.txt",
- repos = [
- "rules_proto",
- ],
-)
-
filegroup(
name = "test-deps",
testonly = 1,
@@ -346,10 +338,7 @@ sh_test(
name = "bazel_proto_library_test",
size = "large", # Downloads and compiles protobuf for *every* *test* *case*
srcs = ["bazel_proto_library_test.sh"],
- data = [
- ":rules_proto_stanza.txt",
- ":test-deps",
- ],
+ data = [":test-deps"],
exec_compatible_with = ["//:highcpu_machine"],
tags = ["no_windows"], # Doesn't work on Windows for unknown reason
)
diff --git a/src/test/shell/bazel/bazel_proto_library_test.sh b/src/test/shell/bazel/bazel_proto_library_test.sh
index 015ed45e691a58..b6ab60089838e7 100755
--- a/src/test/shell/bazel/bazel_proto_library_test.sh
+++ b/src/test/shell/bazel/bazel_proto_library_test.sh
@@ -48,8 +48,6 @@ function write_workspace() {
mkdir -p "$workspace"
fi
- cat $(rlocation io_bazel/src/tests/shell/bazel/rules_proto_stanza.txt) >> "$workspace"WORKSPACE
-
cat >> "$workspace"WORKSPACE << EOF
load("@rules_proto//proto:repositories.bzl", "rules_proto_dependencies", "rules_proto_toolchains")
rules_proto_dependencies()
diff --git a/src/test/shell/integration/aspect_test.sh b/src/test/shell/integration/aspect_test.sh
index 367636e92ddc3c..961a3f5c9e8805 100755
--- a/src/test/shell/integration/aspect_test.sh
+++ b/src/test/shell/integration/aspect_test.sh
@@ -1466,4 +1466,30 @@ EOF
expect_log "Attr '\$tool' declares a transition for non-existent exec group 'exec_gp'"
}
+function test_aspect_with_missing_attr() {
+ local package="test"
+ mkdir -p "${package}"
+
+ cat > "${package}/BUILD" < "${package}/a.bzl" < $TEST_log && fail "Build succeeded, expected to fail"
+
+ expect_not_log "IllegalStateException"
+ expect_log "no such package 'missing'"
+}
+
run_suite "Tests for aspects"
diff --git a/src/test/shell/integration_test_setup.sh b/src/test/shell/integration_test_setup.sh
index bbf862e837bdea..17e3cea4bc1f5d 100755
--- a/src/test/shell/integration_test_setup.sh
+++ b/src/test/shell/integration_test_setup.sh
@@ -20,7 +20,20 @@ function print_message_and_exit() {
}
if type rlocation >&/dev/null; then
- # If rlocation is defined, use it to look up data-dependencies.
+ # An incomplete rlocation function is defined in Bazel's test-setup.sh script,
+ # load the actual Bash runfiles library from @bazel_tools//tools/bash/runfiles
+ # to make sure repo mappings is respected.
+ # --- begin runfiles.bash initialization v3 ---
+ # Copy-pasted from the Bazel Bash runfiles library v3.
+ set -uo pipefail; set +e; f=bazel_tools/tools/bash/runfiles/runfiles.bash
+ source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \
+ source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \
+ source "$0.runfiles/$f" 2>/dev/null || \
+ source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
+ source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
+ { echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e
+ # --- end runfiles.bash initialization v3 ---
+
# Load the unit test framework
source "$(rlocation io_bazel/src/test/shell/unittest.bash)" \
|| print_message_and_exit "unittest.bash not found!"
diff --git a/src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java b/src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java
index 222276ad929945..791ad657afc789 100644
--- a/src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java
+++ b/src/tools/starlark/java/com/google/devtools/starlark/common/DocstringUtils.java
@@ -20,7 +20,9 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
+import java.util.Iterator;
import java.util.List;
+import java.util.Locale;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
@@ -30,7 +32,7 @@ public final class DocstringUtils {
private DocstringUtils() {} // uninstantiable
/**
- * Parses a docstring.
+ * Parses a trimmed docstring.
*
* The format of the docstring is as follows
*
@@ -58,7 +60,12 @@ private DocstringUtils() {} // uninstantiable
* """
* }
*
- * @param docstring a docstring of the format described above
+ *
We expect the docstring to already be trimmed and dedented to a minimal common indentation
+ * level by {@link Starlark#trimDocString} or an equivalent PEP-257 style trim() implementation;
+ * note that {@link StarlarkFunction#getDocumentation} returns a correctly trimmed and dedented
+ * doc string.
+ *
+ * @param doc a docstring of the format described above
* @param parseErrors a list to which parsing error messages are written
* @return the parsed docstring information
*/
@@ -166,64 +173,35 @@ public String getDescription() {
}
private static class DocstringParser {
- private final String docstring;
- /** Start offset of the current line. */
- private int startOfLineOffset = 0;
- /** End offset of the current line. */
- private int endOfLineOffset = 0;
- /** Current line number within the docstring. */
- private int lineNumber = 0;
/**
- * The indentation of the docstring literal in the source file.
- *
- *
Every line except the first one must be indented by at least that many spaces.
+ * Current line number within the docstring, 1-based; 0 indicates that parsing has not started;
+ * {@code lineNumber > lines.size()} indicates EOF.
*/
- private int baselineIndentation = 0;
+ private int lineNumber = 0;
+
/** Whether there was a blank line before the current line. */
private boolean blankLineBefore = false;
+
/** Whether we've seen a special section, e.g. 'Args:', already. */
private boolean specialSectionsStarted = false;
- /** List of all parsed lines in the docstring so far, including all indentation. */
- private ArrayList originalLines = new ArrayList<>();
- /**
- * The current line in the docstring with the baseline indentation removed.
- *
- * If the indentation of a docstring line is less than the expected {@link
- * #baselineIndentation}, only the existing indentation is stripped; none of the remaining
- * characters are cut off.
- */
+
+ /** List of all parsed lines in the docstring. */
+ private final ImmutableList lines;
+
+ /** Iterator over lines. */
+ private final Iterator linesIter;
+
+ /** The current line in the docstring. */
private String line = "";
+
/** Errors that occurred so far. */
private final List errors = new ArrayList<>();
private DocstringParser(String docstring) {
- this.docstring = docstring;
-
- // Infer the indentation level:
- // the smallest amount of leading whitespace
- // common to all non-blank lines except the first.
- int indentation = Integer.MAX_VALUE;
- boolean first = true;
- for (String line : Splitter.on("\n").split(docstring)) {
- // ignore first line
- if (first) {
- first = false;
- continue;
- }
- // count leading spaces
- int i;
- for (i = 0; i < line.length() && line.charAt(i) == ' '; i++) {}
- if (i != line.length()) {
- indentation = Math.min(indentation, i);
- }
- }
- if (indentation == Integer.MAX_VALUE) {
- indentation = 0;
- }
-
+ this.lines = ImmutableList.copyOf(Splitter.on("\n").split(docstring));
+ this.linesIter = lines.iterator();
+ // Load the summary line
nextLine();
- // the indentation is only relevant for the following lines, not the first one:
- this.baselineIndentation = indentation;
}
/**
@@ -232,69 +210,20 @@ private DocstringParser(String docstring) {
* @return whether there are lines remaining to be parsed
*/
private boolean nextLine() {
- if (startOfLineOffset >= docstring.length()) {
- return false;
- }
- blankLineBefore = line.trim().isEmpty();
- startOfLineOffset = endOfLineOffset;
- if (startOfLineOffset >= docstring.length()) {
- // Previous line was the last; previous line had no trailing newline character.
+ if (linesIter.hasNext()) {
+ blankLineBefore = line.trim().isEmpty();
+ line = linesIter.next();
+ lineNumber++;
+ return true;
+ } else {
line = "";
+ lineNumber = lines.size() + 1;
return false;
}
- // If not the first line, advance start past the newline character. In the case where there is
- // no more content, then the previous line was the second-to-last line and this last line is
- // empty.
- if (docstring.charAt(startOfLineOffset) == '\n') {
- startOfLineOffset += 1;
- }
- lineNumber++;
- endOfLineOffset = docstring.indexOf('\n', startOfLineOffset);
- if (endOfLineOffset < 0) {
- endOfLineOffset = docstring.length();
- }
- String originalLine = docstring.substring(startOfLineOffset, endOfLineOffset);
- originalLines.add(originalLine);
- int indentation = getIndentation(originalLine);
- if (endOfLineOffset == docstring.length() && startOfLineOffset != 0) {
- if (!originalLine.trim().isEmpty()) {
- error("closing docstring quote should be on its own line, indented the same as the "
- + "opening quote");
- } else if (indentation != baselineIndentation) {
- error("closing docstring quote should be indented the same as the opening quote");
- }
- }
- if (originalLine.trim().isEmpty()) {
- line = "";
- } else {
- if (indentation < baselineIndentation) {
- error(
- "line indented too little (here: "
- + indentation
- + " spaces; expected: "
- + baselineIndentation
- + " spaces)");
- startOfLineOffset += indentation;
- } else {
- startOfLineOffset += baselineIndentation;
- }
- line = docstring.substring(startOfLineOffset, endOfLineOffset);
- }
- return true;
- }
-
- /**
- * Returns whether the current line is the last one in the docstring.
- *
- * It is possible for both this function and {@link #eof} to return true if all content has
- * been exhausted, or if the last line is empty.
- */
- private boolean onLastLine() {
- return endOfLineOffset >= docstring.length();
}
private boolean eof() {
- return startOfLineOffset >= docstring.length();
+ return lineNumber > lines.size();
}
private static int getIndentation(String line) {
@@ -310,7 +239,7 @@ private void error(String message) {
}
private void error(int lineNumber, String message) {
- errors.add(new DocstringParseError(message, lineNumber, originalLines.get(lineNumber - 1)));
+ errors.add(new DocstringParseError(message, lineNumber, lines.get(lineNumber - 1)));
}
private void parseArgumentSection(
@@ -375,9 +304,7 @@ DocstringInfo parse() {
+ " \n\n"
+ "For more details, please have a look at the documentation.");
}
- if (!(onLastLine() && line.trim().isEmpty())) {
- longDescriptionLines.add(line);
- }
+ longDescriptionLines.add(line);
nextLine();
}
}
@@ -399,7 +326,7 @@ private void checkSectionStart(boolean duplicateSection) {
}
private String checkForNonStandardDeprecation(String line) {
- if (line.toLowerCase().startsWith("deprecated:") || line.contains("DEPRECATED")) {
+ if (line.toLowerCase(Locale.ROOT).startsWith("deprecated:") || line.contains("DEPRECATED")) {
error(
"use a 'Deprecated:' section for deprecations, similar to a 'Returns:' section:\n\n"
+ "Deprecated:\n"
@@ -562,7 +489,10 @@ public String getMessage() {
return message;
}
- /** Returns the line number in the containing Starlark file which contains this error. */
+ /**
+ * Returns the line number (skipping leading blank lines, if any) in the original doc string
+ * which contains this error.
+ */
public int getLineNumber() {
return lineNumber;
}