Open Bug 847568 Opened 7 years ago Updated 8 months ago

Allow to build against systemwide harfbuzz and graphite2 libraries

Categories

(Firefox Build System :: General, defect)

x86
OpenBSD
defect
Not set

Tracking

(Not tracked)

People

(Reporter: gaston, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 9 obsolete files)

To be discussed - we have --with-system-{nss,nspr,jpeg,libevent,bz2,png,libvpx}, --enable-system-{hunspell,ffi,sqlite,cairo,pixman} what would prevent us from having --with-system-harfbuzz ?

- we need to depend on the version we have in-tree (0.9.12 atm), easily doable via pkg-config
- no api/header change in the in-tree version ?

Would there be opposition to that or i can wrap up a patch ?
Currently, harfbuzz is still evolving at a fairly rapid rate, such that we sometimes take local patches for issues, or cherry-pick commits from upstream that are newer than the current tagged release. (As of now, the most recent update was in bug 841313, which was not an actual "release" but just a specific state of upstream trunk, plus an extra fix that we cherry-picked in bug 827093.) It's still plausible, I think, that we could have new API developed in harfbuzz trunk and getting used within our tree prior to there being a stable, packaged release with a version bump for pkg-config to handle.

In this situation, I'd prefer to stay with the in-tree harfbuzz for the time being, so that we can be confident of having the same set of features across all platforms. In addition, I suspect that few distros are like to be updating harfbuzz on a more aggressive schedule than our in-tree copy, which reduces the potential benefit of --with-system-harfbuzz.

I do think harfbuzz is reaching a level of maturity where we'll be able to rely on header/api stability, and it will make sense to offer a --with-system-harfbuzz option; it just feels a little premature as yet, given that we're a particularly "bleeding-edge" consumer of the still-evolving harfbuzz.
Sure - the bug will stay as a placeholder until harfbuzz reaches a stable / mature level, whenever that is. At that point, i'll take care of the build goo allowing --with-system-harfbuzz..
Compared with sqlite3 and nss releases I don't expect rapid development of harfbuzz would be much an issue provided we bump version requirment on updates. At the time internal harfbuzz reaches mozilla-release the code would be a few months old which gives plenty of time to update ports/packages downstream.

For example, Firefox 28 would include harfbuzz 205bf834 with version between 0.9.24 and 0.9.25. Fedora, Debian and PkgSrc are already at 0.9.26 which would make system harfbuzz slightly newer than in Firefox 30.

(In reply to Jonathan Kew (:jfkthame) from comment #1)
> In addition, I suspect that few distros are like to be updating
> harfbuzz on a more aggressive schedule than our in-tree copy, which
> reduces the potential benefit of --with-system-harfbuzz.

What is the benefit of older harfbuzz in Mozilla tree? Downstream may already have some experience with API breakages given that Pango, libass and sometimes Chromium depend on harfbuzz as well.
Attached patch v0 (obsolete) — Splinter Review
Comment on attachment 8378151 [details] [diff] [review]
v0

It seems esr24 shipped with harfbuzz-0.9.16/graphite2-1.2.3 works fine with system harfbuzz-0.9.25/graphite2-1.2.4. Let's start review then.
Attachment #8378151 - Flags: review?(mh+mozilla)
Attachment #8378151 - Flags: feedback?(jfkthame)
Comment on attachment 8378151 [details] [diff] [review]
v0

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

Should we have a separate bug for "build against system graphite2" rather than piggy-backing onto the harfbuzz one here?

::: configure.in
@@ +7839,5 @@
> +MOZ_NATIVE_HARFBUZZ= )
> +
> +if test -n "$MOZ_NATIVE_HARFBUZZ"; then
> +    PKG_CHECK_MODULES(MOZ_HARFBUZZ, harfbuzz >= 0.9.26)
> +    MOZ_NATIVE_GRAPHITE2=1

Do we really want this interdependency? I know harfbuzz -can- link to graphite, but AFAIK it's not a hard dependency, and we don't actually use that option at all.
Attached patch v0.1 (obsolete) — Splinter Review
This version removes MOZ_NATIVE_GRAPHITE2=1 for harfbuzz in configure.in and MOZ_GRAPHITE2_CFLAGS from gfx/harfbuzz/src/Makefile.in.

Need more testing. My CFLAGS are

  $ pkg-config harfbuzz --cflags
  -I/usr/local/include/harfbuzz

  $ pkg-config graphite2 --cflags
  -I/usr/local/include

  $ pkg-config glib-2.0 --cflags
  -I/usr/local/include/glib-2.0 -I/usr/local/include

(In reply to Jonathan Kew (:jfkthame) from comment #6)
> Comment on attachment 8378151 [details] [diff] [review]
> v0
> 
> Review of attachment 8378151 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Should we have a separate bug for "build against system graphite2" rather
> than piggy-backing onto the harfbuzz one here?

They're closely related font shaping libraries. Quite a few files use headers from both. Given that harfbuzz may depend on graphite2 the reviewer may want to check when only one of them is system building code against in-tree copy doesn't accidentally include its system headers.

> ::: configure.in
> @@ +7839,5 @@
> > +MOZ_NATIVE_HARFBUZZ= )
> > +
> > +if test -n "$MOZ_NATIVE_HARFBUZZ"; then
> > +    PKG_CHECK_MODULES(MOZ_HARFBUZZ, harfbuzz >= 0.9.26)
> > +    MOZ_NATIVE_GRAPHITE2=1
> 
> Do we really want this interdependency? I know harfbuzz -can- link to
> graphite, but AFAIK it's not a hard dependency, and we don't actually use
> that option at all.

graphite2 is a hard dependency for harfbuzz *package* at least on Arch, Fedora, Debian, FreeBSD, OpenBSD and probably more. harfbuzz-icu, on the other hand, is often provided as a subpackage.
Attachment #8378151 - Attachment is obsolete: true
Attachment #8378151 - Flags: review?(mh+mozilla)
Attachment #8378151 - Flags: feedback?(jfkthame)
Attachment #8379678 - Flags: review?(mh+mozilla)
Attachment #8379678 - Flags: feedback?(jfkthame)
(In reply to Jan Beich from comment #7)
> closely related

Er, should have used "similar" instead but as I'm not familiar with their API both word choices are likely wrong.
Comment on attachment 8379678 [details] [diff] [review]
v0.1

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

::: configure.in
@@ +7856,5 @@
> +MOZ_NATIVE_HARFBUZZ=1,
> +MOZ_NATIVE_HARFBUZZ= )
> +
> +if test -n "$MOZ_NATIVE_HARFBUZZ"; then
> +    PKG_CHECK_MODULES(MOZ_HARFBUZZ, harfbuzz >= 0.9.26)

Please update the update instructions for harfbuzz and graphite2 so that those versions are updated as well. Re-r? me then.
Attachment #8379678 - Flags: review?(mh+mozilla) → feedback+
Attached patch v0.2 (obsolete) — Splinter Review
harfbuzz has no update.sh, only verbose README-mozilla
graphite2 has verbose update.sh and short README.mozilla referring to it
Attachment #8379678 - Attachment is obsolete: true
Attachment #8379678 - Flags: feedback?(jfkthame)
Attachment #8382370 - Flags: review?(mh+mozilla)
Attachment #8382370 - Flags: feedback?(jfkthame)
Attached patch v0.2 (obsolete) — Splinter Review
rebased after bug 978594
Attachment #8382370 - Attachment is obsolete: true
Attachment #8382370 - Flags: review?(mh+mozilla)
Attachment #8382370 - Flags: feedback?(jfkthame)
Attachment #8393991 - Flags: review?(mh+mozilla)
Attachment #8393991 - Flags: feedback?(jfkthame)
Oops, forgot to salvage README-mozilla and moz-gr-update.sh hunks from attachment 8382370 [details] [diff] [review].
Attached patch v0.2 (obsolete) — Splinter Review
Attachment #8393991 - Attachment is obsolete: true
Attachment #8393991 - Flags: review?(mh+mozilla)
Attachment #8393991 - Flags: feedback?(jfkthame)
Attachment #8394001 - Flags: review?(mh+mozilla)
Attachment #8394001 - Flags: feedback?(jfkthame)
Comment on attachment 8394001 [details] [diff] [review]
v0.2

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

Looks OK to me, not that I know much about build-system stuff. :)  See a couple of suggestions below.

::: gfx/graphite2/moz-gr-update.sh
@@ +38,3 @@
>  # summarize what's been touched
>  echo Updated to $CHANGESET.
>  echo Here is what changed in the gfx/graphite2 directory:

Now that this script is also touching our top-level configure.in, I think that should be included in the summary it reports.

::: intl/unicharutil/util/moz.build
@@ +35,5 @@
> +if CONFIG['MOZ_NATIVE_HARFBUZZ']:
> +    for f in ['nsUnicodeProperties.cpp']:
> +        UNIFIED_SOURCES.remove('%s/intl/unicharutil/util/%s' % (TOPSRCDIR, f))
> +        SOURCES += ['%s/intl/unicharutil/util/%s' % (TOPSRCDIR, f)]
> +        SOURCES['%s/intl/unicharutil/util/%s' % (TOPSRCDIR, f)].flags += [CONFIG['MOZ_HARFBUZZ_CFLAGS']]

Is it really worth doing this, or should we simply leave nsUnicodeProperties.cpp in SOURCES for all builds?

Or can we add MOZ_HARFBUZZ_CFLAGS to the unified compilation? It ought to be harmless, presumably.

::: netwerk/dns/moz.build
@@ +66,5 @@
> +
> +if CONFIG['MOZ_NATIVE_HARFBUZZ']:
> +    UNIFIED_SOURCES.remove('nsIDNService.cpp')
> +    SOURCES += ['nsIDNService.cpp']
> +    SOURCES['nsIDNService.cpp'].flags += [CONFIG['MOZ_HARFBUZZ_CFLAGS']]

Same comment as for nsUnicodeProperties.cpp; can/should we avoid the munging of the SOURCES and UNIFIED_SOURCES lists here?
Attachment #8394001 - Flags: feedback?(jfkthame) → feedback+
Comment on attachment 8394001 [details] [diff] [review]
v0.2

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

::: configure.in
@@ +7944,5 @@
> +MOZ_NATIVE_GRAPHITE2=1,
> +MOZ_NATIVE_GRAPHITE2= )
> +
> +if test -n "$MOZ_NATIVE_GRAPHITE2"; then
> +    PKG_CHECK_MODULES(MOZ_GRAPHITE2, graphite2 >= 1.2.4)

Doh, graphite2-1.2.4.tgz installs .pc file with version 3.0.1.
Attached patch v1 (obsolete) — Splinter Review
Attachment #8394001 - Attachment is obsolete: true
Attachment #8394001 - Flags: review?(mh+mozilla)
Attachment #8404466 - Flags: review?(mh+mozilla)
Landry, can you push to try this one along with bug 983953 and bug 517422 ? Alas, they can only be applied in that sequence due to conflicting changes in config/system-headers and config/Makefile.in.
Flags: needinfo?(landry)
Attached patch v1.1 (obsolete) — Splinter Review
HB_VERSION_CHECK logic is confusing. I shouldn't have copied it as is for graphite2.
Attachment #8404466 - Attachment is obsolete: true
Attachment #8404466 - Flags: review?(mh+mozilla)
Attachment #8405065 - Flags: review?(mh+mozilla)
(In reply to Jan Beich from comment #17)
> Landry, can you push to try this one along with bug 983953 and bug 517422 ?
> Alas, they can only be applied in that sequence due to conflicting changes
> in config/system-headers and config/Makefile.in.

Fwiw, bug 517422 doesnt apply perfectly - needs to be updated wrt header changes in those files..

Hunk #1 FAILED at 7
1 out of 1 hunks FAILED -- saving rejects to file content/media/AudioStream.cpp.rej
patching file content/media/AudioStream.h
Hunk #1 FAILED at 9
1 out of 1 hunks FAILED -- saving rejects to file content/media/AudioStream.h.rej

https://tbpl.mozilla.org/?tree=Try&rev=90af6a4757aa
Flags: needinfo?(landry)
Try is green except for 2 possibly transient/infra errors.
mike, jonathan, any news on this ?
Duplicate of this bug: 1044099
Attached patch v1.1 (obsolete) — Splinter Review
rebased, https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5a9b1f3f18c6
Attachment #8405065 - Attachment is obsolete: true
Attachment #8405065 - Flags: review?(mh+mozilla)
Attachment #8466140 - Flags: review?(mh+mozilla)
Attached patch v1.1 (graphite2/harfbuzz) (obsolete) — Splinter Review
rebased after bug 1045783, https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c2ec4ee42985
Attachment #8466140 - Attachment is obsolete: true
Attachment #8466140 - Flags: review?(mh+mozilla)
Attachment #8468590 - Flags: review?(mh+mozilla)
Harfbuzz reached 1.0 milestone.
Is it possible to rebase the patch on current trunk but require latest harfbuzz release?
Comment on attachment 8468590 [details] [diff] [review]
v1.1 (graphite2/harfbuzz)

Rebased and moved to mozreview, see attachment 8721665 [details].

(In reply to Hussam Al-Tayeb from comment #26)
> Is it possible to rebase the patch on current trunk but require latest
> harfbuzz release?

Apologies, downstream the patch lives as
  https://svnweb.freebsd.org/ports/head/www/firefox/files/patch-bug847568
  https://svnweb.freebsd.org/ports/head/www/firefox-esr/files/patch-bug847568
except it may slack on bumping version requirements. The patch allows us to skip some releases e.g., 38.6.0 -> 38.6.1.
Attachment #8468590 - Attachment is obsolete: true
Attachment #8468590 - Flags: review?(mh+mozilla)
Comment on attachment 8721665 [details]
Bug 847568 - Allow building against system-wide harfbuzz.

https://reviewboard.mozilla.org/r/35763/#review33325

With the ongoing autoconf transition, I'd rather wait a little before adding more --with-system options, as I'm considering moving  them early on, because they seem like an easy target. This has been lingering long enough that this can wait a bit more to be reviewed. (and it will need an update after --with-system options have moved)
Attachment #8721665 - Flags: review?(mh+mozilla)
Depends on: 1251497
One of the reasons I wanted this patch is that it fixes an issue where I get 'hidden symbols' linker warnings since gtk3/freetype/etc... stack uses harfbuzz but firefox is building against the intree version.
Is it possible to merge this now so it gets quickly converted to the python scripts?
The patch should still apply right now if it is edited to replace configure.in to old-configure.in.
The warnings are the linker being stupid. They are not saying anything about actual problems. Just ignore them.
(In reply to Mike Hommey [:glandium] from comment #31)
> The warnings are the linker being stupid. They are not saying anything about
> actual problems. Just ignore them.

Maybe stupid in a way that it doesn't know the actual situation; linking against gtk+ which pulls in system harfbuzz so linking against two versions of harfbuzz, hence the hidden dependency.
FYI bug 1244743 changes these from MOZ_NATIVE_X to MOZ_SYSTEM_X.
Summary: Allow to build against systemwide harfbuzz → Allow to build against systemwide harfbuzz and graphite2 libraries
Comment on attachment 8721665 [details]
Bug 847568 - Allow building against system-wide harfbuzz.

https://reviewboard.mozilla.org/r/35763/#review45167

The versions should be updated to match what we're currently using in-tree, as there have been fresh updates since this was posted.

Aside from that, I'm fine with adding this, but it's really up to the build system folk to review and decide when is the right time to do it.

::: old-configure.in:7876
(Diff revision 2)
> +[  --with-system-harfbuzz  Use system harfbuzz (located with pkgconfig)],
> +MOZ_SYSTEM_HARFBUZZ=1,
> +MOZ_SYSTEM_HARFBUZZ=)
> +
> +if test -n "$MOZ_SYSTEM_HARFBUZZ"; then
> +    PKG_CHECK_MODULES(MOZ_HARFBUZZ, harfbuzz >= 1.2.2)

You'll need to bump this to 1.2.6, as we have updated gecko code to take advantage of some newly-added harfbuzz API.

::: old-configure.in:7900
(Diff revision 2)
> +                     #define GR2_VERSION_REQUIRE(major,minor,bugfix)  \
> +                             ( GR2_VERSION_MAJOR * 10000 + GR2_VERSION_MINOR \
> +                               * 100 + GR2_VERSION_BUGFIX >= \
> +                               (major) * 10000 + (minor) * 100 + (bugfix) )
> +                   ], [
> +                     #if !GR2_VERSION_REQUIRE(1,3,6)

Require 1.3.8, please.
Attachment #8721665 - Flags: review?(jfkthame)
https://reviewboard.mozilla.org/r/35763/#review45167

> ... up to the build system folk to review

If you mean comment 3 then I'm unlikely to contribute there. Bug 1251497 doesn't impact FreeBSD unlike bug 1259537.
(In reply to Jan Beich from comment #37)
> comment 3

Typo: comment 29.
Comment on attachment 8721665 [details]
Bug 847568 - Allow building against system-wide harfbuzz.

https://reviewboard.mozilla.org/r/35763/#review45509

r=me, from the "what font-shaping libraries do we use" PoV, but this should still wait for r+ from a build system peer in order to land.
Attachment #8721665 - Flags: review?(jfkthame) → review+
This requires a new patch for trunk. It doesn't apply anymore.
Re-diffed against trunk http://pastebin.com/raw/zfQXdBir
Converting to moz.configure proved to be trivial but getting try_compile() in the scope is not so. Moving --with-system-graphite2 check into build/moz.configure/toolchain.configure seems like a wrong idea.

Traceback (most recent call last):
  File "configure.py", line 94, in <module>
    sys.exit(main(sys.argv))
  File "configure.py", line 22, in main
    sandbox.run(os.path.join(os.path.dirname(__file__), 'moz.configure'))
  File "python/mozbuild/mozbuild/configure/__init__.py", line 231, in run
    self.include_file(path)
  File "python/mozbuild/mozbuild/configure/__init__.py", line 222, in include_file
    exec_(code, self)
  File "python/mozbuild/mozbuild/util.py", line 58, in exec_
    exec(object, globals, locals)
  File "moz.configure", line 234, in <module>
    include('build/moz.configure/compilechecks.configure')
  File "python/mozbuild/mozbuild/configure/__init__.py", line 477, in include_impl
    self.include_file(what)
  File "python/mozbuild/mozbuild/configure/__init__.py", line 214, in include_file
    'Cannot include `%s` because it was included already.' % path)
mozbuild.configure.ConfigureError: Cannot include `build/moz.configure/compilechecks.configure` because it was included already.
Comment on attachment 8721665 [details]
Bug 847568 - Allow building against system-wide harfbuzz.

Changes since previous version
- Rebase similar to comment 41
- Save CFLAGS before checking graphite2 version
- Partially convert to moz.configure
See comment 42 for why this isn't part of review yet.
I've split harfbuzz and graphite2 changes into separate commits:
- harfbuzz is ready for review (and rebased after bug 1313097)
- graphite2 has yet to be converted to moz.configure completely
Attachment #8806980 - Flags: review?(mh+mozilla)
Attachment #8806980 - Flags: review?(jfkthame)
Comment on attachment 8806980 [details]
Bug 847568 - (WIP) Allow building against system-wide graphite2.

https://reviewboard.mozilla.org/r/90218/#review90022

I'm fine with doing this, but the review you really want is glandium's -- build system stuff is largely a mystery to me. :)

::: moz.configure:276
(Diff revision 1)
> +@depends('--with-system-graphite2', compile_environment)
> +def check_for_graphite2(value, compile_env):
> +    return value and compile_env
> +
> +system_graphite2 = pkg_check_modules('MOZ_GRAPHITE2', 'graphite2',
> +                                    check_for_graphite2)

nit: looks like the indent here wants one more space
Attachment #8806980 - Flags: review+
FYI, the commit applied to fix bug 1306543 requires that hb-glib.h be added to the config/system-headers portion of the harfbuzz patch.
(In reply to Ian Stakenvicius from comment #51)
> bug 1306543 requires that hb-glib.h

Addressed by comment 46.
See Also: → 517422
Comment on attachment 8721665 [details]
Bug 847568 - Allow building against system-wide harfbuzz.

https://reviewboard.mozilla.org/r/35763/#review91474

::: moz.configure:268
(Diff revision 6)
> +option('--with-system-harfbuzz',
> +       help="Use system harfbuzz (located with pkgconfig)")
> +
> +@depends('--with-system-harfbuzz', compile_environment)
> +def check_for_harfbuzz(value, compile_env):
> +    return value and compile_env
> +
> +system_harfbuzz = pkg_check_modules('MOZ_HARFBUZZ', 'harfbuzz >= 1.3.3',
> +                                    check_for_harfbuzz)
> +
> +set_config('MOZ_SYSTEM_HARFBUZZ', depends_if(system_harfbuzz)(lambda _: True))

This should be in toolkit/moz.configure.

::: moz.configure:271
(Diff revision 6)
>             allow_missing=True)
>  
> +option('--with-system-harfbuzz',
> +       help="Use system harfbuzz (located with pkgconfig)")
> +
> +@depends('--with-system-harfbuzz', compile_environment)

As of bug 1307355, pkg_check_modules has an implicit dependency on compile_environment, so you don't need to add one manually.

::: moz.configure:276
(Diff revision 6)
> +@depends('--with-system-harfbuzz', compile_environment)
> +def check_for_harfbuzz(value, compile_env):
> +    return value and compile_env
> +
> +system_harfbuzz = pkg_check_modules('MOZ_HARFBUZZ', 'harfbuzz >= 1.3.3',
> +                                    check_for_harfbuzz)

Please qualify this argument (when=...)
Attachment #8721665 - Flags: review?(mh+mozilla)
Comment on attachment 8721665 [details]
Bug 847568 - Allow building against system-wide harfbuzz.

https://reviewboard.mozilla.org/r/35763/#review91830

::: toolkit/moz.configure:351
(Diff revision 7)
> +@depends('--with-system-harfbuzz')
> +def check_for_harfbuzz(value):
> +    return bool(value)
> +
> +system_harfbuzz = pkg_check_modules('MOZ_HARFBUZZ', 'harfbuzz >= 1.3.3',
> +                                    when=check_for_harfbuzz)

when='--with-system-harfbuzz' should work.
Attachment #8721665 - Flags: review?(mh+mozilla) → review+
The part in intl/unicharutil/util/moz.build no longer applies.
Jan are you still interested in working on this?
Flags: needinfo?(jbeich)
No, for now. Fixing regressions on my platform take priority. If you just need a rebased patch see comment 28.
Flags: needinfo?(jbeich)
Product: Core → Firefox Build System
Just a note that the current version of Jan's patch still builds and works fine on 61.0b5 (and 60.0), although the version tests should of course be changed to 1.3.11 and 1.7.6 (probably
simple enough to just manually edit it before applying) :

--- patch-bug847568	2018-05-15 17:30:13.232036372 +0100
+++ patch-bug847568.updated	2018-05-15 20:14:09.215727765 +0100
@@ -198,7 +198,7 @@
 +                               * 100 + GR2_VERSION_BUGFIX >= \
 +                               (major) * 10000 + (minor) * 100 + (bugfix) )
 +                   ], [
-+                     #if !GR2_VERSION_REQUIRE(1,3,10)
++                     #if !GR2_VERSION_REQUIRE(1,3,11)
 +                     #error "Insufficient graphite2 version."
 +                     #endif
 +                   ], [],
@@ -249,7 +249,7 @@
 +option('--with-system-harfbuzz',
 +       help="Use system harfbuzz (located with pkgconfig)")
 +
-+system_harfbuzz = pkg_check_modules('MOZ_HARFBUZZ', 'harfbuzz >= 1.7.4',
++system_harfbuzz = pkg_check_modules('MOZ_HARFBUZZ', 'harfbuzz >= 1.7.6',
 +                                    when='--with-system-harfbuzz')
 +
 +set_config('MOZ_SYSTEM_HARFBUZZ', depends_if(system_harfbuzz)(lambda _: True))

It would be nice to see this added to mozilla instead of having to patch it.
You need to log in before you can comment on or make changes to this bug.