Closed Bug 941595 Opened 7 years ago Closed 7 years ago

Build with system-cairo is broken

Categories

(Firefox Build System :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: stransky, Assigned: stransky)

References

Details

Attachments

(1 file, 6 obsolete files)

The recent trunk can't be build with system cairo (I testes gtk2 and gtk3 builds). There are missing pixman build flags in some directories, so pixman.h can't be found.
One (among the others) culprint is Bug 931404.
... which should not have changed which directories have those flags. Can you point to the change that did?
(In reply to :Ms2ger from comment #2)
> ... which should not have changed which directories have those flags. Can
> you point to the change that did?

The pixman (cairo) flags we included indirectly here, via. chromium-config.mk. But it's difficult to point a single changeset which breaks it, because its broken all over the tree and it fails before it hits the content/events/src dir.

I'm just working on a patch which brigs it back. Anyway, do you mind if I add the content/events/src/Makefile.in back?
Where? chromium-config.mk doesn't seem to have done that when it was removed in <https://hg.mozilla.org/mozilla-central/diff/c031747aac8a/ipc/chromium/chromium-config.mk>.
This seems to be the same issue as bug 940373
Assignee: nobody → stransky
Actually the culprint is Bug 845874 which adds pixman.h to nsRegion.h file. Sorry for the confusion.
No longer blocks: 931404
Blocks: 845874
I wonder if we could just add the right flags tree-wide.
Attached patch patch (obsolete) — Splinter Review
There's the patch. But I wonder if there's some better way how to add the pixman flags there. Mike?
Attachment #8336787 - Flags: feedback?(mh+mozilla)
Adding pixman flags in more places is needed for Maemo 5 build to succeed. Not sure whether it is because of OMTC enabled or because of system-pixman is enabled in the build.
Yeah, considering this is going to break any time something ends up including nsRegion.h, you might as well add the pixman flags to CXXFLAGS globally.
Attachment #8336787 - Flags: feedback?(mh+mozilla) → feedback-
Attached patch system wide build flags (obsolete) — Splinter Review
Attachment #8338480 - Flags: review?(mh+mozilla)
https://mxr.mozilla.org/mozilla-central/search?string=MOZ_PIXMAN_CFLAGS shows that there's more cases in makefiles; would you feel like removing those?
Pixman remove patch. Try build for those two patches: https://tbpl.mozilla.org/?tree=Try&rev=fa2db9e19f3d
Attachment #8336787 - Attachment is obsolete: true
Attachment #8337708 - Attachment is obsolete: true
Comment on attachment 8338548 [details] [diff] [review]
removes pixman from local includes

The try build looks okay. Mike can you check the patches?
Attachment #8338548 - Flags: review?(mh+mozilla)
Attachment #8338548 - Attachment description: pixman-remove.patch → removes pixman from local includes
Comment on attachment 8338548 [details] [diff] [review]
removes pixman from local includes

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

::: gfx/tests/gtest/Makefile.in
@@ +13,5 @@
>    $(NULL)
>  
>  include $(topsrcdir)/config/rules.mk
>  
> +CXXFLAGS += $(MOZ_CAIRO_CFLAGS) $(TK_CFLAGS) 

Please remove the trailing whitespace while you're here.
Attachment #8338548 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8338480 [details] [diff] [review]
system wide build flags

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

::: gfx/tests/gtest/Makefile.in
@@ +13,5 @@
>    $(NULL)
>  
>  include $(topsrcdir)/config/rules.mk
> +
> +CXXFLAGS += $(MOZ_CAIRO_CFLAGS) $(TK_CFLAGS)

That should go in the other patch. Or fold both.

::: media/webrtc/signaling/signaling.gyp
@@ +181,5 @@
>  
>        'cflags_mozilla': [
>          '$(NSPR_CFLAGS)',
>          '$(NSS_CFLAGS)',
> +        '$(PIXMAN_CFLAGS)',

I don't think this is needed, and PIXMAN_CFLAGS is unlikely to match an existing variable.
Attachment #8338480 - Flags: review?(mh+mozilla) → feedback+
Attached patch complete patch, v2 (obsolete) — Splinter Review
Attachment #8339874 - Flags: review?(mh+mozilla)
Attachment #8338480 - Attachment is obsolete: true
Attachment #8338548 - Attachment is obsolete: true
> Review of attachment 8338480 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/tests/gtest/Makefile.in
> @@ +13,5 @@
> >    $(NULL)
> >  
> >  include $(topsrcdir)/config/rules.mk
> > +
> > +CXXFLAGS += $(MOZ_CAIRO_CFLAGS) $(TK_CFLAGS)
> 
> That should go in the other patch. Or fold both.

Fixed by the complete one.

> ::: media/webrtc/signaling/signaling.gyp
> @@ +181,5 @@
> >  
> >        'cflags_mozilla': [
> >          '$(NSPR_CFLAGS)',
> >          '$(NSS_CFLAGS)',
> > +        '$(PIXMAN_CFLAGS)',
> 
> I don't think this is needed, and PIXMAN_CFLAGS is unlikely to match an
> existing variable.

I was surprised too but it is really needed and it works.
Comment on attachment 8339874 [details] [diff] [review]
complete patch, v2

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

::: media/webrtc/signaling/signaling.gyp
@@ +181,5 @@
>  
>        'cflags_mozilla': [
>          '$(NSPR_CFLAGS)',
>          '$(NSS_CFLAGS)',
> +        '$(PIXMAN_CFLAGS)',

Well, that works because PKG_CHECK_MODULES exports it, but it's also something that only happens when building with system pixman. While the setup with the in-tree pixman has the value empty, it's still more correct to use MOZ_PIXMAN_CFLAGS.
Attachment #8339874 - Flags: review?(mh+mozilla) → feedback+
Attached patch system pixman, v3 (obsolete) — Splinter Review
Thanks, there's the one with MOZ_PIXMAN_CFLAGS.
Attachment #8339874 - Attachment is obsolete: true
Attachment #8341042 - Flags: review?(mh+mozilla)
Attachment #8341042 - Flags: review?(mh+mozilla) → review+
Duplicate of this bug: 940373
Keywords: checkin-needed
This has a ton of conflicts. Please rebase.
Keywords: checkin-needed
Attachment #8344635 - Attachment description: system pixman, updated → system pixman, for check-in
Keywords: checkin-needed
Attachment #8341042 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/4db1dae48435
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 993467
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.