Closed Bug 950840 Opened 6 years ago Closed 6 years ago

Build is broken when --enable-logging and --enable-warnings-as-errors is used

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: briansmith, Assigned: ehsan)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #950505 +++

From Bug #950505 comment #12:
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #11)
> > (In reply to comment #10)
> > > Created attachment 8348216 [details]
> > >   --> https://bugzilla.mozilla.org/attachment.cgi?id=8348216&action=edit
> > > .mozconfig
> > 
> > I bet --enable-logging is causing this.  Please file a new bug.  Thanks!
> 
> Comment #3 explains very clearly that the problem is with LOG. Why did you
> check these patches back in knowing that there is a bug with them, and after
> the bug was clearly pointed out, and without giving me even 24 hours to
> reply to your request? Lots of people build with --enable-logging and it
> isn't like I backed out the patches for fun.
Summary: Build is broken when --enable-logging is used → Build is broken when --enable-logging and --enable-warnings-as-errors is used
Actually I'm tired of having the chromium headers break us with their LOG macro.  I'll fix that instead I guess.
Severity: blocker → normal
Component: Video/Audio → IPC
Priority: P1 → --
Comment on attachment 8348471 [details] [diff] [review]
Rename the chromium LOG macro to CHROMIUM_LOG

I'm not sure why but I still cannot reproduce your build failures, but this should fix it I believe.  Can you please test this to make sure?

Thanks!
Attachment #8348471 - Flags: review?(brian)
Ehsan, these build failures are not due to LOG defined in the Chromium headers, but due to LOG defined in the source files in this directory.

I saw that some source files already had an #undef LOG. I absolutely cannot understand why you wouldn't get the same build failures I'm getting with the same .mozconfig.

Here is a patch that fixes the problem for me, by brute force #undef LOG in every .cpp file under content/media.
Assignee: ehsan → brian
Status: NEW → ASSIGNED
Attachment #8348544 - Flags: review?(ehsan)
Component: IPC → Video/Audio
Comment on attachment 8348544 [details] [diff] [review]
#undef LOG in every file in content/media before #define LOG

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

Chris Double did not like this the last time I tried something like this, so I'll let him review this code.  But last time he wanted me to rename those log variables to things like DIRECTSHOW_LOG, etc.
Attachment #8348544 - Flags: review?(ehsan) → review?(chris.double)
Comment on attachment 8348471 [details] [diff] [review]
Rename the chromium LOG macro to CHROMIUM_LOG

I'll take this patch to another bug.
Attachment #8348471 - Attachment is obsolete: true
Attachment #8348471 - Flags: review?(brian)
OK I finally repro'ed this, and have a patch.  Will attach it soon.
Assignee: brian → ehsan
Attachment #8348544 - Attachment is obsolete: true
Attachment #8348544 - Flags: review?(chris.double)
Comment on attachment 8348893 [details] [diff] [review]
Rename some logging macros in content/media/wmf

Let's see who gets to review this first.
Attachment #8348893 - Flags: review?(roc)
Attachment #8348893 - Flags: review?(chris.double)
Attachment #8348893 - Flags: review?(brian)
Comment on attachment 8348893 [details] [diff] [review]
Rename some logging macros in content/media/wmf

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

I'll snake this review, as I maintain the code here.

::: content/media/wmf/WMFByteStream.cpp
@@ +25,5 @@
>  namespace mozilla {
>  
>  #ifdef PR_LOGGING
>  PRLogModuleInfo* gWMFByteStreamLog = nullptr;
> +#define WMF_LOG(...) PR_LOG(gWMFByteStreamLog, PR_LOG_DEBUG, (__VA_ARGS__))

Call this WMF_BS_LOG. This is not a generic WMF log, it's a log for a sub component.

::: content/media/wmf/WMFSourceReaderCallback.cpp
@@ +10,5 @@
>  namespace mozilla {
>  
>  #ifdef PR_LOGGING
>  static PRLogModuleInfo* gWMFSourceReaderCallbackLog = nullptr;
> +#define WMF_READER_LOG(...) PR_LOG(gWMFSourceReaderCallbackLog, PR_LOG_DEBUG, (__VA_ARGS__))

Call this WMF_CB_LOG. It's not the log for a reader component, but a sub component.
Attachment #8348893 - Flags: review?(brian) → review+
Comment on attachment 8348893 [details] [diff] [review]
Rename some logging macros in content/media/wmf

Thanks, cpearce!
Attachment #8348893 - Flags: review?(roc)
Attachment #8348893 - Flags: review?(chris.double)
https://hg.mozilla.org/mozilla-central/rev/81820a46dee2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Duplicate of this bug: 951537
You need to log in before you can comment on or make changes to this bug.