1161186 - Cannot build with PR_LOGGING unset

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Nathan Toone [:toonetown]]

The change in Bug 1160064 sets the value of gMediaDecoderLog on line 130 - however, the extern gMediaDecoderLog is only set when PR_LOGGING is defined.

Comment 1

[Nathan Toone [:toonetown]]

Attachment: bug-1161186.diff

Just wrap the usage with #ifdef

Comment 2

[Bobby Holley (:bholley)]

Isn't PR_LOGGING supposed to be true on all builds at this point? I at least have been operating on that assumption. I think we should just remove the #ifdefs.

Comment 3

[Nathan Toone [:toonetown]]

I don't know.  All I know is that for some reason it's not true for my builds...but I'm building with --disable-logging.

If --disable-logging shouldn't be doing anything anymore, then I say go ahead, remove the ifdefs, and get rid of --disable-logging as well.  However, if the option exists, it shouldn't break existing (working) builds.

Comment 4

[Eric Rahm [:erahm]]

(In reply to Nathan Toone from comment #3)
> I don't know.  All I know is that for some reason it's not true for my
> builds...but I'm building with --disable-logging.
> 
> If --disable-logging shouldn't be doing anything anymore, then I say go
> ahead, remove the ifdefs, and get rid of --disable-logging as well. 
> However, if the option exists, it shouldn't break existing (working) builds.

Yes, the issue is that |--disable-logging| is still an option [1]. At this point we could remove that option; if that's something you feel we should do lets get a bug on file. This would certainly clean up a lot of needless ifdefs!

[1] https://hg.mozilla.org/mozilla-central/annotate/34828fed1639/configure.in#l6949

Comment 5

[Nathan Toone [:toonetown]]

Personally, I like --disable-logging...particularly on android - so that it doesn't fill up my logcat log.  That, however, should be a separate bug (in my opinion), and as long as it's an option, compilation should at least work (thus the patch attached to this bug should probably be applied)

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8601059 [details] [diff] [review]
bug-1161186.diff

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

I filed bug 1161238. As for this particular bug, please fix it by removing the other |#ifdef PR_LOGGING| instance(s) around this log module rather than adding more here. :-)

Thanks for stepping up and getting this sorted out!

Comment 7

[Bobby Holley (:bholley)]

(In reply to Nathan Toone from comment #5)
> Personally, I like --disable-logging...particularly on android - so that it
> doesn't fill up my logcat log.

Logging still has no effect without the NSPR_LOG_MODULES environmental variable set, right?

> That, however, should be a separate bug (in
> my opinion), and as long as it's an option, compilation should at least work
> (thus the patch attached to this bug should probably be applied)

See comment 5, sorry for mid-airing.

Comment 8

[Nathan Toone [:toonetown]]

Attachment: bug-1161186.diff

Remove the other #ifdef instead of adding a new one.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8601112 [details] [diff] [review]
bug-1161186.diff

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

Excellent, thank you!

Comment 10

[Carsten Book [:Tomcat]]

can we get a try run for changes, thanks!

Comment 11

[Nathan Toone [:toonetown]]

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

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Gerald - can you see if this patch still matters and land it if needed?

Comment 13

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

PR_LOGGING was completely removed in bug 1163201, as well as --disable-logging in bug 1161238.
Also gMediaDecoderLog is now a statically-initialized LazyLogModule object.

So this bug (and patch) is not relevant anymore.
I'll mark it as fixed, through the magic of the above-mentioned bugs.