941595 - Build with system-cairo is broken

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

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.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

One (among the others) culprint is Bug 931404.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

... which should not have changed which directories have those flags. Can you point to the change that did?

Comment 3

[Martin Stránský [:stransky] (ni? me)]

(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?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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>.

Comment 5

[Robert Longson [:longsonr]]

This seems to be the same issue as bug 940373

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Actually the culprint is Bug 845874 which adds pixman.h to nsRegion.h file. Sorry for the confusion.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I wonder if we could just add the right flags tree-wide.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

There's the patch. But I wonder if there's some better way how to add the pixman flags there. Mike?

Comment 9

[Ivaylo Dimitrov]

Attachment: 0001-Add-pixman-flags-to-more-places.patch

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.

Comment 10

[Mike Hommey [:glandium]]

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.

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Attachment: system wide build flags

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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?

Comment 13

[Martin Stránský [:stransky] (ni? me)]

Attachment: removes pixman from local includes

Pixman remove patch. Try build for those two patches: https://tbpl.mozilla.org/?tree=Try&rev=fa2db9e19f3d

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8338548 [details] [diff] [review]
removes pixman from local includes

The try build looks okay. Mike can you check the patches?

Comment 15

[Mike Hommey [:glandium]]

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.

Comment 16

[Mike Hommey [:glandium]]

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.

Comment 17

[Martin Stránský [:stransky] (ni? me)]

Attachment: complete patch, v2

Comment 18

[Martin Stránský [:stransky] (ni? me)]

> 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 19

[Mike Hommey [:glandium]]

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.

Comment 20

[Martin Stránský [:stransky] (ni? me)]

Attachment: system pixman, v3

Thanks, there's the one with MOZ_PIXMAN_CFLAGS.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

This has a ton of conflicts. Please rebase.

Comment 23

[Martin Stránský [:stransky] (ni? me)]

Attachment: system pixman, for check-in

unbitrotten patch, try run - https://tbpl.mozilla.org/?tree=Try&rev=9d03d1e74e2c

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4db1dae48435

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4db1dae48435