From 5c794e2f18850e1068c3dcf1a7730a672ba64bd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Harald=20J=C3=B8rgensen?= <58829763+adamjoer@users.noreply.github.com> Date: Tue, 10 Sep 2024 22:09:01 +0200 Subject: [PATCH 1/4] Make info, warning, and error output functions const --- src/status.h | 6 +++--- src/status_printer.cc | 6 +++--- src/status_printer.h | 6 +++--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/status.h b/src/status.h index 29db7c203a..fedc7d0385 100644 --- a/src/status.h +++ b/src/status.h @@ -39,9 +39,9 @@ struct Status { /// (which is the default). virtual void SetExplanations(Explanations*) = 0; - virtual void Info(const char* msg, ...) = 0; - virtual void Warning(const char* msg, ...) = 0; - virtual void Error(const char* msg, ...) = 0; + virtual void Info(const char* msg, ...) const = 0; + virtual void Warning(const char* msg, ...) const = 0; + virtual void Error(const char* msg, ...) const = 0; virtual ~Status() { } diff --git a/src/status_printer.cc b/src/status_printer.cc index 62c7d7a9d2..c1deda4756 100644 --- a/src/status_printer.cc +++ b/src/status_printer.cc @@ -437,21 +437,21 @@ void StatusPrinter::PrintStatus(const Edge* edge, int64_t time_millis) { force_full_command ? LinePrinter::FULL : LinePrinter::ELIDE); } -void StatusPrinter::Warning(const char* msg, ...) { +void StatusPrinter::Warning(const char* msg, ...) const { va_list ap; va_start(ap, msg); ::Warning(msg, ap); va_end(ap); } -void StatusPrinter::Error(const char* msg, ...) { +void StatusPrinter::Error(const char* msg, ...) const { va_list ap; va_start(ap, msg); ::Error(msg, ap); va_end(ap); } -void StatusPrinter::Info(const char* msg, ...) { +void StatusPrinter::Info(const char* msg, ...) const { va_list ap; va_start(ap, msg); ::Info(msg, ap); diff --git a/src/status_printer.h b/src/status_printer.h index 08a8d1a93d..5b63c69ba8 100644 --- a/src/status_printer.h +++ b/src/status_printer.h @@ -36,9 +36,9 @@ struct StatusPrinter : Status { void BuildStarted() override; void BuildFinished() override; - void Info(const char* msg, ...) override; - void Warning(const char* msg, ...) override; - void Error(const char* msg, ...) override; + void Info(const char* msg, ...) const override; + void Warning(const char* msg, ...) const override; + void Error(const char* msg, ...) const override; /// Format the progress status string by replacing the placeholders. /// See the user manual for more information about the available From 0a74baa398a41062fa03bdff8f2aa2f3f08c5a7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Harald=20J=C3=B8rgensen?= <58829763+adamjoer@users.noreply.github.com> Date: Tue, 10 Sep 2024 21:50:22 +0200 Subject: [PATCH 2/4] Move StatusPrinter tests to their own file --- CMakeLists.txt | 1 + src/build_test.cc | 29 ------------------- src/status_printer_test.cc | 58 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 29 deletions(-) create mode 100644 src/status_printer_test.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index 47b1f9c117..33544bf9ae 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -275,6 +275,7 @@ if(BUILD_TESTING) src/missing_deps_test.cc src/ninja_test.cc src/state_test.cc + src/status_printer_test.cc src/string_piece_util_test.cc src/subprocess_test.cc src/test.cc diff --git a/src/build_test.cc b/src/build_test.cc index c84190a040..69568955cc 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -2258,35 +2258,6 @@ TEST_F(BuildTest, DepsGccWithEmptyDepfileErrorsOut) { ASSERT_EQ(1u, command_runner_.commands_ran_.size()); } -TEST_F(BuildTest, StatusFormatElapsed_e) { - status_.BuildStarted(); - // Before any task is done, the elapsed time must be zero. - EXPECT_EQ("[%/e0.000]", status_.FormatProgressStatus("[%%/e%e]", 0)); -} - -TEST_F(BuildTest, StatusFormatElapsed_w) { - status_.BuildStarted(); - // Before any task is done, the elapsed time must be zero. - EXPECT_EQ("[%/e00:00]", status_.FormatProgressStatus("[%%/e%w]", 0)); -} - -TEST_F(BuildTest, StatusFormatETA) { - status_.BuildStarted(); - // Before any task is done, the ETA time must be unknown. - EXPECT_EQ("[%/E?]", status_.FormatProgressStatus("[%%/E%E]", 0)); -} - -TEST_F(BuildTest, StatusFormatTimeProgress) { - status_.BuildStarted(); - // Before any task is done, the percentage of elapsed time must be zero. - EXPECT_EQ("[%/p 0%]", status_.FormatProgressStatus("[%%/p%p]", 0)); -} - -TEST_F(BuildTest, StatusFormatReplacePlaceholder) { - EXPECT_EQ("[%/s0/t0/r0/u0/f0]", - status_.FormatProgressStatus("[%%/s%s/t%t/r%r/u%u/f%f]", 0)); -} - TEST_F(BuildTest, FailedDepsParse) { ASSERT_NO_FATAL_FAILURE(AssertParse(&state_, "build bad_deps.o: cat in1\n" diff --git a/src/status_printer_test.cc b/src/status_printer_test.cc new file mode 100644 index 0000000000..ab9633495a --- /dev/null +++ b/src/status_printer_test.cc @@ -0,0 +1,58 @@ +// Copyright 2024 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "status_printer.h" + +#include "build.h" +#include "test.h" + +struct StatusPrinterTest : public testing::Test { + StatusPrinterTest() : config_(MakeConfig()), status_(config_) {} + + virtual void SetUp() { status_.BuildStarted(); } + + static BuildConfig MakeConfig() { + BuildConfig config; + config.verbosity = BuildConfig::QUIET; + return config; + } + + BuildConfig config_; + StatusPrinter status_; +}; + +TEST_F(StatusPrinterTest, StatusFormatElapsed_e) { + // Before any task is done, the elapsed time must be zero. + EXPECT_EQ("[%/e0.000]", status_.FormatProgressStatus("[%%/e%e]", 0)); +} + +TEST_F(StatusPrinterTest, StatusFormatElapsed_w) { + // Before any task is done, the elapsed time must be zero. + EXPECT_EQ("[%/e00:00]", status_.FormatProgressStatus("[%%/e%w]", 0)); +} + +TEST_F(StatusPrinterTest, StatusFormatETA) { + // Before any task is done, the ETA time must be unknown. + EXPECT_EQ("[%/E?]", status_.FormatProgressStatus("[%%/E%E]", 0)); +} + +TEST_F(StatusPrinterTest, StatusFormatTimeProgress) { + // Before any task is done, the percentage of elapsed time must be zero. + EXPECT_EQ("[%/p 0%]", status_.FormatProgressStatus("[%%/p%p]", 0)); +} + +TEST_F(StatusPrinterTest, StatusFormatReplacePlaceholder) { + EXPECT_EQ("[%/s0/t0/r0/u0/f0]", + status_.FormatProgressStatus("[%%/s%s/t%t/r%r/u%u/f%f]", 0)); +} From 9df9b1c35fac4f952993e6dae15e60534a687e9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Harald=20J=C3=B8rgensen?= <58829763+adamjoer@users.noreply.github.com> Date: Wed, 11 Sep 2024 00:14:58 +0200 Subject: [PATCH 3/4] Check progress status format on init and use default format as fallback --- src/status_printer.cc | 47 +++++++++++++++++++++++++++++++++++++- src/status_printer.h | 9 ++++++++ src/status_printer_test.cc | 12 ++++++++++ 3 files changed, 67 insertions(+), 1 deletion(-) diff --git a/src/status_printer.cc b/src/status_printer.cc index c1deda4756..cdc6cb8ec4 100644 --- a/src/status_printer.cc +++ b/src/status_printer.cc @@ -49,6 +49,13 @@ StatusPrinter::StatusPrinter(const BuildConfig& config) printer_.set_smart_terminal(false); progress_status_format_ = getenv("NINJA_STATUS"); + + std::string error_output; + if (progress_status_format_ && + !IsValidProgressStatus(progress_status_format_, &error_output)) { + Warning("%s", error_output.c_str()); + progress_status_format_ = nullptr; + } if (!progress_status_format_) progress_status_format_ = "[%f/%t] "; } @@ -257,6 +264,45 @@ void StatusPrinter::BuildFinished() { printer_.PrintOnNewLine(""); } +bool StatusPrinter::IsValidProgressStatus(const char* progress_status_format, + string* error_output) { + for (const char* s = progress_status_format; *s != '\0'; ++s) { + if (*s != '%') + continue; + + ++s; + switch (*s) { + case 's': // The number of started edges. + case 't': // The total number of edges that must be run to complete the + // build. + case 'p': // The percentage of finished edges. + case 'r': // The number of currently running edges. + case 'u': // The number of remaining edges to start. + case 'f': // The number of finished edges. + case 'o': // Overall rate of finished edges per second + case 'c': // Current rate of finished edges per second (average over builds + case 'e': // Elapsed time in seconds. + case 'E': // Remaining time (ETA) in seconds. + case 'w': // Elapsed time in [h:]mm:ss format. + case 'W': // Remaining time (ETA) in [h:]mm:ss format. + case 'P': // The percentage (in ppp% format) of time elapsed out of + // predicted total runtime. + case '%': // A plain `%` character. + break; + + default: + if (error_output) { + char buffer[64]; + snprintf(buffer, sizeof(buffer), + "unknown placeholder '%%%c' in $NINJA_STATUS", *s); + *error_output = buffer; + } + return false; + } + } + return true; +} + string StatusPrinter::FormatProgressStatus(const char* progress_status_format, int64_t time_millis) const { string out; @@ -390,7 +436,6 @@ string StatusPrinter::FormatProgressStatus(const char* progress_status_format, } default: - Fatal("unknown placeholder '%%%c' in $NINJA_STATUS", *s); return ""; } } else { diff --git a/src/status_printer.h b/src/status_printer.h index 5b63c69ba8..b4f775a642 100644 --- a/src/status_printer.h +++ b/src/status_printer.h @@ -40,6 +40,15 @@ struct StatusPrinter : Status { void Warning(const char* msg, ...) const override; void Error(const char* msg, ...) const override; + /// Validate the given status format string to verify it only contains valid + /// escape sequences. On failure, an error message is copied to error_output + /// if it isn't null. + /// @param progress_status_format The format of the progress status. + /// @param error_output Where to write the error message on failure. + /// @return Whether the string is a valid progress status format. + static bool IsValidProgressStatus(const char* progress_status_format, + std::string* error_output = nullptr); + /// Format the progress status string by replacing the placeholders. /// See the user manual for more information about the available /// placeholders. diff --git a/src/status_printer_test.cc b/src/status_printer_test.cc index ab9633495a..4fe55b5c0d 100644 --- a/src/status_printer_test.cc +++ b/src/status_printer_test.cc @@ -56,3 +56,15 @@ TEST_F(StatusPrinterTest, StatusFormatReplacePlaceholder) { EXPECT_EQ("[%/s0/t0/r0/u0/f0]", status_.FormatProgressStatus("[%%/s%s/t%t/r%r/u%u/f%f]", 0)); } + +TEST_F(StatusPrinterTest, StatusFormatValidator) { + EXPECT_TRUE(StatusPrinter::IsValidProgressStatus("[%f/%t] ")); + { + std::string error_output; + EXPECT_FALSE( + StatusPrinter::IsValidProgressStatus("[%f/%X] ", &error_output)); + EXPECT_EQ("unknown placeholder '%X' in $NINJA_STATUS", error_output); + } + + EXPECT_EQ("", status_.FormatProgressStatus("[%f/%X] ", 0)); +} From 57f6ed5e72fc7ebc5125128792d1bcc0c7befc15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Harald=20J=C3=B8rgensen?= <58829763+adamjoer@users.noreply.github.com> Date: Thu, 19 Sep 2024 23:11:27 +0200 Subject: [PATCH 4/4] Move default progress status format string into constant --- src/status_printer.cc | 8 +++++--- src/status_printer.h | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/status_printer.cc b/src/status_printer.cc index cdc6cb8ec4..412bb61b5b 100644 --- a/src/status_printer.cc +++ b/src/status_printer.cc @@ -42,12 +42,14 @@ Status* Status::factory(const BuildConfig& config) { StatusPrinter::StatusPrinter(const BuildConfig& config) : config_(config), started_edges_(0), finished_edges_(0), total_edges_(0), - running_edges_(0), progress_status_format_(NULL), - current_rate_(config.parallelism) { + running_edges_(0), current_rate_(config.parallelism) { // Don't do anything fancy in verbose mode. if (config_.verbosity != BuildConfig::NORMAL) printer_.set_smart_terminal(false); + // The progress status format to use by default + static const char kDefaultProgressStatusFormat[] = "[%f/%t] "; + progress_status_format_ = getenv("NINJA_STATUS"); std::string error_output; @@ -57,7 +59,7 @@ StatusPrinter::StatusPrinter(const BuildConfig& config) progress_status_format_ = nullptr; } if (!progress_status_format_) - progress_status_format_ = "[%f/%t] "; + progress_status_format_ = kDefaultProgressStatusFormat; } void StatusPrinter::EdgeAddedToPlan(const Edge* edge) { diff --git a/src/status_printer.h b/src/status_printer.h index b4f775a642..ddf74fe186 100644 --- a/src/status_printer.h +++ b/src/status_printer.h @@ -100,7 +100,7 @@ struct StatusPrinter : Status { Explanations* explanations_ = nullptr; /// The custom progress status format to use. - const char* progress_status_format_; + const char* progress_status_format_ = nullptr; template void SnprintfRate(double rate, char (&buf)[S], const char* format) const {