1165518 - Switch over from "prlog.h" to "mozilla/Logging.h"

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eric Rahm [:erahm]]

Add a skeleton header that simply passes through to "prlog.h" and swap out all variations of |#include "prlog.h"| to |#include "mozilla/Logging.h|.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Part 1: Add Logging.h

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 2: Replace prlog.h with Logging.h

Comment 3

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c898ff1c0f93

Comment 4

[Eric Rahm [:erahm]]

Comment on attachment 8606580 [details] [diff] [review]
Part 2: Replace prlog.h with Logging.h

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

Note: this excludes instances in nsprpub, security/nss, and a few config files which probably still want to use prlog.h

Comment 5

[Eric Rahm [:erahm]]

Attachment: Part 2: Replace prlog.h with Logging.h

My sed-fu was weak, I missed a few '>' characters.

Comment 6

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=049177984cae

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8606579 [details] [diff] [review]
Part 1: Add Logging.h

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

So I have no problem with the text of the patch, but I'm a little unsure what this patch and the next really do for us in terms of converting to Logging 2.0:

- We don't even use MOZ_LOG in the next patch (and it sounds like we are not making that conversion in this bug...if ever?);
- Having this be a pass-through at the moment either:
  a) implicitly assumes that we like the PR_LOG-style logging (I haven't read through the mega-logging bug in a while, so I don't remember what was the consensus--if there was one--there; or
  b) does nothing for us, because we're going to have to change all the PR_LOG/MOZ_LOG calls anyway if we decide there's a different style we should use.

Can you explain the rationale here a bit more, and maybe where you expect this work to be going?

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8606587 [details] [diff] [review]
Part 2: Replace prlog.h with Logging.h

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

rs=me (I'm assuming you did something like |sed -e 's/#include "prlog.h"/#include "mozilla/Logging.h"/'| to generate this patch), but let's get the first part worked out.

Comment 9

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #7)
> Comment on attachment 8606579 [details] [diff] [review]
> Part 1: Add Logging.h
> 
> Review of attachment 8606579 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So I have no problem with the text of the patch, but I'm a little unsure
> what this patch and the next really do for us in terms of converting to
> Logging 2.0:
> 
> - We don't even use MOZ_LOG in the next patch (and it sounds like we are not
> making that conversion in this bug...if ever?);
> - Having this be a pass-through at the moment either:
>   a) implicitly assumes that we like the PR_LOG-style logging (I haven't
> read through the mega-logging bug in a while, so I don't remember what was
> the consensus--if there was one--there; or
>   b) does nothing for us, because we're going to have to change all the
> PR_LOG/MOZ_LOG calls anyway if we decide there's a different style we should
> use.
> 
> Can you explain the rationale here a bit more, and maybe where you expect
> this work to be going?

There's a parent bug 1160285 for adding a "stub" replacement to PR_LOG_*. My goal is to slowly replace the logging functionality provided by prlog rather than swap out wholesale. For part 1 (this bug) I just want get everything including our Logging.h file rather than NSPR's.

From here we'll be able to start mass replacing PR_LOG references. Once that is done we can then start improving functionality at the gecko level rather than the NSPR level. And then hopefully one day we will remove our dependency on NSPR logging completely.

As far as why |MOZ_LOG| is in there now, I guess I was try to show my intent. I can remove it and add it in when we actually start using it.

Comment 10

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #8)
> Comment on attachment 8606587 [details] [diff] [review]
> Part 2: Replace prlog.h with Logging.h
> 
> Review of attachment 8606587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> rs=me (I'm assuming you did something like |sed -e 's/#include
> "prlog.h"/#include "mozilla/Logging.h"/'| to generate this patch), but let's
> get the first part worked out.

Yes a combo of grepping to get a list of files, removing ones we didn't want to change (nsprpub, nss, config files), and then running sed on that list of files. I believe my command was something like:

> grep -r "prlog.h" * > pr_logging_sites.txt
> vim pr_logging_sites.txt
> cut -d ':' -f 1 pr_logging_sites.txt | xargs sed -E -i 's/#include\s+("|<)prlog.h("|>)/#include "mozilla\/Logging.h"/g'

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Eric Rahm [:erahm] from comment #10)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #8)
> > Comment on attachment 8606587 [details] [diff] [review]
> > Part 2: Replace prlog.h with Logging.h
> > 
> > Review of attachment 8606587 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > rs=me (I'm assuming you did something like |sed -e 's/#include
> > "prlog.h"/#include "mozilla/Logging.h"/'| to generate this patch), but let's
> > get the first part worked out.
> 
> Yes a combo of grepping to get a list of files, removing ones we didn't want
> to change (nsprpub, nss, config files), and then running sed on that list of
> files. I believe my command was something like:
> 
> > grep -r "prlog.h" * > pr_logging_sites.txt
> > vim pr_logging_sites.txt
> > cut -d ':' -f 1 pr_logging_sites.txt | xargs sed -E -i 's/#include\s+("|<)prlog.h("|>)/#include "mozilla\/Logging.h"/g'

Optionally easier to do something like:

find . \( -path nsprpub/\* -o -path nss/\* \) -prune -o -name '*.cpp' -o -name '*.h' -o -name '*.c' | xargs sed -i ...

which requires less manual intervention (or filter find's results through grep -v if you can't remember the -prune syntax).

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Eric Rahm [:erahm] from comment #9)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #7)
> > Comment on attachment 8606579 [details] [diff] [review]
> > Part 1: Add Logging.h
> > 
> > Review of attachment 8606579 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > So I have no problem with the text of the patch, but I'm a little unsure
> > what this patch and the next really do for us in terms of converting to
> > Logging 2.0:
> > 
> > - We don't even use MOZ_LOG in the next patch (and it sounds like we are not
> > making that conversion in this bug...if ever?);
> > - Having this be a pass-through at the moment either:
> >   a) implicitly assumes that we like the PR_LOG-style logging (I haven't
> > read through the mega-logging bug in a while, so I don't remember what was
> > the consensus--if there was one--there; or
> >   b) does nothing for us, because we're going to have to change all the
> > PR_LOG/MOZ_LOG calls anyway if we decide there's a different style we should
> > use.
> > 
> > Can you explain the rationale here a bit more, and maybe where you expect
> > this work to be going?
> 
> There's a parent bug 1160285 for adding a "stub" replacement to PR_LOG_*.

Oh.  I think that bug and this one could really go together...

Anyway, r+ on the changes, I guess we'll see how this shakes out...

Comment 13

[Eric Rahm [:erahm]]

Attachment: Part 2: Replace prlog.h with Logging.h. rs=froydnj

Cleaned up some bitrot, updated to rs=froydnj

Comment 14

[Eric Rahm [:erahm]]

Comment on attachment 8607751 [details] [diff] [review]
Part 2: Replace prlog.h with Logging.h. rs=froydnj

Carrying forward r+

Comment 15

[Eric Rahm [:erahm]]

(See comment #6)

Comment 16

[Eric Rahm [:erahm]]

Tree's back open, I'll take care of it.

Comment 17

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06906e5fa9b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb7acc6f108

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06906e5fa9b5
https://hg.mozilla.org/mozilla-central/rev/9fb7acc6f108