Have setup test use an an output Err Ths suppresses stdout printing during the test, and allows confirming the error message as well. See also: https://gn-review.googlesource.com/c/gn/+/8940 https://gn-review.googlesource.com/c/gn/+/8500 Change-Id: I1457d7611c8b4327c1cda083f1408ca7cc0cd1f8 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/8941 Commit-Queue: Nico Weber <thakis@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org>
diff --git a/src/gn/setup.cc b/src/gn/setup.cc index 1d2bea2..c1d8688 100644 --- a/src/gn/setup.cc +++ b/src/gn/setup.cc
@@ -350,6 +350,19 @@ bool Setup::DoSetup(const std::string& build_dir, bool force_create, const base::CommandLine& cmdline) { + Err err; + if (!DoSetupWithErr(build_dir, force_create, cmdline, &err)) { + err.PrintToStdout(); + return false; + } + DCHECK(!err.has_error()); + return true; +} + +bool Setup::DoSetupWithErr(const std::string& build_dir, + bool force_create, + const base::CommandLine& cmdline, + Err* err) { scheduler_.set_verbose_logging(cmdline.HasSwitch(switches::kVerbose)); if (cmdline.HasSwitch(switches::kTime) || cmdline.HasSwitch(switches::kTracelog)) @@ -357,15 +370,15 @@ ScopedTrace setup_trace(TraceItem::TRACE_SETUP, "DoSetup"); - if (!FillSourceDir(cmdline)) + if (!FillSourceDir(cmdline, err)) return false; - if (!RunConfigFile()) + if (!RunConfigFile(err)) return false; - if (!FillOtherConfig(cmdline)) + if (!FillOtherConfig(cmdline, err)) return false; // Must be after FillSourceDir to resolve. - if (!FillBuildDir(build_dir, !force_create)) + if (!FillBuildDir(build_dir, !force_create, err)) return false; // Apply project-specific default (if specified). @@ -377,16 +390,14 @@ } if (fill_arguments_) { - if (!FillArguments(cmdline)) + if (!FillArguments(cmdline, err)) return false; } - if (!FillPythonPath(cmdline)) + if (!FillPythonPath(cmdline, err)) return false; // Check for unused variables in the .gn file. - Err err; - if (!dotfile_scope_.CheckForUnusedVars(&err)) { - err.PrintToStdout(); + if (!dotfile_scope_.CheckForUnusedVars(err)) { return false; } @@ -460,7 +471,7 @@ return true; } -bool Setup::FillArguments(const base::CommandLine& cmdline) { +bool Setup::FillArguments(const base::CommandLine& cmdline, Err* err) { // Use the args on the command line if specified, and save them. Do this even // if the list is empty (this means clear any defaults). // If --args is not set, args.gn file does not exist and gen_empty_args @@ -471,8 +482,8 @@ auto switch_value = cmdline.GetSwitchValueASCII(switches::kArgs); if (cmdline.HasSwitch(switches::kArgs) || (gen_empty_args_ && !PathExists(build_arg_file))) { - if (!FillArgsFromCommandLine(switch_value.empty() ? kDefaultArgsGn - : switch_value)) { + if (!FillArgsFromCommandLine( + switch_value.empty() ? kDefaultArgsGn : switch_value, err)) { return false; } SaveArgsToFile(); @@ -480,17 +491,17 @@ } // No command line args given, use the arguments from the build dir (if any). - return FillArgsFromFile(); + return FillArgsFromFile(err); } -bool Setup::FillArgsFromCommandLine(const std::string& args) { +bool Setup::FillArgsFromCommandLine(const std::string& args, Err* err) { args_input_file_ = std::make_unique<InputFile>(SourceFile()); args_input_file_->SetContents(args); args_input_file_->set_friendly_name("the command-line \"--args\""); - return FillArgsFromArgsInputFile(); + return FillArgsFromArgsInputFile(err); } -bool Setup::FillArgsFromFile() { +bool Setup::FillArgsFromFile(Err* err) { ScopedTrace setup_trace(TraceItem::TRACE_SETUP, "Load args file"); SourceFile build_arg_source_file = GetBuildArgFile(); @@ -514,22 +525,19 @@ "build arg file (use \"gn args <out_dir>\" to edit)"); setup_trace.Done(); // Only want to count the load as part of the trace. - return FillArgsFromArgsInputFile(); + return FillArgsFromArgsInputFile(err); } -bool Setup::FillArgsFromArgsInputFile() { +bool Setup::FillArgsFromArgsInputFile(Err* err) { ScopedTrace setup_trace(TraceItem::TRACE_SETUP, "Parse args"); - Err err; - args_tokens_ = Tokenizer::Tokenize(args_input_file_.get(), &err); - if (err.has_error()) { - err.PrintToStdout(); + args_tokens_ = Tokenizer::Tokenize(args_input_file_.get(), err); + if (err->has_error()) { return false; } - args_root_ = Parser::Parse(args_tokens_, &err); - if (err.has_error()) { - err.PrintToStdout(); + args_root_ = Parser::Parse(args_tokens_, err); + if (err->has_error()) { return false; } @@ -538,9 +546,8 @@ SourceDir root_source_dir = SourceDirForCurrentDirectory(build_settings_.root_path()); arg_scope.set_source_dir(root_source_dir); - args_root_->Execute(&arg_scope, &err); - if (err.has_error()) { - err.PrintToStdout(); + args_root_->Execute(&arg_scope, err); + if (err->has_error()) { return false; } @@ -586,7 +593,7 @@ return true; } -bool Setup::FillSourceDir(const base::CommandLine& cmdline) { +bool Setup::FillSourceDir(const base::CommandLine& cmdline, Err* err) { // Find the .gn file. base::FilePath root_path; @@ -596,10 +603,9 @@ if (!relative_root_path.empty()) { root_path = base::MakeAbsoluteFilePath(relative_root_path); if (root_path.empty()) { - Err(Location(), "Root source path not found.", - "The path \"" + FilePathToUTF8(relative_root_path) + - "\" doesn't exist.") - .PrintToStdout(); + *err = Err(Location(), "Root source path not found.", + "The path \"" + FilePathToUTF8(relative_root_path) + + "\" doesn't exist."); return false; } @@ -613,10 +619,9 @@ } else { dotfile_name_ = base::MakeAbsoluteFilePath(dotfile_path); if (dotfile_name_.empty()) { - Err(Location(), "Could not load dotfile.", - "The file \"" + FilePathToUTF8(dotfile_path) + - "\" couldn't be loaded.") - .PrintToStdout(); + *err = Err(Location(), "Could not load dotfile.", + "The file \"" + FilePathToUTF8(dotfile_path) + + "\" couldn't be loaded."); return false; } // Only set dotfile_name if it was passed explicitly. @@ -629,10 +634,9 @@ base::GetCurrentDirectory(&cur_dir); dotfile_name_ = FindDotFile(cur_dir); if (dotfile_name_.empty()) { - Err(Location(), "Can't find source root.", + *err = Err(Location(), "Can't find source root.", "I could not find a \".gn\" file in the current directory or any " - "parent,\nand the --root command-line argument was not specified.") - .PrintToStdout(); + "parent,\nand the --root command-line argument was not specified."); return false; } root_path = dotfile_name_.DirName(); @@ -640,10 +644,9 @@ base::FilePath root_realpath = base::MakeAbsoluteFilePath(root_path); if (root_realpath.empty()) { - Err(Location(), "Can't get the real root path.", - "I could not get the real path of \"" + FilePathToUTF8(root_path) + - "\".") - .PrintToStdout(); + *err = Err(Location(), "Can't get the real root path.", + "I could not get the real path of \"" + + FilePathToUTF8(root_path) + "\"."); return false; } if (scheduler_.verbose_logging()) @@ -653,32 +656,30 @@ return true; } -bool Setup::FillBuildDir(const std::string& build_dir, bool require_exists) { - Err err; +bool Setup::FillBuildDir(const std::string& build_dir, + bool require_exists, + Err* err) { SourceDir resolved = SourceDirForCurrentDirectory(build_settings_.root_path()) - .ResolveRelativeDir(Value(nullptr, build_dir), &err, + .ResolveRelativeDir(Value(nullptr, build_dir), err, build_settings_.root_path_utf8()); - if (err.has_error()) { - err.PrintToStdout(); + if (err->has_error()) { return false; } base::FilePath build_dir_path = build_settings_.GetFullPath(resolved); if (!base::CreateDirectory(build_dir_path)) { - Err(Location(), "Can't create the build dir.", - "I could not create the build dir \"" + FilePathToUTF8(build_dir_path) + - "\".") - .PrintToStdout(); + *err = Err(Location(), "Can't create the build dir.", + "I could not create the build dir \"" + + FilePathToUTF8(build_dir_path) + "\"."); return false; } base::FilePath build_dir_realpath = base::MakeAbsoluteFilePath(build_dir_path); if (build_dir_realpath.empty()) { - Err(Location(), "Can't get the real build dir path.", - "I could not get the real path of \"" + FilePathToUTF8(build_dir_path) + - "\".") - .PrintToStdout(); + *err = Err(Location(), "Can't get the real build dir path.", + "I could not get the real path of \"" + + FilePathToUTF8(build_dir_path) + "\"."); return false; } resolved = SourceDirForPath(build_settings_.root_path(), build_dir_realpath); @@ -689,12 +690,12 @@ if (require_exists) { if (!base::PathExists( build_dir_path.Append(FILE_PATH_LITERAL("build.ninja")))) { - Err(Location(), "Not a build directory.", + *err = Err( + Location(), "Not a build directory.", "This command requires an existing build directory. I interpreted " "your input\n\"" + build_dir + "\" as:\n " + FilePathToUTF8(build_dir_path) + - "\nwhich doesn't seem to contain a previously-generated build.") - .PrintToStdout(); + "\nwhich doesn't seem to contain a previously-generated build."); return false; } } @@ -703,7 +704,7 @@ return true; } -bool Setup::FillPythonPath(const base::CommandLine& cmdline) { +bool Setup::FillPythonPath(const base::CommandLine& cmdline, Err* err) { // Trace this since it tends to be a bit slow on Windows. ScopedTrace setup_trace(TraceItem::TRACE_SETUP, "Fill Python Path"); const Value* value = dotfile_scope_.GetValue("script_executable", true); @@ -711,9 +712,7 @@ build_settings_.set_python_path( cmdline.GetSwitchValuePath(switches::kScriptExecutable)); } else if (value) { - Err err; - if (!value->VerifyTypeIs(Value::STRING, &err)) { - err.PrintToStdout(); + if (!value->VerifyTypeIs(Value::STRING, err)) { return false; } build_settings_.set_python_path( @@ -735,28 +734,25 @@ return true; } -bool Setup::RunConfigFile() { +bool Setup::RunConfigFile(Err* err) { if (scheduler_.verbose_logging()) scheduler_.Log("Got dotfile", FilePathToUTF8(dotfile_name_)); dotfile_input_file_ = std::make_unique<InputFile>(SourceFile("//.gn")); if (!dotfile_input_file_->Load(dotfile_name_)) { - Err(Location(), "Could not load dotfile.", - "The file \"" + FilePathToUTF8(dotfile_name_) + "\" couldn't be loaded") - .PrintToStdout(); + *err = Err(Location(), "Could not load dotfile.", + "The file \"" + FilePathToUTF8(dotfile_name_) + + "\" couldn't be loaded"); return false; } - Err err; - dotfile_tokens_ = Tokenizer::Tokenize(dotfile_input_file_.get(), &err); - if (err.has_error()) { - err.PrintToStdout(); + dotfile_tokens_ = Tokenizer::Tokenize(dotfile_input_file_.get(), err); + if (err->has_error()) { return false; } - dotfile_root_ = Parser::Parse(dotfile_tokens_, &err); - if (err.has_error()) { - err.PrintToStdout(); + dotfile_root_ = Parser::Parse(dotfile_tokens_, err); + if (err->has_error()) { return false; } @@ -767,17 +763,15 @@ // Also add a build dependency to the scope, which is used by `gn analyze`. dotfile_scope_.AddBuildDependencyFile(SourceFile("//.gn")); - dotfile_root_->Execute(&dotfile_scope_, &err); - if (err.has_error()) { - err.PrintToStdout(); + dotfile_root_->Execute(&dotfile_scope_, err); + if (err->has_error()) { return false; } return true; } -bool Setup::FillOtherConfig(const base::CommandLine& cmdline) { - Err err; +bool Setup::FillOtherConfig(const base::CommandLine& cmdline, Err* err) { SourceDir current_dir("//"); Label root_target_label(current_dir, ""); @@ -786,8 +780,7 @@ const Value* secondary_value = dotfile_scope_.GetValue("secondary_source", true); if (secondary_value) { - if (!secondary_value->VerifyTypeIs(Value::STRING, &err)) { - err.PrintToStdout(); + if (!secondary_value->VerifyTypeIs(Value::STRING, err)) { return false; } build_settings_.SetSecondarySourcePath( @@ -798,8 +791,7 @@ const Value* build_file_extension_value = dotfile_scope_.GetValue("build_file_extension", true); if (build_file_extension_value) { - if (!build_file_extension_value->VerifyTypeIs(Value::STRING, &err)) { - err.PrintToStdout(); + if (!build_file_extension_value->VerifyTypeIs(Value::STRING, err)) { return false; } @@ -807,8 +799,8 @@ auto normalized_extension = UTF8ToFilePath(extension).value(); if (normalized_extension.find_first_of(base::FilePath::kSeparators) != base::FilePath::StringType::npos) { - Err(Location(), "Build file extension '" + extension + "' cannot " + - "contain a path separator").PrintToStdout(); + *err = Err(Location(), "Build file extension '" + extension + + "' cannot " + "contain a path separator"); return false; } loader_->set_build_file_extension(extension); @@ -818,8 +810,7 @@ const Value* ninja_required_version_value = dotfile_scope_.GetValue("ninja_required_version", true); if (ninja_required_version_value) { - if (!ninja_required_version_value->VerifyTypeIs(Value::STRING, &err)) { - err.PrintToStdout(); + if (!ninja_required_version_value->VerifyTypeIs(Value::STRING, err)) { return false; } std::optional<Version> version = @@ -836,15 +827,13 @@ // Root build file. const Value* root_value = dotfile_scope_.GetValue("root", true); if (root_value) { - if (!root_value->VerifyTypeIs(Value::STRING, &err)) { - err.PrintToStdout(); + if (!root_value->VerifyTypeIs(Value::STRING, err)) { return false; } root_target_label = Label::Resolve(current_dir, std::string_view(), Label(), - *root_value, &err); - if (err.has_error()) { - err.PrintToStdout(); + *root_value, err); + if (err->has_error()) { return false; } } @@ -863,8 +852,7 @@ "didn't specify a \"buildconfig\" value.") .PrintToStdout(); return false; - } else if (!build_config_value->VerifyTypeIs(Value::STRING, &err)) { - err.PrintToStdout(); + } else if (!build_config_value->VerifyTypeIs(Value::STRING, err)) { return false; } build_settings_.set_build_config_file( @@ -876,9 +864,8 @@ if (check_targets_value) { check_patterns_.reset(new std::vector<LabelPattern>); ExtractListOfLabelPatterns(&build_settings_, *check_targets_value, - current_dir, check_patterns_.get(), &err); - if (err.has_error()) { - err.PrintToStdout(); + current_dir, check_patterns_.get(), err); + if (err->has_error()) { return false; } } @@ -886,8 +873,7 @@ const Value* check_system_includes_value = dotfile_scope_.GetValue("check_system_includes", true); if (check_system_includes_value) { - if (!check_system_includes_value->VerifyTypeIs(Value::BOOLEAN, &err)) { - err.PrintToStdout(); + if (!check_system_includes_value->VerifyTypeIs(Value::BOOLEAN, err)) { return false; } check_system_includes_ = check_system_includes_value->boolean_value(); @@ -898,20 +884,17 @@ dotfile_scope_.GetValue("exec_script_whitelist", true); if (exec_script_whitelist_value) { // Fill the list of targets to check. - if (!exec_script_whitelist_value->VerifyTypeIs(Value::LIST, &err)) { - err.PrintToStdout(); + if (!exec_script_whitelist_value->VerifyTypeIs(Value::LIST, err)) { return false; } std::unique_ptr<SourceFileSet> whitelist = std::make_unique<SourceFileSet>(); for (const auto& item : exec_script_whitelist_value->list_value()) { - if (!item.VerifyTypeIs(Value::STRING, &err)) { - err.PrintToStdout(); + if (!item.VerifyTypeIs(Value::STRING, err)) { return false; } - whitelist->insert(current_dir.ResolveRelativeFile(item, &err)); - if (err.has_error()) { - err.PrintToStdout(); + whitelist->insert(current_dir.ResolveRelativeFile(item, err)); + if (err->has_error()) { return false; } } @@ -922,8 +905,7 @@ const Value* default_args_value = dotfile_scope_.GetValue("default_args", true); if (default_args_value) { - if (!default_args_value->VerifyTypeIs(Value::SCOPE, &err)) { - err.PrintToStdout(); + if (!default_args_value->VerifyTypeIs(Value::SCOPE, err)) { return false; } @@ -933,8 +915,7 @@ const Value* arg_file_template_value = dotfile_scope_.GetValue("arg_file_template", true); if (arg_file_template_value) { - if (!arg_file_template_value->VerifyTypeIs(Value::STRING, &err)) { - err.PrintToStdout(); + if (!arg_file_template_value->VerifyTypeIs(Value::STRING, err)) { return false; } SourceFile path(arg_file_template_value->string_value());
diff --git a/src/gn/setup.h b/src/gn/setup.h index 25389f1..bf1e1ca 100644 --- a/src/gn/setup.h +++ b/src/gn/setup.h
@@ -57,6 +57,12 @@ bool force_create, const base::CommandLine& cmdline); + // Same as DoSetup() but used for tests to capture error output. + bool DoSetupWithErr(const std::string& build_dir, + bool force_create, + const base::CommandLine& cmdline, + Err* err); + // Runs the load, returning true on success. On failure, prints the error // and returns false. This includes both RunPreMessageLoop() and // RunPostMessageLoop(). @@ -117,35 +123,38 @@ bool RunPostMessageLoop(const base::CommandLine& cmdline); // Fills build arguments. Returns true on success. - bool FillArguments(const base::CommandLine& cmdline); + bool FillArguments(const base::CommandLine& cmdline, Err* err); // Fills the build arguments from the command line or from the build arg file. - bool FillArgsFromCommandLine(const std::string& args); - bool FillArgsFromFile(); + bool FillArgsFromCommandLine(const std::string& args, Err* err); + bool FillArgsFromFile(Err* err); // Given an already-loaded args_input_file_, parses and saves the resulting // arguments. Backend for the different FillArgs variants. - bool FillArgsFromArgsInputFile(); + bool FillArgsFromArgsInputFile(Err* err); // Writes the build arguments to the build arg file. bool SaveArgsToFile(); - // Fills the root directory into the settings. Returns true on success. - bool FillSourceDir(const base::CommandLine& cmdline); + // Fills the root directory into the settings. Returns true on success, or + // |err| filled out. + bool FillSourceDir(const base::CommandLine& cmdline, Err* err); // Fills the build directory given the value the user has specified. // Must happen after FillSourceDir so we can resolve source-relative // paths. If require_exists is false, it will fail if the dir doesn't exist. - bool FillBuildDir(const std::string& build_dir, bool require_exists); + bool FillBuildDir(const std::string& build_dir, + bool require_exists, + Err* err); // Fills the python path portion of the command line. On failure, sets // it to just "python". - bool FillPythonPath(const base::CommandLine& cmdline); + bool FillPythonPath(const base::CommandLine& cmdline, Err* err); // Run config file. - bool RunConfigFile(); + bool RunConfigFile(Err* err); - bool FillOtherConfig(const base::CommandLine& cmdline); + bool FillOtherConfig(const base::CommandLine& cmdline, Err* err); BuildSettings build_settings_; scoped_refptr<LoaderImpl> loader_;
diff --git a/src/gn/setup_unittest.cc b/src/gn/setup_unittest.cc index eda66ab..af10a6e 100644 --- a/src/gn/setup_unittest.cc +++ b/src/gn/setup_unittest.cc
@@ -11,6 +11,7 @@ #include "gn/filesystem_utils.h" #include "gn/switches.h" #include "gn/test_with_scheduler.h" +#include "util/build_config.h" using SetupTest = TestWithScheduler; @@ -45,7 +46,9 @@ EXPECT_EQ(gen_deps[0], base::MakeAbsoluteFilePath(dot_gn_name)); } -static void RunExtensionCheckTest(std::string extension, bool success) { +static void RunExtensionCheckTest(std::string extension, + bool success, + const std::string& expected_error_message) { base::CommandLine cmdline(base::CommandLine::NO_PROGRAM); // Create a temp directory containing a .gn file and a BUILDCONFIG.gn file, @@ -54,8 +57,10 @@ ASSERT_TRUE(in_temp_dir.CreateUniqueTempDir()); base::FilePath in_path = in_temp_dir.GetPath(); base::FilePath dot_gn_name = in_path.Append(FILE_PATH_LITERAL(".gn")); - WriteFile(dot_gn_name, "buildconfig = \"//BUILDCONFIG.gn\"\n\ - build_file_extension = \"" + extension + "\""); + WriteFile(dot_gn_name, + "buildconfig = \"//BUILDCONFIG.gn\"\n\ + build_file_extension = \"" + + extension + "\""); WriteFile(in_path.Append(FILE_PATH_LITERAL("BUILDCONFIG.gn")), ""); cmdline.AppendSwitchASCII(switches::kRoot, FilePathToUTF8(in_path)); @@ -65,16 +70,25 @@ // Run setup and check that its status. Setup setup; + Err err; EXPECT_EQ(success, - setup.DoSetup(FilePathToUTF8(build_temp_dir.GetPath()), true, cmdline)); + setup.DoSetupWithErr(FilePathToUTF8(build_temp_dir.GetPath()), true, + cmdline, &err)); + EXPECT_EQ(success, !err.has_error()); + EXPECT_EQ(expected_error_message, err.message()); } TEST_F(SetupTest, NoSeparatorInExtension) { RunExtensionCheckTest( - "hello" + std::string(1, base::FilePath::kSeparators[0]) + "world", - false); + "hello" + std::string(1, base::FilePath::kSeparators[0]) + "world", false, +#if defined(OS_WIN) + "Build file extension 'hello\\world' cannot contain a path separator" +#else + "Build file extension 'hello/world' cannot contain a path separator" +#endif + ); } TEST_F(SetupTest, Extension) { - RunExtensionCheckTest("yay", true); + RunExtensionCheckTest("yay", true, ""); }