Support multiple --bazelrc on command line

wisechengyi · copybara-github · commit 61da1d2bf10e · 2021-03-22T14:21:27.000-07:00
Address bazelbuild#7489 ### Motivation Multiple --bazelrc on CLI would be useful for us for various reasons: Mostly importantly, we will have very long argument lists to pass to bazel query, so they have to be in a bazelrc file. `import/try import` in bazelrc would still work but very awkwardly. For example, if there are multiple bazelrc files to import, say `A` and `B`, import `A` needs to be added into `$WORKSPACE/.bazelrc` import `B` needs to be added into `A` meaning the former bazelrc file needs to know what comes next. Therefore allowing multiple --bazelrc would greatly improve the ergonomics, so the caller can create and append the new bazelrc without modifying the previous rc files. ### Note Options passed on CLI will still overwrite any options specified in any bazelrcs. ### Result `bazel --bazelrc=x.rc --bazelrc=y.rc ...` now works. ``` --bazelrc (a string; default: see description) The location of the user .bazelrc file containing default values of Bazel options. This option can also be chained together. E.g. `--bazelrc=x.rc --bazelrc=y.rc` so options in both RCs will be read. Note: `--bazelrc x.rc y.rc` is illegal, and each bazelrc file needs to be accompanied by --bazelrc flag before it. If unspecified, Bazel uses the first .bazelrc file it finds in the following two locations: the workspace directory, then the user's home directory. Use /dev/null to disable the search for a user rc file, e.g. in release builds. ``` Closes bazelbuild#12740. PiperOrigin-RevId: 364407234
diff --git a/site/docs/guide.md b/site/docs/guide.md
@@ -776,8 +776,17 @@ before the command (`build`, `test`, etc).
 4.  **The user-specified RC file**, if specified with
     <code>--bazelrc=<var>file</var></code>
 
-    This flag is optional. However, if the flag is specified, then the file must
-    exist.
+    This flag is optional but can also be specified multiple times.
+
+    `/dev/null` indicates that all further `--bazelrc`s will be ignored, which
+     is useful to disable the search for a user rc file, e.g. in release builds.
+
+    For example:
+    ```
+    --bazelrc=x.rc --bazelrc=y.rc --bazelrc=/dev/null --bazelrc=z.rc
+    ```
+    1. `x.rc` and `y.rc` are read.
+    2. `z.rc` is ignored due to the prior `/dev/null`.
 
 In addition to this optional configuration file, Bazel looks for a global rc
 file. For more details, see the [global bazelrc section](#global_bazelrc).
diff --git a/src/main/cpp/blaze_util.cc b/src/main/cpp/blaze_util.cc
@@ -19,6 +19,8 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+
+#include <algorithm>
 #include <cassert>
 #include <iostream>
 
@@ -35,6 +37,7 @@
 namespace blaze {
 
 using std::map;
+using std::min;
 using std::string;
 using std::vector;
 
@@ -74,6 +77,40 @@ bool GetNullaryOption(const char *arg, const char *key) {
   return true;
 }
 
+std::vector<std::string> GetAllUnaryOptionValues(
+    const vector<string>& args, const char* key,
+    const char* ignore_after_value) {
+  vector<std::string> values;
+  for (vector<string>::size_type i = 0; i < args.size(); ++i) {
+    if (args[i] == "--") {
+      // "--" means all remaining args aren't options
+      return values;
+    }
+
+    const char* next_arg = args[std::min(i + 1, args.size() - 1)].c_str();
+    const char* result = GetUnaryOption(args[i].c_str(), next_arg, key);
+    if (result != nullptr) {
+      // 'key' was found and 'result' has its value.
+      values.push_back(result);
+
+      if (ignore_after_value != nullptr &&
+          strcmp(result, ignore_after_value) == 0) {
+        break;
+      }
+    }
+
+    // This is a pointer comparison, so equality means that the result must be
+    // from the next arg instead of happening to match the value from
+    // "--key=<value>" string, in which case we need to advance the index to
+    // skip the next arg for later iterations.
+    if (result == next_arg) {
+      i++;
+    }
+  }
+
+  return values;
+}
+
 const char* SearchUnaryOption(const vector<string>& args,
                               const char *key, bool warn_if_dupe) {
   if (args.empty()) {
diff --git a/src/main/cpp/blaze_util.h b/src/main/cpp/blaze_util.h
@@ -53,6 +53,15 @@ bool GetNullaryOption(const char *arg, const char *key);
 const char* SearchUnaryOption(const std::vector<std::string>& args,
                               const char* key, bool warn_if_dupe);
 
+// Searches for 'key' in 'args' using GetUnaryOption. Arguments found after '--'
+// are omitted from the search.
+// If 'ignore_after_value' is not nullptr, all values matching the 'key'
+// following 'ignore_after_value' will be ignored. Returns the values of the
+// 'key' flag iff it occurs in args. Returns empty vector otherwise.
+std::vector<std::string> GetAllUnaryOptionValues(
+    const std::vector<std::string>& args, const char* key,
+    const char* ignore_after_value = nullptr);
+
 // Searches for '--flag_name' and '--noflag_name' in 'args' using
 // GetNullaryOption. Arguments found after '--' are omitted from the search.
 // Returns true if '--flag_name' is a flag in args and '--noflag_name' does not
diff --git a/src/main/cpp/option_processor.cc b/src/main/cpp/option_processor.cc
@@ -201,11 +201,24 @@ std::set<std::string> GetOldRcPaths(
         internal::FindRcAlongsideBinary(cwd, path_to_binary);
     candidate_bazelrc_paths = {workspace_rc, binary_rc, system_bazelrc_path};
   }
-  string user_bazelrc_path = internal::FindLegacyUserBazelrc(
-      SearchUnaryOption(startup_args, "--bazelrc", /* warn_if_dupe */ true),
-      workspace);
-  if (!user_bazelrc_path.empty()) {
-    candidate_bazelrc_paths.push_back(user_bazelrc_path);
+  vector<std::string> cmd_line_rc_files =
+      GetAllUnaryOptionValues(startup_args, "--bazelrc", "/dev/null");
+  // Divide the cases where the vector is empty vs not, as
+  // `FindLegacyUserBazelrc` has a case for rc_file to be a nullptr.
+  if (cmd_line_rc_files.empty()) {
+    string user_bazelrc_path =
+        internal::FindLegacyUserBazelrc(nullptr, workspace);
+    if (!user_bazelrc_path.empty()) {
+      candidate_bazelrc_paths.push_back(user_bazelrc_path);
+    }
+  } else {
+    for (auto& rc_file : cmd_line_rc_files) {
+      string user_bazelrc_path =
+          internal::FindLegacyUserBazelrc(rc_file.c_str(), workspace);
+      if (!user_bazelrc_path.empty()) {
+        candidate_bazelrc_paths.push_back(user_bazelrc_path);
+      }
+    }
   }
   // DedupeBlazercPaths returns paths whose canonical path could be computed,
   // therefore these paths must exist.
@@ -370,11 +383,10 @@ blaze_exit_code::ExitCode OptionProcessor::GetRcFiles(
 
   // Get the command-line provided rc, passed as --bazelrc or nothing if the
   // flag is absent.
-  const char* cmd_line_rc_file =
-      SearchUnaryOption(cmd_line->startup_args, "--bazelrc",
-                        /* warn_if_dupe */ true);
-  if (cmd_line_rc_file != nullptr) {
-    string absolute_cmd_line_rc = blaze::AbsolutePathFromFlag(cmd_line_rc_file);
+  vector<std::string> cmd_line_rc_files =
+      GetAllUnaryOptionValues(cmd_line->startup_args, "--bazelrc", "/dev/null");
+  for (auto& rc_file : cmd_line_rc_files) {
+    string absolute_cmd_line_rc = blaze::AbsolutePathFromFlag(rc_file);
     // Unlike the previous 3 paths, where we ignore it if the file does not
     // exist or is unreadable, since this path is explicitly passed, this is an
     // error. Check this condition here.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelStartupOptionsModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelStartupOptionsModule.java
@@ -33,10 +33,19 @@ public static final class Options extends OptionsBase {
         valueHelp = "<path>",
         help =
             "The location of the user .bazelrc file containing default values of "
-                + "Bazel options. If unspecified, Bazel uses the first .bazelrc file it finds in "
+                + "Bazel options. "
+                + "/dev/null indicates that all further `--bazelrc`s will be ignored, "
+                + "which is useful to disable the search for a user rc file, "
+                + "e.g. in release builds.\n"
+                + "This option can also be specified multiple times.\n"
+                + "E.g. with "
+                + "`--bazelrc=x.rc --bazelrc=y.rc --bazelrc=/dev/null --bazelrc=z.rc`,\n"
+                + "  1) x.rc and y.rc are read.\n"
+                + "  2) z.rc is ignored due to the prior /dev/null.\n"
+                + "If unspecified, Bazel uses the first .bazelrc file it finds in "
                 + "the following two locations: the workspace directory, then the user's home "
-                + "directory. Use /dev/null to disable the search for a user rc file, e.g. in "
-                + "release builds.")
+                + "directory.\n"
+                + "Note: command line options will always supersede any option in bazelrc.")
     public String blazerc;
 
     // TODO(b/36168162): Remove this after the transition period is ower. This now only serves to
diff --git a/src/test/cpp/blaze_util_test.cc b/src/test/cpp/blaze_util_test.cc
@@ -234,4 +234,140 @@ TEST_F(BlazeUtilTest, TestSearchUnaryCommandOptionWarnsAboutDuplicates) {
   }
 }
 
+void assert_equal_vector_char_pointer(std::vector<std::string> expected,
+                                      std::vector<std::string> actual) {
+  ASSERT_EQ(actual.size(), expected.size())
+      << "Vectors expected and actual are of unequal length";
+
+  for (int i = 0; i < actual.size(); ++i) {
+    ASSERT_EQ(actual[i], expected[i])
+        << "Vectors expected and actual differ at index " << i;
+  }
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryForEmpty) {
+  assert_equal_vector_char_pointer(
+      {}, GetAllUnaryOptionValues({"bazel", "build", ":target"}, ""));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryFlagNotPresent) {
+  assert_equal_vector_char_pointer(
+      {}, GetAllUnaryOptionValues({"bazel", "build", ":target"}, "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryOptionWithEquals) {
+  assert_equal_vector_char_pointer(
+      {"value"}, GetAllUnaryOptionValues(
+                     {"bazel", "--flag=value", "build", ":target"}, "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryOptionWithEquals2) {
+  assert_equal_vector_char_pointer(
+      {"value1", "value2"},
+      GetAllUnaryOptionValues(
+          {"bazel", "--flag=value1", "--flag=value2", "build", ":target"},
+          "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryOptionWithRepeatingFlag) {
+  assert_equal_vector_char_pointer(
+      {"--flag"}, GetAllUnaryOptionValues({"bazel", "--flag", "--flag",
+                                           "value1", "build", ":target"},
+                                          "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryOptionWithRepeatingFlagOptions) {
+  assert_equal_vector_char_pointer(
+      {"--flag"}, GetAllUnaryOptionValues(
+                      {"bazel", "--flag", "--flag", "value1"}, "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryOptionValuesWithEquals) {
+  assert_equal_vector_char_pointer(
+      {"--flag", "value1"},
+      GetAllUnaryOptionValues({"bazel", "--flag=--flag", "--flag", "value1"},
+                              "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryOptionWithEquals3) {
+  assert_equal_vector_char_pointer(
+      {"value1", "value2", "value3"},
+      GetAllUnaryOptionValues({"bazel", "--flag=value1", "--flag=value2",
+                               "--flag=value3", "build", ":target"},
+                              "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryOptionWithoutEquals) {
+  assert_equal_vector_char_pointer(
+      {"value"},
+      GetAllUnaryOptionValues({"bazel", "--flag", "value", "build", ":target"},
+                              "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryOptionWithoutEquals2) {
+  assert_equal_vector_char_pointer(
+      {"value1", "value2"},
+      GetAllUnaryOptionValues(
+          {"bazel", "--flag", "value1", "--flag", "value2", "build", ":target"},
+          "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryCommandOptionWithEquals) {
+  assert_equal_vector_char_pointer(
+      {"value"},
+      GetAllUnaryOptionValues({"bazel", "build", ":target", "--flag", "value"},
+                              "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryCommandOptionWithEquals2) {
+  assert_equal_vector_char_pointer(
+      {"value1", "value2"},
+      GetAllUnaryOptionValues(
+          {"bazel", "build", ":target", "--flag", "value1", "--flag", "value2"},
+          "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryCommandOptionWithoutEquals) {
+  assert_equal_vector_char_pointer(
+      {"value"}, GetAllUnaryOptionValues(
+                     {"bazel", "build", ":target", "--flag=value"}, "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryCommandOptionWithoutEquals2) {
+  assert_equal_vector_char_pointer(
+      {"value1", "value2"},
+      GetAllUnaryOptionValues(
+          {"bazel", "build", ":target", "--flag=value1", "--flag=value2"},
+          "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnarySkipsAfterDashDashWithEquals) {
+  assert_equal_vector_char_pointer(
+      {},
+      GetAllUnaryOptionValues(
+          {"bazel", "build", ":target", "--", "--flag", "value"}, "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnarySkipsAfterDashDashWithoutEquals) {
+  assert_equal_vector_char_pointer(
+      {}, GetAllUnaryOptionValues(
+              {"bazel", "build", ":target", "--", "--flag=value"}, "--flag"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryCommandOptionWithIgnoreAfter) {
+  assert_equal_vector_char_pointer(
+      {"value1", "/dev/null"},
+      GetAllUnaryOptionValues({"bazel", "build", ":target", "--flag", "value1",
+                               "--flag", "/dev/null", "--flag", "value3"},
+                              "--flag", "/dev/null"));
+}
+
+TEST_F(BlazeUtilTest, TestSearchAllUnaryCommandOptionWithIgnoreAfterDevNull) {
+  assert_equal_vector_char_pointer(
+      {"/dev/null"}, GetAllUnaryOptionValues(
+                         {"bazel", "build", ":target", "--flag", "/dev/null",
+                          "--flag", "value2", "--flag", "value3"},
+                         "--flag", "/dev/null"));
+}
+
 }  // namespace blaze
diff --git a/src/test/shell/integration/startup_options_test.sh b/src/test/shell/integration/startup_options_test.sh
@@ -82,4 +82,40 @@ function test_autodetect_server_javabase() {
   bazel --noautodetect_server_javabase version &> $TEST_log || fail "Should pass"
 }
 
+# Below are the regression tests for Issue #7489
+function test_multiple_bazelrc_later_overwrites_earlier() {
+  # Help message only visible with --help_verbosity=medium
+  help_message_in_description="--${PRODUCT_NAME}rc (a string; default: see description)"
+
+  echo "help --help_verbosity=short" > 1.rc
+  echo "help --help_verbosity=medium" > 2.rc
+  bazel "--${PRODUCT_NAME}rc=1.rc" "--${PRODUCT_NAME}rc=2.rc" help startup_options &> $TEST_log || fail "Should pass"
+  expect_log "$help_message_in_description"
+
+  echo "help --help_verbosity=medium" > 1.rc
+  echo "help --help_verbosity=short" > 2.rc
+  bazel "--${PRODUCT_NAME}rc=1.rc" "--${PRODUCT_NAME}rc=2.rc" help startup_options &> $TEST_log || fail "Should pass"
+  expect_not_log "$help_message_in_description"
+}
+
+function test_multiple_bazelrc_set_different_options() {
+  echo "common --verbose_failures" > 1.rc
+  echo "common --test_output=all" > 2.rc
+  bazel "--${PRODUCT_NAME}rc=1.rc" "--${PRODUCT_NAME}rc=2.rc" info --announce_rc &> $TEST_log || fail "Should pass"
+  expect_log "Inherited 'common' options: --verbose_failures"
+  expect_log "Inherited 'common' options: --test_output=all"
+}
+
+function test_bazelrc_after_devnull_ignored() {
+  echo "common --verbose_failures" > 1.rc
+  echo "common --test_output=all" > 2.rc
+  echo "common --definitely_invalid_config" > 3.rc
+
+  bazel "--${PRODUCT_NAME}rc=1.rc" "--${PRODUCT_NAME}rc=2.rc" "--${PRODUCT_NAME}rc=/dev/null" \
+   "--${PRODUCT_NAME}rc=3.rc" info --announce_rc &> $TEST_log || fail "Should pass"
+  expect_log "Inherited 'common' options: --verbose_failures"
+  expect_log "Inherited 'common' options: --test_output=all"
+  expect_not_log "--definitely_invalid_config"
+}
+
 run_suite "${PRODUCT_NAME} startup options test"
