Merge branch 'master' into patch-1

site/en/docs/user-manual.md

@@ -1715,21 +1715,18 @@ The syntax and the remaining options are exactly like
 ## Running executables {:#running-executables}
 
 The `bazel run` command is similar to `bazel build`, except
-it is used to build _and run_ a single target. Here is a typical session:
+it is used to build _and run_ a single target. Here is a typical session
+(`//java/myapp:myapp` says hello and prints out its args):
 
 <pre>
   % bazel run java/myapp:myapp -- --arg1 --arg2
-  Welcome to Bazel
-  INFO: Loading package: java/myapp
-  INFO: Loading package: foo/bar
-  INFO: Loading complete.  Analyzing...
+  INFO: Analyzed target //java/myapp:myapp (13 packages loaded, 27 targets configured).
   INFO: Found 1 target...
-  ...
   Target //java/myapp:myapp up-to-date:
-    bazel-bin/java/myapp:myapp
-  INFO: Elapsed time: 0.638s, Critical Path: 0.34s
-
-  INFO: Running command line: bazel-bin/java/myapp:myapp --arg1 --arg2
+    bazel-bin/java/myapp/myapp
+  INFO: Elapsed time: 14.290s, Critical Path: 5.54s, ...
+  INFO: Build completed successfully, 4 total actions
+  INFO: Running command line: bazel-bin/java/myapp/myapp &lt;args omitted&gt;
   Hello there
   $EXEC_ROOT/java/myapp/myapp
   --arg1
@@ -1739,7 +1736,8 @@ it is used to build _and run_ a single target. Here is a typical session:
 Note: `--` is needed so that Bazel
 does not interpret `--arg1` and `--arg2` as
 Bazel options, but rather as part of the command line for running the binary.
-(The program being run simply says hello and prints out its args.)
+Additionally, Bazel will avoid logging these arguments to the console in case
+they contain sensitive information.
 
 `bazel run` is similar, but not identical, to directly invoking
 the binary built by Bazel and its behavior is different depending on whether the

src/main/java/com/google/devtools/build/lib/BUILD

@@ -349,7 +349,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:provider_collection",
         "//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
         "//src/main/java/com/google/devtools/build/lib/analysis:template_expansion_exception",
-        "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_action_finished_event",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_artifacts_known_event",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report_action_factory",
         "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context",

src/main/java/com/google/devtools/build/lib/actions/cache/PersistentStringIndexer.java

@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.actions.cache;
 
+import static java.lang.Math.max;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.devtools.build.lib.clock.Clock;
@@ -26,6 +27,7 @@
 import java.io.IOException;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicReferenceArray;
 import javax.annotation.Nullable;
 
 /**
@@ -35,50 +37,52 @@
  * canonicalization mapping. The other direction is handled purely in memory and reconstituted at
  * load-time.
  *
- * <p>Thread-safety is ensured by locking on all mutating operations from the superclass. Read-only
- * operations are not locked, but rather backed by ConcurrentMaps.
+ * <p>Thread-safety is ensured by locking on all mutating operations. Read-only operations are not
+ * locked.
  */
 @ConditionallyThreadSafe // Each instance must be instantiated with a different dataPath.
 final class PersistentStringIndexer implements StringIndexer {
 
-  private static final int INITIAL_ENTRIES = 10000;
+  private static final int INITIAL_CAPACITY = 8192;
 
   /** Instantiates and loads instance of the persistent string indexer. */
   static PersistentStringIndexer create(Path dataPath, Clock clock) throws IOException {
-    PersistentIndexMap persistentIndexMap =
+    PersistentIndexMap stringToInt =
         new PersistentIndexMap(
             dataPath, FileSystemUtils.replaceExtension(dataPath, ".journal"), clock);
-    Map<Integer, String> reverseMapping = new ConcurrentHashMap<>(INITIAL_ENTRIES);
-    for (Map.Entry<String, Integer> entry : persistentIndexMap.entrySet()) {
-      if (reverseMapping.put(entry.getValue(), entry.getKey()) != null) {
+
+    // INITIAL_CAPACITY or the next power of two greater than the size.
+    int capacity = max(INITIAL_CAPACITY, Integer.highestOneBit(stringToInt.size()) << 1);
+
+    var intToString = new AtomicReferenceArray<String>(capacity);
+    for (Map.Entry<String, Integer> entry : stringToInt.entrySet()) {
+      if (intToString.getAndSet(entry.getValue(), entry.getKey()) != null) {
         throw new IOException("Corrupted filename index has duplicate entry: " + entry.getKey());
       }
     }
-    return new PersistentStringIndexer(persistentIndexMap, reverseMapping);
+    return new PersistentStringIndexer(stringToInt, intToString);
   }
 
-  // This is similar to (Synchronized) BiMap.
-  // These maps *must* be weakly threadsafe to ensure thread safety for string indexer as a whole.
-  // Specifically, mutating operations are serialized, but read-only operations may be executed
-  // concurrently with mutators.
+  // These two fields act similarly to a (synchronized) BiMap. Mutating operations are performed in
+  // synchronized blocks. Reads are done lock-free.
   private final PersistentIndexMap stringToInt;
-  private final Map<Integer, String> intToString;
+  private volatile AtomicReferenceArray<String> intToString;
 
   private PersistentStringIndexer(
-      PersistentIndexMap stringToInt, Map<Integer, String> intToString) {
+      PersistentIndexMap stringToInt, AtomicReferenceArray<String> intToString) {
     this.stringToInt = stringToInt;
     this.intToString = intToString;
   }
 
   @Override
   public synchronized void clear() {
     stringToInt.clear();
-    intToString.clear();
+    intToString = new AtomicReferenceArray<>(INITIAL_CAPACITY);
   }
 
   @Override
   public int size() {
-    return intToString.size();
+    return stringToInt.size();
   }
 
   @Override
@@ -94,11 +98,24 @@ public Integer getOrCreateIndex(String s) {
       if (existing != null) {
         return existing; // Another thread won the race.
       }
-      intToString.put(i, s);
+      int capacity = intToString.length();
+      if (i == capacity) {
+        intToString = copyOf(intToString, capacity * 2);
+      }
+      intToString.set(i, s);
       return i;
     }
   }
 
+  private static AtomicReferenceArray<String> copyOf(
+      AtomicReferenceArray<String> oldArray, int newCapacity) {
+    var newArray = new AtomicReferenceArray<String>(newCapacity);
+    for (int j = 0; j < oldArray.length(); j++) {
+      newArray.setPlain(j, oldArray.getPlain(j));
+    }
+    return newArray;
+  }
+
   @Override
   @Nullable
   public Integer getIndex(String s) {
@@ -108,7 +125,11 @@ public Integer getIndex(String s) {
   @Override
   @Nullable
   public String getStringForIndex(Integer i) {
-    return intToString.get(i);
+    if (i < 0) {
+      return null;
+    }
+    var snapshot = intToString;
+    return i < snapshot.length() ? snapshot.get(i) : null;
   }
 
   /** Saves index data to the file. */
@@ -143,7 +164,7 @@ private static final class PersistentIndexMap extends PersistentMap<String, Inte
     private long nextUpdate;
 
     PersistentIndexMap(Path mapFile, Path journalFile, Clock clock) throws IOException {
-      super(VERSION, new ConcurrentHashMap<>(INITIAL_ENTRIES), mapFile, journalFile);
+      super(VERSION, new ConcurrentHashMap<>(INITIAL_CAPACITY), mapFile, journalFile);
       this.clock = clock;
       nextUpdate = clock.nanoTime();
       load(/* failFast= */ true);

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -437,7 +437,6 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
-        "//src/main/java/com/google/devtools/build/skyframe",
         "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
         "//src/main/java/com/google/devtools/common/options",
         "//src/main/java/net/starlark/java/annot",
@@ -2741,16 +2740,6 @@ java_library(
     ],
 )
 
-java_library(
-    name = "test/coverage_action_finished_event",
-    srcs = ["test/CoverageActionFinishedEvent.java"],
-    deps = [
-        "//src/main/java/com/google/devtools/build/lib/buildeventstream",
-        "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
-        "//third_party:guava",
-    ],
-)
-
 java_library(
     name = "test/coverage_artifacts_known_event",
     srcs = ["test/CoverageArtifactsKnownEvent.java"],

src/main/java/com/google/devtools/build/lib/analysis/BuildView.java

@@ -853,32 +853,39 @@ private ImmutableSet<Artifact> memoizedGetCoverageArtifactsHelper(
       EventBus eventBus,
       TargetPatternPhaseValue loadingResult)
       throws InterruptedException {
-    if (memoizedCoverageArtifacts != null) {
-      return memoizedCoverageArtifacts;
+    if (memoizedCoverageArtifacts == null) {
+      memoizedCoverageArtifacts =
+          constructCoverageArtifacts(
+              configuredTargets, allTargetsToTest, eventHandler, eventBus, loadingResult);
     }
-    ImmutableSet.Builder<Artifact> resultBuilder = ImmutableSet.builder();
-    // Coverage
-    NestedSet<Artifact> baselineCoverageArtifacts = getBaselineCoverageArtifacts(configuredTargets);
-    resultBuilder.addAll(baselineCoverageArtifacts.toList());
+    return memoizedCoverageArtifacts;
+  }
 
-    if (coverageReportActionFactory != null) {
-      CoverageReportActionsWrapper actionsWrapper =
-          coverageReportActionFactory.createCoverageReportActionsWrapper(
-              eventHandler,
-              eventBus,
-              directories,
-              allTargetsToTest,
-              baselineCoverageArtifacts,
-              getArtifactFactory(),
-              skyframeExecutor.getActionKeyContext(),
-              CoverageReportValue.COVERAGE_REPORT_KEY,
-              loadingResult.getWorkspaceName());
-      if (actionsWrapper != null) {
-        skyframeExecutor.injectCoverageReportData(actionsWrapper.getActions());
-        actionsWrapper.getCoverageOutputs().forEach(resultBuilder::add);
-      }
+  private ImmutableSet<Artifact> constructCoverageArtifacts(
+      Set<ConfiguredTarget> configuredTargets,
+      Set<ConfiguredTarget> allTargetsToTest,
+      EventHandler eventHandler,
+      EventBus eventBus,
+      TargetPatternPhaseValue loadingResult)
+      throws InterruptedException {
+    if (coverageReportActionFactory == null) {
+      return ImmutableSet.of();
     }
-    memoizedCoverageArtifacts = resultBuilder.build();
-    return memoizedCoverageArtifacts;
+    CoverageReportActionsWrapper actionsWrapper =
+        coverageReportActionFactory.createCoverageReportActionsWrapper(
+            eventHandler,
+            eventBus,
+            directories,
+            allTargetsToTest,
+            getBaselineCoverageArtifacts(configuredTargets),
+            getArtifactFactory(),
+            skyframeExecutor.getActionKeyContext(),
+            CoverageReportValue.COVERAGE_REPORT_KEY,
+            loadingResult.getWorkspaceName());
+    if (actionsWrapper == null) {
+      return ImmutableSet.of();
+    }
+    skyframeExecutor.injectCoverageReportData(actionsWrapper.getActions());
+    return ImmutableSet.copyOf(actionsWrapper.getCoverageOutputs());
   }
 }

src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java

@@ -174,7 +174,6 @@ private TargetCompleteEvent(
           instrumentedFilesProvider.getBaselineCoverageArtifacts();
       if (!baselineCoverageArtifacts.isEmpty()) {
         this.baselineCoverageArtifacts = baselineCoverageArtifacts;
-        postedAfterBuilder.add(BuildEventIdUtil.coverageActionsFinished());
       } else {
         this.baselineCoverageArtifacts = null;
       }

src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java

@@ -15,12 +15,16 @@
 
 package com.google.devtools.build.lib.bazel;
 
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.CharMatcher;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
@@ -131,8 +135,8 @@ public class BazelRepositoryModule extends BlazeModule {
   public static final String DEFAULT_CACHE_LOCATION = "cache/repos/v1";
 
   // Default list of registries.
-  public static final ImmutableList<String> DEFAULT_REGISTRIES =
-      ImmutableList.of("https://bcr.bazel.build/");
+  public static final ImmutableSet<String> DEFAULT_REGISTRIES =
+      ImmutableSet.of("https://bcr.bazel.build/");
 
   // A map of repository handlers that can be looked up by rule class name.
   private final ImmutableMap<String, RepositoryFunction> repositoryHandlers;
@@ -150,7 +154,7 @@ public class BazelRepositoryModule extends BlazeModule {
   private ImmutableMap<String, ModuleOverride> moduleOverrides = ImmutableMap.of();
   private Optional<RootedPath> resolvedFileReplacingWorkspace = Optional.empty();
   private FileSystem filesystem;
-  private List<String> registries;
+  private ImmutableSet<String> registries;
   private final AtomicBoolean ignoreDevDeps = new AtomicBoolean(false);
   private CheckDirectDepsMode checkDirectDepsMode = CheckDirectDepsMode.WARNING;
   private BazelCompatibilityMode bazelCompatibilityMode = BazelCompatibilityMode.ERROR;
@@ -495,7 +499,7 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
       }
 
       if (repoOptions.registries != null && !repoOptions.registries.isEmpty()) {
-        registries = repoOptions.registries;
+        registries = normalizeRegistries(repoOptions.registries);
       } else {
         registries = DEFAULT_REGISTRIES;
       }
@@ -525,6 +529,14 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
     }
   }
 
+  private static ImmutableSet<String> normalizeRegistries(List<String> registries) {
+    // Ensure that registries aren't duplicated even after `/modules/...` paths are appended to
+    // them.
+    return registries.stream()
+        .map(url -> CharMatcher.is('/').trimTrailingFrom(url))
+        .collect(toImmutableSet());
+  }
+
   /**
    * If the given path is absolute path, leave it as it is. If the given path is a relative path, it
    * is relative to the current working directory. If the given path starts with '%workspace%, it is