Closed
Bug 1305145
Opened 7 years ago
Closed 7 years ago
Move --enable-tree-freetype to Python configure
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox52 fixed)
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: chmanchester, Assigned: chmanchester)
References
(Blocks 1 open bug)
Details
Attachments
(6 files, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Details |
No description provided.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → cmanchester
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
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 8•7 years ago
|
||
mozreview-review |
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 9•7 years ago
|
||
mozreview-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 10•7 years ago
|
||
mozreview-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 11•7 years ago
|
||
mozreview-review |
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 12•7 years ago
|
||
mozreview-review |
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 13•7 years ago
|
||
mozreview-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 14•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 16•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8797838 -
Attachment is obsolete: true
Comment 24•7 years ago
|
||
mozreview-review |
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 25•7 years ago
|
||
mozreview-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 26•7 years ago
|
||
mozreview-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 27•7 years ago
|
||
mozreview-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)
Comment 28•7 years ago
|
||
Karl, Jeff, thoughts on the last paragraph of comment 27?
Flags: needinfo?(karlt)
Flags: needinfo?(jmuizelaar)
Assignee | ||
Updated•7 years ago
|
Summary: Move --enable-tree-freetype, --enable-system-pixman and --enable-system-cairo to Python configure → Move --enable-tree-freetype to Python configure
Comment 29•7 years ago
|
||
(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)
Comment 30•7 years ago
|
||
(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)
Comment 31•7 years ago
|
||
(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•7 years ago
|
||
mozreview-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/#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•7 years ago
|
||
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
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
Flags: needinfo?(cmanchester)
Assignee | ||
Comment 35•7 years ago
|
||
The issue is https://bugzilla.mozilla.org/show_bug.cgi?id=1298740#c7. These patches depend on that, though.
Flags: needinfo?(cmanchester)
Comment 36•7 years ago
|
||
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
Comment 37•7 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 38•7 years ago
|
||
(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•7 years ago
|
||
cairo is still used for printing on all platforms as well as font metrics.
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•