1305145 - Move --enable-tree-freetype to Python configure

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1305145 - Forward pkg-config environment variables from mozconfig to Python configure's pkg_check_modules.

Review commit: https://reviewboard.mozilla.org/r/83450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83450/

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1305145 - Make libs and flags set by pkg_check_modules available to the caller.

Review commit: https://reviewboard.mozilla.org/r/83452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83452/

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1305145 - Move Pango detection to Python configure.

A test against MOZ_PANGO is also removed, this has not been
set since bug 947379.

Review commit: https://reviewboard.mozilla.org/r/83454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83454/

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1305145 - Move fontconfig detection to Python configure.

Review commit: https://reviewboard.mozilla.org/r/83456/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83456/

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1305145 - Move freetype2 detection to Python configure.

Review commit: https://reviewboard.mozilla.org/r/83458/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83458/

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1305145 - Move --enable-tree-freetype to Python configure.

Review commit: https://reviewboard.mozilla.org/r/83460/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83460/

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1305145 - Move --enable-system-pixman and --enable-system-cairo to Python configure.

Review commit: https://reviewboard.mozilla.org/r/83462/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83462/

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8797838 [details]
Bug 1305145 - Forward pkg-config environment variables from mozconfig to Python configure's pkg_check_modules.

https://reviewboard.mozilla.org/r/83450/#review82114

This looks good, but this should be attached to bug 1298740 instead.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8797839 [details]
Bug 1305145 - Make libs and flags set by pkg_check_modules available to the caller.

https://reviewboard.mozilla.org/r/83452/#review82116

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8797840 [details]
Bug 1305145 - Move Pango detection to Python configure.

https://reviewboard.mozilla.org/r/83454/#review82118

::: old-configure.in
(Diff revision 1)
> -            MOZ_PANGO_CFLAGS="$MOZ_PANGO_CFLAGS $_FONTCONFIG_CFLAGS"
> -            MOZ_PANGO_LIBS="$MOZ_PANGO_LIBS $_FONTCONFIG_LIBS"

This removal likely breaks building gfx/thebes, which uses fontconfig types.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8797840 [details]
Bug 1305145 - Move Pango detection to Python configure.

https://reviewboard.mozilla.org/r/83454/#review82120

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8797841 [details]
Bug 1305145 - Move fontconfig detection to Python configure.

https://reviewboard.mozilla.org/r/83456/#review82122

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8797842 [details]
Bug 1305145 - Move freetype2 detection to Python configure.

https://reviewboard.mozilla.org/r/83458/#review82124

::: toolkit/moz.configure:304
(Diff revision 1)
> +    if not freetype2_info:
> +        return
> +    if not fontconfig_info:
> +        return freetype2_info
> +    return namespace(
> +        cflags=tuple(set(fontconfig_info.cflags + freetype2_info.cflags)),

The order of cflags /can/ be important. I'd rather avoid using sets for this (especially when this could be cargo-culted)

::: toolkit/moz.configure:305
(Diff revision 1)
> +        return
> +    if not fontconfig_info:
> +        return freetype2_info
> +    return namespace(
> +        cflags=tuple(set(fontconfig_info.cflags + freetype2_info.cflags)),
> +        libs=tuple(set(fontconfig_info.libs + freetype2_info.libs)),

Order of libraries is even more important.

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8797843 [details]
Bug 1305145 - Move --enable-tree-freetype to Python configure.

https://reviewboard.mozilla.org/r/83460/#review82126

::: old-configure.in
(Diff revision 1)
> -   AC_SUBST(MOZ_TREE_FREETYPE)
>     MOZ_ENABLE_CAIRO_FT=1
>     FT_FONT_FEATURE="#define CAIRO_HAS_FT_FONT 1"
> -   FT2_CFLAGS="-I$_topsrcdir/modules/freetype2/include"
>     CAIRO_FT_CFLAGS="-I$_topsrcdir/modules/freetype2/include"
> -   CAIRO_FT_OSLIBS=''

There is a reference to this variable in MOZ_CAIRO_OSLIBS, you can remove that too.

::: toolkit/moz.configure:318
(Diff revision 1)
> +option('--enable-tree-freetype', help='Enable Tree Freetype')
> +
> +@depends('--enable-tree-freetype', target)
> +def tree_freetype(value, target):
> +    if value and target.os == 'WINNT':
> +        die("building with in-tree freetype is not supported on MSVC")

I /think/ this actually doesn't even work on other non-Android platforms. You should do a try build, and if it fails, just remove the option().

::: toolkit/moz.configure:337
(Diff revision 1)
> +set_config('FT2_LIBS', delayed_getattr(freetype2_combined_info, 'libs'))
> +set_config('FT2_CFLAGS', ft2_cflags)

The asymmetry between FT2_CFLAGS and FT2_LIBS seems weird. I wonder if there's not a latent bug in there, or just something declared weirdly that would be better fixed.

Comment 15

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8797840 [details]
Bug 1305145 - Move Pango detection to Python configure.

https://reviewboard.mozilla.org/r/83454/#review82118

> This removal likely breaks building gfx/thebes, which uses fontconfig types.

The commit message mentions this, but MOZ_PANGO hasn't been set since bug 947379, so this branch isn't ever taken. Why this doesn't break gfx/thebes, I'm not sure.

Comment 16

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8797843 [details]
Bug 1305145 - Move --enable-tree-freetype to Python configure.

https://reviewboard.mozilla.org/r/83460/#review82126

> The asymmetry between FT2_CFLAGS and FT2_LIBS seems weird. I wonder if there's not a latent bug in there, or just something declared weirdly that would be better fixed.

Looking at this again, FT2_CFLAGS aren't used in the build beyond contributing to CAIRO_FT_CLFAGS, so there's no need to set them here, but to the point of this question, FT2_LIBS are only used if we're not building with tree freetype, so we never have a mismatch in practice, and there's no need to set FT2_LIBS if we're not using freetype from the tree.

Comment 17

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8797843 [details]
Bug 1305145 - Move --enable-tree-freetype to Python configure.

https://reviewboard.mozilla.org/r/83460/#review82126

> I /think/ this actually doesn't even work on other non-Android platforms. You should do a try build, and if it fails, just remove the option().

Indeed, it failed on Mac and Linux, I'll remove the option.

Comment 18

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8797839 [details]
Bug 1305145 - Make libs and flags set by pkg_check_modules available to the caller.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83452/diff/1-2/

Comment 19

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8797840 [details]
Bug 1305145 - Move Pango detection to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83454/diff/1-2/

Comment 20

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8797841 [details]
Bug 1305145 - Move fontconfig detection to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83456/diff/1-2/

Comment 21

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8797842 [details]
Bug 1305145 - Move freetype2 detection to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83458/diff/1-2/

Comment 22

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8797843 [details]
Bug 1305145 - Move --enable-tree-freetype to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83460/diff/1-2/

Comment 23

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8797844 [details]
Bug 1305145 - Move --enable-system-pixman and --enable-system-cairo to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83462/diff/1-2/

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8797840 [details]
Bug 1305145 - Move Pango detection to Python configure.

https://reviewboard.mozilla.org/r/83454/#review82740

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8797842 [details]
Bug 1305145 - Move freetype2 detection to Python configure.

https://reviewboard.mozilla.org/r/83458/#review82742

::: old-configure.in:3877
(Diff revisions 1 - 2)
> -AC_SUBST(FT2_CFLAGS)
> -AC_SUBST(FT2_LIBS)
> +AC_SUBST_LIST(FT2_CFLAGS)
> +AC_SUBST_LIST(FT2_LIBS)

ISTR you move those later, but you might as well use set_config from toolkit/moz.configure and remove the AC_SUBSTs in this patch.

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8797843 [details]
Bug 1305145 - Move --enable-tree-freetype to Python configure.

https://reviewboard.mozilla.org/r/83460/#review82746

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8797844 [details]
Bug 1305145 - Move --enable-system-pixman and --enable-system-cairo to Python configure.

https://reviewboard.mozilla.org/r/83462/#review82748

Let's separate this one in a new bug. I don't want to put the others on hold, while thinking how best to address this, because this is way too much cargo-culting for my taste. I think we can do better than a large number of AC_SUBST and AC_DEFINE equivalents.

Also, I'm not even sure --enable-system-cairo actually works properly with recent versions of system cairo, so maybe we should remove it instead.

Comment 28

[Mike Hommey [:glandium]]

Karl, Jeff, thoughts on the last paragraph of comment 27?

Comment 29

[Jeff Muizelaar [:jrmuizel]]

(In reply to Mike Hommey [:glandium] from comment #27)
> Comment on attachment 8797844 [details]
> Bug 1305145 - Move --enable-system-pixman and --enable-system-cairo to
> Python configure.
> 
> https://reviewboard.mozilla.org/r/83462/#review82748
> 
> Let's separate this one in a new bug. I don't want to put the others on
> hold, while thinking how best to address this, because this is way too much
> cargo-culting for my taste. I think we can do better than a large number of
> AC_SUBST and AC_DEFINE equivalents.
> 
> Also, I'm not even sure --enable-system-cairo actually works properly with
> recent versions of system cairo, so maybe we should remove it instead.

I don't feel strongly. I'm not sure how widely used it is still.

Comment 30

[Karl Tomlinson (:karlt)]

(In reply to Mike Hommey [:glandium] from comment #27)
> Also, I'm not even sure --enable-system-cairo actually works properly with
> recent versions of system cairo, so maybe we should remove it instead.

Gentoo offers a system-cairo option.  Some people have used it, and there have been bugs resulting.  However, many of those bugs are likely resolved by the move to skia for content.  system-cairo may make more sense now than it did before.
I would keep it if not too much effort.  If it is getting in the way, then I don't have a strong objection to removing it.

Comment 31

[Mike Hommey [:glandium]]

(In reply to Karl Tomlinson (back Oct 10 :karlt) from comment #30)
> (In reply to Mike Hommey [:glandium] from comment #27)
> > Also, I'm not even sure --enable-system-cairo actually works properly with
> > recent versions of system cairo, so maybe we should remove it instead.
> 
> Gentoo offers a system-cairo option.  Some people have used it, and there
> have been bugs resulting.  However, many of those bugs are likely resolved
> by the move to skia for content.  system-cairo may make more sense now than
> it did before.

If content uses skia, what would be left in gecko using cairo?

Comment 32

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

Comment on attachment 8797839 [details]
Bug 1305145 - Make libs and flags set by pkg_check_modules available to the caller.

https://reviewboard.mozilla.org/r/83452/#review83092

::: js/ffi.configure:23
(Diff revision 2)
>  use_system_ffi = depends_if('--with-system-ffi')(lambda _: True)
>  
>  system_ffi = pkg_check_modules('MOZ_FFI', 'libffi > 3.0.9',
>                                 when=use_system_ffi)
>  
> -building_ffi = depends(system_ffi)(lambda v: not bool(v))
> +building_ffi = depends(system_ffi)(lambda v: not bool(v is not None))

Can this be simplified to `lambda v: v is None`?

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33e563d2d834
Make libs and flags set by pkg_check_modules available to the caller. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/85082a111806
Move Pango detection to Python configure. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/aece1b8a8673
Move fontconfig detection to Python configure. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab38e43fdcb6
Move freetype2 detection to Python configure. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f39b7305d41
Move --enable-tree-freetype to Python configure. r=glandium

Comment 34

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

I had to back these out because it appears to have caused Windows builds to fail like https://treeherder.mozilla.org/logviewer.html#?job_id=37679014&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/acee3d0166dc

Comment 35

[Chris Manchester (limited bugmail, email directly)]

The issue is https://bugzilla.mozilla.org/show_bug.cgi?id=1298740#c7. These patches depend on that, though.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a103e419f1ee
Make libs and flags set by pkg_check_modules available to the caller. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/32fe0c9940c3
Move Pango detection to Python configure. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f67a3849c45
Move fontconfig detection to Python configure. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/794ce41590cd
Move freetype2 detection to Python configure. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/074a25a74513
Move --enable-tree-freetype to Python configure. r=glandium

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a103e419f1ee
https://hg.mozilla.org/mozilla-central/rev/32fe0c9940c3
https://hg.mozilla.org/mozilla-central/rev/4f67a3849c45
https://hg.mozilla.org/mozilla-central/rev/794ce41590cd
https://hg.mozilla.org/mozilla-central/rev/074a25a74513

Comment 38

[Karl Tomlinson (:karlt)]

(In reply to Mike Hommey [:glandium] from comment #31)
> If content uses skia, what would be left in gecko using cairo?

I think text shaping (font glyph positioning) is still using cairo fonts, pending a replacement font object type.

I don't know whether printing using skia or cairo.

Comment 39

[Jeff Muizelaar [:jrmuizel]]

cairo is still used for printing on all platforms as well as font metrics.