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

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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|.
Created attachment 8606579 [details] [diff] [review]
Part 1: Add Logging.h
Attachment #8606579 - Flags: review?(nfroyd)
Created attachment 8606580 [details] [diff] [review]
Part 2: Replace prlog.h with Logging.h
Attachment #8606580 - Flags: review?(nfroyd)
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
Created attachment 8606587 [details] [diff] [review]
Part 2: Replace prlog.h with Logging.h

My sed-fu was weak, I missed a few '>' characters.
Attachment #8606587 - Flags: review?(nfroyd)
Attachment #8606580 - Attachment is obsolete: true
Attachment #8606580 - Flags: review?(nfroyd)
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?
Attachment #8606579 - Flags: review?(nfroyd)
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.
Attachment #8606587 - Flags: review?(nfroyd) → review+
(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.
(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'
(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).
(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...
Attachment #8606579 - Flags: review+
Created attachment 8607751 [details] [diff] [review]
Part 2: Replace prlog.h with Logging.h. rs=froydnj

Cleaned up some bitrot, updated to rs=froydnj
Comment on attachment 8607751 [details] [diff] [review]
Part 2: Replace prlog.h with Logging.h. rs=froydnj

Carrying forward r+
Attachment #8607751 - Flags: review+
Attachment #8606587 - Attachment is obsolete: true
(See comment #6)
Keywords: checkin-needed
Tree's back open, I'll take care of it.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/06906e5fa9b5
https://hg.mozilla.org/mozilla-central/rev/9fb7acc6f108
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.