diff --git a/src/main/cpp/rc_file.cc b/src/main/cpp/rc_file.cc index 2ad9d6dd72f557..e323ae976af4df 100644 --- a/src/main/cpp/rc_file.cc +++ b/src/main/cpp/rc_file.cc @@ -144,6 +144,13 @@ RcFile::ParseError RcFile::ParseFile(const string& filename, } else { auto words_it = words.begin(); words_it++; // Advance past command. + if (words_it == words.end()) { + blaze_util::StringPrintf( + error_text, + "Incomplete line in .blazerc file '%s': '%s'", + canonical_filename.c_str(), line.c_str()); + return ParseError::INVALID_FORMAT; + } for (; words_it != words.end(); words_it++) { options_[command].push_back({*words_it, rcfile_index}); } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java index bee31632fdbd31..b1883a1377fa84 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java @@ -629,15 +629,13 @@ static ListMultimap structureRcOptionsAndConfigs( if (!validCommands.contains(command) && !command.equals(ALWAYS_PSEUDO_COMMAND) && !command.equals(COMMON_PSEUDO_COMMAND)) { - eventHandler.handle( - Event.warn( - "while reading option defaults file '" - + rcFile - + "':\n" - + " invalid command name '" - + override.command - + "'.")); - continue; + throw new OptionsParsingException( + "while reading option defaults file '" + + rcFile + + "':\n" + + " invalid command name '" + + override.command + + "'."); } // We've moved on to another rc file "chunk," store the accumulated args from the last one. diff --git a/src/test/cpp/rc_options_test.cc b/src/test/cpp/rc_options_test.cc index c7cec75e40f101..085fb93495f0ed 100644 --- a/src/test/cpp/rc_options_test.cc +++ b/src/test/cpp/rc_options_test.cc @@ -97,6 +97,18 @@ class RcOptionsTest : public ::testing::Test { } } } + + void FailToParseRcWithErrorAndRegex( + const string& filename, + const RcFile::ParseError expected_error, + const string& expected_error_regex) { + RcFile::ParseError error; + string error_text; + std::unique_ptr rc = Parse(filename, &error, &error_text); + // Test the text first, as the error code is hard to debug. + ASSERT_THAT(error_text, MatchesRegex(expected_error_regex)); + ASSERT_EQ(error, expected_error); + } }; TEST_F(RcOptionsTest, Empty) { @@ -124,17 +136,21 @@ TEST_F(RcOptionsTest, CommentedStartup) { TEST_F(RcOptionsTest, EmptyStartupLine) { WriteRc("empty_startup_line.bazelrc", "startup"); - unordered_map> no_expected_args; - SuccessfullyParseRcWithExpectedArgs("empty_startup_line.bazelrc", - no_expected_args); + FailToParseRcWithErrorAndRegex( + "empty_startup_line.bazelrc", + RcFile::ParseError::INVALID_FORMAT, + "Incomplete line in \\.blazerc file '.*empty_startup_line\\.bazelrc': " + "'startup'"); } TEST_F(RcOptionsTest, StartupWithOnlyCommentedArg) { WriteRc("startup_with_comment.bazelrc", "startup # bar"); - unordered_map> no_expected_args; - SuccessfullyParseRcWithExpectedArgs("startup_with_comment.bazelrc", - no_expected_args); + FailToParseRcWithErrorAndRegex( + "startup_with_comment.bazelrc", + RcFile::ParseError::INVALID_FORMAT, + "Incomplete line in \\.blazerc file '.*startup_with_comment\\.bazelrc': " + "'startup # bar'"); } TEST_F(RcOptionsTest, SingleStartupArg) { @@ -355,17 +371,13 @@ TEST_F(RcOptionsTest, ImportCycleFails) { WriteRc("import_cycle_2.bazelrc", "import %workspace%/import_cycle_1.bazelrc"); - RcFile::ParseError error; - string error_text; - std::unique_ptr rc = - Parse("import_cycle_1.bazelrc", &error, &error_text); - EXPECT_EQ(error, RcFile::ParseError::IMPORT_LOOP); - ASSERT_THAT( - error_text, - MatchesRegex("Import loop detected:\n" - " .*import_cycle_1.bazelrc\n" - " .*import_cycle_2.bazelrc\n" - " .*import_cycle_1.bazelrc\n")); + FailToParseRcWithErrorAndRegex( + "import_cycle_1.bazelrc", + RcFile::ParseError::IMPORT_LOOP, + "Import loop detected:\n" + " .*import_cycle_1.bazelrc\n" + " .*import_cycle_2.bazelrc\n" + " .*import_cycle_1.bazelrc\n"); } TEST_F(RcOptionsTest, LongImportCycleFails) { @@ -382,53 +394,62 @@ TEST_F(RcOptionsTest, LongImportCycleFails) { WriteRc("import_cycle_2.bazelrc", "import %workspace%/import_cycle_1.bazelrc"); - RcFile::ParseError error; - string error_text; - std::unique_ptr rc = - Parse("chain_to_cycle_1.bazelrc", &error, &error_text); - EXPECT_EQ(error, RcFile::ParseError::IMPORT_LOOP); - ASSERT_THAT( - error_text, - MatchesRegex("Import loop detected:\n" - " .*chain_to_cycle_1.bazelrc\n" - " .*chain_to_cycle_2.bazelrc\n" - " .*chain_to_cycle_3.bazelrc\n" - " .*chain_to_cycle_4.bazelrc\n" - " .*import_cycle_1.bazelrc\n" - " .*import_cycle_2.bazelrc\n" - " .*import_cycle_1.bazelrc\n")); + FailToParseRcWithErrorAndRegex( + "chain_to_cycle_1.bazelrc", + RcFile::ParseError::IMPORT_LOOP, + "Import loop detected:\n" + " .*chain_to_cycle_1.bazelrc\n" + " .*chain_to_cycle_2.bazelrc\n" + " .*chain_to_cycle_3.bazelrc\n" + " .*chain_to_cycle_4.bazelrc\n" + " .*import_cycle_1.bazelrc\n" + " .*import_cycle_2.bazelrc\n" + " .*import_cycle_1.bazelrc\n"); +} + +TEST_F(RcOptionsTest, MissingCommandFails) { + WriteRc("missing_command.bazelrc", + "--debug_client"); + + FailToParseRcWithErrorAndRegex( + "missing_command.bazelrc", + RcFile::ParseError::INVALID_FORMAT, + "Incomplete line in \\.blazerc file '.*missing_command\\.bazelrc': " + "'--debug_client'"); +} + +TEST_F(RcOptionsTest, EmptyOptionsFails) { + WriteRc("empty_options.bazelrc", + "build:debug # --debug_client"); + + FailToParseRcWithErrorAndRegex( + "empty_options.bazelrc", + RcFile::ParseError::INVALID_FORMAT, + "Incomplete line in \\.blazerc file '.*empty_options\\.bazelrc': " + "'build:debug # --debug_client'"); } TEST_F(RcOptionsTest, FileDoesNotExist) { - RcFile::ParseError error; - string error_text; - std::unique_ptr rc = Parse("not_a_file.bazelrc", &error, &error_text); - EXPECT_EQ(error, RcFile::ParseError::UNREADABLE_FILE); - ASSERT_THAT( - error_text, - MatchesRegex(kIsWindows - ? "Unexpected error reading \\.blazerc file '.*not_a_file\\.bazelrc':.*" - : "Unexpected error reading \\.blazerc file '.*not_a_file\\.bazelrc': " - "\\(error: 2\\): No such file or directory")); + FailToParseRcWithErrorAndRegex( + "not_a_file.bazelrc", + RcFile::ParseError::UNREADABLE_FILE, + kIsWindows + ? "Unexpected error reading \\.blazerc file '.*not_a_file\\.bazelrc':.*" + : "Unexpected error reading \\.blazerc file '.*not_a_file\\.bazelrc': " + "\\(error: 2\\): No such file or directory"); } TEST_F(RcOptionsTest, ImportedFileDoesNotExist) { WriteRc("import_fake_file.bazelrc", "import somefile"); - RcFile::ParseError error; - string error_text; - std::unique_ptr rc = - Parse("import_fake_file.bazelrc", &error, &error_text); - EXPECT_EQ(error, RcFile::ParseError::UNREADABLE_FILE); - if (kIsWindows) { - ASSERT_THAT(error_text, MatchesRegex( - "Unexpected error reading \\.blazerc file 'somefile':.*")); - } else { - ASSERT_EQ(error_text, - "Unexpected error reading .blazerc file 'somefile': (error: 2): No such " - "file or directory"); - } + FailToParseRcWithErrorAndRegex( + "import_fake_file.bazelrc", + RcFile::ParseError::UNREADABLE_FILE, + kIsWindows + ? "Unexpected error reading \\.blazerc file 'somefile':.*" + : "Unexpected error reading .blazerc file 'somefile': \\(error: 2\\): " + "No such file or directory"); } TEST_F(RcOptionsTest, TryImportedFileDoesNotExist) { @@ -443,29 +464,23 @@ TEST_F(RcOptionsTest, ImportHasTooManyArgs) { WriteRc("bad_import.bazelrc", "import somefile bar"); - RcFile::ParseError error; - string error_text; - std::unique_ptr rc = Parse("bad_import.bazelrc", &error, &error_text); - EXPECT_EQ(error, RcFile::ParseError::INVALID_FORMAT); - ASSERT_THAT( - error_text, - MatchesRegex("Invalid import declaration in .blazerc file " - "'.*bad_import.bazelrc': 'import somefile bar' \\(are you " - "in your source checkout/WORKSPACE\\?\\)")); + FailToParseRcWithErrorAndRegex( + "bad_import.bazelrc", + RcFile::ParseError::INVALID_FORMAT, + "Invalid import declaration in .blazerc file " + "'.*bad_import.bazelrc': 'import somefile bar' \\(are you " + "in your source checkout/WORKSPACE\\?\\)"); } TEST_F(RcOptionsTest, TryImportHasTooManyArgs) { WriteRc("bad_import.bazelrc", "try-import somefile bar"); - RcFile::ParseError error; - string error_text; - std::unique_ptr rc = Parse("bad_import.bazelrc", &error, &error_text); - EXPECT_EQ(error, RcFile::ParseError::INVALID_FORMAT); - ASSERT_THAT( - error_text, - MatchesRegex("Invalid import declaration in .blazerc file " - "'.*bad_import.bazelrc': 'try-import somefile bar' \\(are " - "you in your source checkout/WORKSPACE\\?\\)")); + FailToParseRcWithErrorAndRegex( + "bad_import.bazelrc", + RcFile::ParseError::INVALID_FORMAT, + "Invalid import declaration in .blazerc file " + "'.*bad_import.bazelrc': 'try-import somefile bar' \\(are " + "you in your source checkout/WORKSPACE\\?\\)"); } // TODO(b/34811299) The tests below identify ways that '\' used as a line @@ -482,9 +497,11 @@ TEST_F(RcOptionsTest, BadStartupLineContinuation_HasWhitespaceAfterSlash) { WriteRc("bad_startup_line_continuation.bazelrc", "startup foo \\ \n" "bar"); - SuccessfullyParseRcWithExpectedArgs( + FailToParseRcWithErrorAndRegex( "bad_startup_line_continuation.bazelrc", - {{"startup", {"foo"}}}); // Does not contain "bar" from the next line. + RcFile::ParseError::INVALID_FORMAT, + "Incomplete line in \\.blazerc file '.*bad_startup_line_continuation\\.bazelrc': " + "'bar'"); } TEST_F(RcOptionsTest, BadStartupLineContinuation_HasErroneousSlash) { @@ -500,11 +517,11 @@ TEST_F(RcOptionsTest, BadStartupLineContinuation_HasCommentAfterSlash) { WriteRc("bad_startup_line_continuation.bazelrc", "startup foo \\ # comment\n" "bar"); - SuccessfullyParseRcWithExpectedArgs( + FailToParseRcWithErrorAndRegex( "bad_startup_line_continuation.bazelrc", - // Whitespace between the slash and comment gets counted as a new token, - // and the bar on the next line is ignored (it's an argumentless command). - {{"startup", {"foo", " "}}}); + RcFile::ParseError::INVALID_FORMAT, + "Incomplete line in \\.blazerc file '.*bad_startup_line_continuation\\.bazelrc': " + "'bar'"); } TEST_F(RcOptionsTest, BadStartupLineContinuation_InterpretsNextLineAsNewline) { diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java index 3beef44e607e68..4414c1352dc9ab 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java @@ -152,14 +152,20 @@ public void testStructureRcOptionsAndConfigs_configOnly() throws Exception { @Test public void testStructureRcOptionsAndConfigs_invalidCommand() throws Exception { - BlazeOptionHandler.structureRcOptionsAndConfigs( - eventHandler, - Arrays.asList("rc1", "rc2"), - Arrays.asList(new ClientOptions.OptionOverride(0, "c1", "a")), - ImmutableSet.of("build")); - assertThat(eventHandler.getEvents()) - .contains( - Event.warn("while reading option defaults file 'rc1':\n invalid command name 'c1'.")); + OptionsParsingException e = + assertThrows( + OptionsParsingException.class, + () -> + BlazeOptionHandler.structureRcOptionsAndConfigs( + eventHandler, + Arrays.asList("rc1", "rc2"), + Arrays.asList(new ClientOptions.OptionOverride(0, "c1", "a")), + ImmutableSet.of("build"))); + assertThat(parser.getResidue()).isEmpty(); + assertThat(optionHandler.getRcfileNotes()).isEmpty(); + assertThat(e) + .hasMessageThat() + .contains("while reading option defaults file 'rc1':\n invalid command name 'c1'."); } @Test