From 61da1d2bf10eabba4c75de959b0374f302d89d70 Mon Sep 17 00:00:00 2001 From: wisechengyi Date: Mon, 22 Mar 2021 14:20:33 -0700 Subject: [PATCH] Support multiple --bazelrc on command line Address #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 #12740. PiperOrigin-RevId: 364407234 --- site/docs/guide.md | 13 +- src/main/cpp/blaze_util.cc | 37 +++++ src/main/cpp/blaze_util.h | 9 ++ src/main/cpp/option_processor.cc | 32 +++-- .../lib/bazel/BazelStartupOptionsModule.java | 15 +- src/test/cpp/blaze_util_test.cc | 136 ++++++++++++++++++ .../shell/integration/startup_options_test.sh | 36 +++++ 7 files changed, 263 insertions(+), 15 deletions(-) diff --git a/site/docs/guide.md b/site/docs/guide.md index 40bb606a34b527..91655157499895 100644 --- 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 --bazelrc=file - 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 index 43dfc63d429be9..c3b897321fa5a9 100644 --- a/src/main/cpp/blaze_util.cc +++ b/src/main/cpp/blaze_util.cc @@ -19,6 +19,8 @@ #include #include #include + +#include #include #include @@ -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 GetAllUnaryOptionValues( + const vector& args, const char* key, + const char* ignore_after_value) { + vector values; + for (vector::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=" 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& 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 index ebf7a6dc2e0749..861cbe256921a9 100644 --- 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& 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 GetAllUnaryOptionValues( + const std::vector& 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 index 421e6ef511226c..9fb942277771fa 100644 --- a/src/main/cpp/option_processor.cc +++ b/src/main/cpp/option_processor.cc @@ -201,11 +201,24 @@ std::set 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 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 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 index 0ea737acf87ae0..b87146f7a65648 100644 --- 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 = "", 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 index dda7bb51388760..6bf539c23237f9 100644 --- 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 expected, + std::vector 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 index 63f1624866d41d..b2cdd6d3d04146 100755 --- 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"