Closed Bug 1305145 Opened 7 years ago Closed 7 years ago

Move --enable-tree-freetype to Python configure

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 1 obsolete file)

Bug 1305145 - Make libs and flags set by pkg_check_modules available to the caller.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1305145 - Move Pango detection to Python configure.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1305145 - Move fontconfig detection to Python configure.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1305145 - Move freetype2 detection to Python configure.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1305145 - Move --enable-tree-freetype to Python configure.
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1305145 - Move --enable-system-pixman and --enable-system-cairo to Python configure.
58 bytes, text/x-review-board-request
Details 
      No description provided.
Assignee: nobody → cmanchester
Summary: Move --enable-system-pixman and --enable-system-cairo to Python configure → Move --enable-tree-freetype, --enable-system-pixman and --enable-system-cairo to Python configure
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.
Attachment #8797838 - Flags: review?(mh+mozilla) → review+
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
Attachment #8797839 - Flags: review?(mh+mozilla) → review+
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 on attachment 8797840 [details]
Bug 1305145 - Move Pango detection to Python configure.

https://reviewboard.mozilla.org/r/83454/#review82120
Attachment #8797840 - Flags: review?(mh+mozilla)
Comment on attachment 8797841 [details]
Bug 1305145 - Move fontconfig detection to Python configure.

https://reviewboard.mozilla.org/r/83456/#review82122
Attachment #8797841 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8797842 - Flags: review?(mh+mozilla)
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.
Attachment #8797843 - Flags: review?(mh+mozilla)
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 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 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.
Attachment #8797838 - Attachment is obsolete: true
Comment on attachment 8797840 [details]
Bug 1305145 - Move Pango detection to Python configure.

https://reviewboard.mozilla.org/r/83454/#review82740
Attachment #8797840 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8797842 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8797843 [details]
Bug 1305145 - Move --enable-tree-freetype to Python configure.

https://reviewboard.mozilla.org/r/83460/#review82746
Attachment #8797843 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8797844 - Flags: review?(mh+mozilla)
Karl, Jeff, thoughts on the last paragraph of comment 27?
Flags: needinfo?(karlt)
Flags: needinfo?(jmuizelaar)
Summary: Move --enable-tree-freetype, --enable-system-pixman and --enable-system-cairo to Python configure → Move --enable-tree-freetype to Python configure
(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.
Flags: needinfo?(jmuizelaar)
(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.
Flags: needinfo?(karlt)
(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 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`?
Pushed by cmanchester@mozilla.com:
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
The issue is https://bugzilla.mozilla.org/show_bug.cgi?id=1298740#c7. These patches depend on that, though.
Flags: needinfo?(cmanchester)
Pushed by cmanchester@mozilla.com:
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
(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.
cairo is still used for printing on all platforms as well as font metrics.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.