Closed Bug 1451686 Opened 2 years ago Closed 2 years ago

Change -MOZ_LOG* arguments to be in the form -MOZ_LOG=modules (and not -MOZ_LOG <space> modules)

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

Details

Attachments

(1 file, 2 obsolete files)

I caught myself (and not just me) to type `./firefox -MOZ_LOG=something` instead of .`/firefox -MOZ_LOG something` and being curious why it doesn't work.

I think it would be better to accept the params rather in the '=' form.  The code would be simpler as well.
Attached patch v1 (obsolete) — Splinter Review
I think self-explanatory.  Let's get this in soon, so that the `-MOZ_LOG modules` notation doesn't get spread.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #8965283 - Flags: review?(erahm)
Comment on attachment 8965283 [details] [diff] [review]
v1

Review of attachment 8965283 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine, but I'd prefer if it supported both (the other options don't seem to support the '=' variant, but in general it's pretty common to support both). Also I just noticed your patch mentions '--MOZ_LOG' but you've actually implemented '-MOZ_LOG' (one dash). '--' as a prefix would be more in line with the existing options, although IIRC they support both for historical reasons.

If you want to land this and file a follow up that's fine, otherwise maybe we can just rework it here.
Attachment #8965283 - Flags: review?(erahm) → review+
Attached patch v2 (obsolete) — Splinter Review
so, this has changed a bit more:
- we accept:
  --MOZ_LOG=module_list
  -MOZ_LOG=module_list
  --MOZ_LOG module_list
  -MOZ_LOG module_list
  (similarly for -MOZ_LOG_FILE)
- code separated to its own file, using Tokenizer
- public API via a static func with a callback (similar to NSPRLogModuleParser)
- a gtest written

I still would like to refer to this publicly with only one '-' so that it's simpler mainly for Windows users to start logging from start of the session easily (whole purpose of this bug)
Attachment #8965283 - Attachment is obsolete: true
Attachment #8967417 - Flags: review?(erahm)
Comment on attachment 8967417 [details] [diff] [review]
v2

Review of attachment 8967417 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks separating this out and adding a test, it makes it a lot easier to review :) I have a few nits, but otherwise looks good.

::: xpcom/base/LogCommandLineHandler.cpp
@@ +8,5 @@
> +
> +#include "mozilla/Tokenizer.h"
> +#include "nsDebug.h"
> +#include "nsString.h"
> +#include "prenv.h"

nit: doesn't need prenv.h

@@ +13,5 @@
> +
> +namespace mozilla {
> +
> +void LoggingHandleCommandLineArgs(int argc, char * argv[],
> +                                  std::function<void(char const*)> consumer)

WDYT about just having the callback take a `const nsACString&`?

@@ +43,5 @@
> +      }
> +      // We accept `-MOZ_LOG` as well as `--MOZ_LOG`.
> +      Unused << p.CheckChar('-');
> +
> +      for (auto name : names) {

nit: const auto&

::: xpcom/base/LogCommandLineHandler.h
@@ +9,5 @@
> +
> +#include <functional>
> +
> +namespace mozilla {
> +

Can you add a doc for this?

@@ +12,5 @@
> +namespace mozilla {
> +
> +void
> +LoggingHandleCommandLineArgs(int argc, char* argv[],
> +                             std::function<void(char const*)> consumer);

nit: const std::function<...>&

::: xpcom/base/Logging.cpp
@@ +14,5 @@
>  #include "mozilla/StaticPtr.h"
>  #include "mozilla/Printf.h"
>  #include "mozilla/Atomics.h"
>  #include "mozilla/Sprintf.h"
> +#include "mozilla/Tokenizer.h"

nit: this shouldn't be needed here, right?

@@ +215,5 @@
> +      // Scripts can pass -MOZ_LOG=$MOZ_LOG,modules as an argument
> +      // to merge existing settings, if required.
> +
> +      // PR_SetEnv takes ownership of the string.
> +      PR_SetEnv(PL_strdup(env));

We can just use `strdup` now, right?

::: xpcom/tests/gtest/TestLogCommandLineHandler.cpp
@@ +98,5 @@
> +  results.Clear();
> +  mozilla::LoggingHandleCommandLineArgs(std::size(argv4), argv4, callback);
> +  EXPECT_TRUE(results.Length() == 2);
> +  EXPECT_TRUE(NS_LITERAL_CSTRING("MOZ_LOG=modules").EqualsASCII(results[0].get()));
> +  EXPECT_TRUE(NS_LITERAL_CSTRING("MOZ_LOG_FILE=file").EqualsASCII(results[1].get()));

Can you add something like { "", "--MOZ_LOG", "modules", "-P", "foo", "--MOZ_LOG_FILE", "file" }
Attachment #8967417 - Flags: review?(erahm) → review+
Attached patch v2.1Splinter Review
All comments addressed, thanks!
Attachment #8967417 - Attachment is obsolete: true
Attachment #8967703 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40352eb9a1cf
Allow also -MOZ_LOG/_FILE=value form of the logging arguments. r=erahm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/40352eb9a1cf
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.