Check progress status format string validity first and fall back to default if it's invalid #2496

adamjoer · 2024-09-08T19:22:18Z

In my opinion it shouldn't be fatal if the progress format string configured with $NINJA_STATUS is invalid. Especially now that we have placeholders that have only been available since Ninja 1.12.

This PR therefore changes the way the progress status format string is validated, so that it is checked in the StatusPrinter constructor, and if it's invalid the default "[%f/%t] " format is used instead. It also adds a small unit test to ensure this functionality.

mcprat · 2024-09-09T08:03:47Z

I don't think it's necessary to make a new function for this... there should be a simple way to do it

adamjoer · 2024-09-09T09:30:16Z

I don't think it's necessary to make a new function for this... there should be a simple way to do it

I agree that it's a clunky way to do it.

Would it make more sense to add a boolean out parameter to StatusPrinter::FormatProgressStatus that indicates if the format is invalid? Then the check could look something like this:

progress_status_format_ = getenv("NINJA_STATUS");
bool is_format_valid = true;
FormatProgressStatus(progress_status_format_, 0, &is_format_valid)
if (!progress_status_format_ || !is_format_valid)
  progress_status_format_ = "[%f/%t] ";

digit-google · 2024-09-09T12:46:49Z

src/status.h

-  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;


These are unrelated changes, they are useful but please move them to their own commit instead. Thanks.

digit-google · 2024-09-09T12:47:16Z

src/status_printer.h

+  void Warning(const char* msg, ...) const override;
+  void Error(const char* msg, ...) const override;
+
+  bool IsValidProgressStatus(const char* progress_status_format) const;


nit: Please make this method static.

Also, document it properly and make it return an error string, instead of calling Warning(), i.e. let the caller decide what to do, e.g.:

/// Validate the status format string passed as an argument to verify it only contains /// valid escape sequences. On success return true, on failure, return false and sets \arg error. static bool IsValidProgressStatusFormat(const char* status_format, std::string* err);

digit-google · 2024-09-09T12:51:51Z

src/build_test.cc

+  EXPECT_FALSE(status_.IsValidProgressStatus("[%f/%X] "));
+  EXPECT_EQ("", status_.FormatProgressStatus("[%f/%X] ", 0));
+}
+


It would make more sense to move this to a new status_printer_test.cc file.

mcprat · 2024-09-11T04:20:04Z

I have not looked deeper into this but at first glance, the goal should be able to be achieved with less than 10 lines changed... I don't understand how we are passing 100 lines...

Find the Fatal() call, turn it into a Warning(), and then set the format value.

am I wrong?

mcprat · 2024-09-17T21:23:42Z

@digit-google would it be acceptable to make FormatProgressStatus() non-const? that would make it take only 9 lines to do this.

digit-google · 2024-09-18T12:47:27Z

src/status_printer.cc

@@ -49,8 +49,14 @@ StatusPrinter::StatusPrinter(const BuildConfig& config)
    printer_.set_smart_terminal(false);

  progress_status_format_ = getenv("NINJA_STATUS");
-  if (!progress_status_format_)
+
+  string error_output;


nit: use std::string as we are trying to remove using namespace std from the sources gradually.

digit-google · 2024-09-18T12:54:13Z

src/status_printer.cc

    progress_status_format_ = "[%f/%t] ";
+  }


nit: For clarity use two conditions and a common variable such as:

std::string error; if (progress_status_format_ && !IsValidProgressStatus(progress_status_format_, &error)) { Warning("%s", error.c_str()); progress_status_format_ = nullptr; } if (!progress_status_format_) { // Enforce default value if unspecified or invalid. progress_status_format_ = kDefaultStatusFormat; }

digit-google · 2024-09-18T12:55:11Z

src/build_test.cc

@@ -2258,47 +2258,6 @@ TEST_F(BuildTest, DepsGccWithEmptyDepfileErrorsOut) {
  ASSERT_EQ(1u, command_runner_.commands_ran_.size());
 }

-TEST_F(BuildTest, StatusFormatElapsed_e) {


Good, but can you put this commit before the previous one, this will make the addition of BuildTest.StatusFormatValidator clearer.

digit-google · 2024-09-18T13:01:42Z

@mcprat, to be honest, I don't think it would be a good idea here. A formatting function that changes the state of its object is always confusing for the reader.

Also a single validation function that is called once before anything heavy work is done is very simple to understand.
Trying to detect the invalid value on each status output, and them modifying the formatting string there is introducing un-needed mutations. And unexpected mutations are the source of ... lots of bugs.

digit-google · 2024-09-20T08:48:33Z

src/status_printer.h

+  const char* progress_status_format_ = nullptr;
+
+  /// The progress status format to use by default
+  static constexpr const char* kDefaultProgressStatusFormat = "[%f/%t] ";


nit: static constexpr members need a definition in the corresponding .cc file or some compilers will complain that it is missing (especially when compiling with -O0). In this case you can simply add constexpr const char* StatusPrinter::kDefaultProgressStatusFormat; (with no value) to status_printer.cc.

However, we do not need to expose the value publicly, so for simplicity, just define it as a const variable in the constructor that uses it.

Also const char kName[] = "..." is preferable to static const char* kName = "..." because the latter introduces an unnecessary relocation in the generated code / data segment.

StatusPrinter::StatusPrinter(...) { ... const char kDefaultProgressStatusFormat[] = "[%f/%t] "; ... }

Sorry for the nitpicks, this is a great PR :-)

It's no problem at all, this is great feedback. :)

adamjoer · 2024-10-02T06:50:24Z

@jhasse @digit-google Any chance that this will get merged?

digit-google · 2024-10-02T09:22:29Z

The code LGTM as-is, though I would appreciate if you could address the latest review commits. I'll let @jhasse decide on the merge itself :)

digit-google reviewed Sep 9, 2024

View reviewed changes

Make info, warning, and error output functions const

5c794e2

adamjoer force-pushed the status-format-check branch 3 times, most recently from e75fabb to 5406a36 Compare September 10, 2024 22:25

digit-google reviewed Sep 18, 2024

View reviewed changes

adamjoer added 2 commits September 19, 2024 22:19

Move StatusPrinter tests to their own file

0a74baa

Check progress status format on init and use default format as fallback

9df9b1c

adamjoer force-pushed the status-format-check branch from 5406a36 to 7749393 Compare September 19, 2024 21:21

digit-google reviewed Sep 20, 2024

View reviewed changes

Move default progress status format string into constant

57f6ed5

adamjoer force-pushed the status-format-check branch from 7749393 to 57f6ed5 Compare September 22, 2024 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check progress status format string validity first and fall back to default if it's invalid #2496

Check progress status format string validity first and fall back to default if it's invalid #2496

adamjoer commented Sep 8, 2024

mcprat commented Sep 9, 2024

adamjoer commented Sep 9, 2024

digit-google Sep 9, 2024

digit-google Sep 9, 2024

digit-google Sep 9, 2024

digit-google Sep 9, 2024

mcprat commented Sep 11, 2024

mcprat commented Sep 17, 2024

digit-google Sep 18, 2024

digit-google Sep 18, 2024

digit-google Sep 18, 2024

digit-google commented Sep 18, 2024

digit-google Sep 20, 2024

adamjoer Sep 22, 2024

adamjoer commented Oct 2, 2024

digit-google commented Oct 2, 2024

Check progress status format string validity first and fall back to default if it's invalid #2496

Are you sure you want to change the base?

Check progress status format string validity first and fall back to default if it's invalid #2496

Conversation

adamjoer commented Sep 8, 2024

mcprat commented Sep 9, 2024

adamjoer commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcprat commented Sep 11, 2024

mcprat commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digit-google commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamjoer commented Oct 2, 2024

digit-google commented Oct 2, 2024