847568 - Allow to build against systemwide harfbuzz and graphite2 libraries

Status: NEW

(Firefox Build System :: General, enhancement)

Description

[Landry Breuil (:gaston)]

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 ?

Comment 1

[Jonathan Kew [:jfkthame]]

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.

Comment 2

[Landry Breuil (:gaston)]

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

Comment 3

[Jan Beich]

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.

Comment 4

[bugmenot]

Attachment: v0

Comment 5

[Jan Beich]

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.

Comment 6

[Jonathan Kew [: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.

Comment 7

[Jan Beich]

Attachment: v0.1

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.

Comment 8

[Jan Beich]

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

[Mike Hommey [:glandium]]

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.

Comment 10

[Jan Beich]

Attachment: v0.2

harfbuzz has no update.sh, only verbose README-mozilla
graphite2 has verbose update.sh and short README.mozilla referring to it

Comment 11

[Jan Beich]

Attachment: v0.2

rebased after bug 978594

Comment 12

[Jan Beich]

Oops, forgot to salvage README-mozilla and moz-gr-update.sh hunks from attachment 8382370 [details] [diff] [review].

Comment 13

[Jan Beich]

Attachment: v0.2

Comment 14

[Jonathan Kew [: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?

Comment 15

[Jan Beich]

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.

Comment 16

[Jan Beich]

Attachment: v1

Comment 17

[Jan Beich]

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.

Comment 18

[Jan Beich]

Attachment: v1.1

HB_VERSION_CHECK logic is confusing. I shouldn't have copied it as is for graphite2.

Comment 19

[Landry Breuil (:gaston)]

(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

Comment 20

[Jan Beich]

Try is green except for 2 possibly transient/infra errors.

Comment 21

[Landry Breuil (:gaston)]

mike, jonathan, any news on this ?

Comment 23

[Jan Beich]

Attachment: v1.1

rebased, https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5a9b1f3f18c6

Comment 24

[Jan Beich]

Attachment: v1.1 (graphite2/harfbuzz)

rebased after bug 1045783, https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c2ec4ee42985

Comment 25

[Hussam Al-Tayeb]

Harfbuzz reached 1.0 milestone.

Comment 26

[Hussam Al-Tayeb]

Is it possible to rebase the patch on current trunk but require latest harfbuzz release?

Comment 27

[Jan Beich]

Attachment: Bug 847568 - Allow building against system-wide harfbuzz.

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

Comment 28

[Jan Beich]

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.

Comment 29

[Mike Hommey [:glandium]]

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)

Comment 30

[Hussam Al-Tayeb]

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.

Comment 31

[Mike Hommey [:glandium]]

The warnings are the linker being stupid. They are not saying anything about actual problems. Just ignore them.

Comment 32

[Hussam Al-Tayeb]

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

Comment 33

[(not currently active) Ted Mielczarek]

FYI bug 1244743 changes these from MOZ_NATIVE_X to MOZ_SYSTEM_X.

Comment 34

[Jan Beich]

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

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

Comment 35

[Jonathan Kew [:jfkthame]]

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.

Comment 36

[Jan Beich]

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

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

Comment 37

[Jan Beich]

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.

Comment 38

[Jan Beich]

(In reply to Jan Beich from comment #37)
> comment 3

Typo: comment 29.

Comment 39

[Jonathan Kew [:jfkthame]]

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.

Comment 40

[Hussam Al-Tayeb]

This requires a new patch for trunk. It doesn't apply anymore.

Comment 41

[Hussam Al-Tayeb]

Re-diffed against trunk http://pastebin.com/raw/zfQXdBir

Comment 42

[Jan Beich]

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 43

[Jan Beich]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35763/diff/3-4/

Comment 44

[Jan Beich]

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

Comment 45

[Jan Beich]

Attachment: move AC_TRY_COMPILE check to moz.configure (WIP)

See comment 42 for why this isn't part of review yet.

Comment 46

[Jan Beich]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35763/diff/4-5/

Comment 47

[Jan Beich]

Attachment: Bug 847568 - (WIP) Allow building against system-wide graphite2.

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

Comment 48

[Jan Beich]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35763/diff/5-6/

Comment 49

[Jan Beich]

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

Comment 50

[Jonathan Kew [: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

Comment 51

[Ian Stakenvicius]

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.

Comment 52

[Jan Beich]

(In reply to Ian Stakenvicius from comment #51)
> bug 1306543 requires that hb-glib.h

Addressed by comment 46.

Comment 53

[Mike Hommey [:glandium]]

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

Comment 54

[Jan Beich]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35763/diff/6-7/

Comment 55

[Jan Beich]

Comment on attachment 8806980 [details]
Bug 847568 - (WIP) Allow building against system-wide graphite2.

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

Comment 56

[Mike Hommey [:glandium]]

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.

Comment 57

[Hussam Al-Tayeb]

The part in intl/unicharutil/util/moz.build no longer applies.

Comment 58

[Jan Beich]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35763/diff/7-8/

Comment 59

[Jan Beich]

Comment on attachment 8806980 [details]
Bug 847568 - (WIP) Allow building against system-wide graphite2.

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

Comment 60

[Jan Beich]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35763/diff/8-9/

Comment 61

[Jan Beich]

Comment on attachment 8806980 [details]
Bug 847568 - (WIP) Allow building against system-wide graphite2.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90218/diff/3-4/

Comment 62

[Jory A. Pratt [:anarchy]]

Jan are you still interested in working on this?

Comment 63

[Jan Beich]

No, for now. Fixing regressions on my platform take priority. If you just need a rebased patch see comment 28.

Comment 64

[Ken Moffat]

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.

Comment 65

[David Heidelberg [:okias]]

since 7 years passed, is going to Mozilla include this patch or not? Would be nice to know.

Comment 66

[Ken Moffat]

The possibility of using system graphite2 appears to have gone with ff72 (new code and header in graphite2/geckoheader which only gets compiled if the shipped graphite2 is compiled, but is referenced from gfx/thebes and therefore breaks the build).

Comment 67

[Jonathan Kew [:jfkthame]]

Hmm, that's a good point. In principle I think it should be possible to use that code in conjunction with a system graphite lib, but it'd probably take some build-system hacking somewhere to rearrange things.

The graphite2/geckoextra code is currently placed (and compiled) together with in-tree graphite because when the whole of graphite will be compiled as a WASM sandboxed library, this code is to be included as part of the lib. I suspect that would be harder to arrange if we moved the graphite2/geckoextra code over to gfx/thebes.

Comment 68

[s.zharkoff]

Looks like the only issue is the addition of 2 functions in the graphite2 library. I've moved 1 cpp and 1 header file into sytem graphite2 source and nothing so far is broken in the system when system graphite2 compiled with this addition. So technically the best solution would be to contribute those additions to main graphite2 library and then no more issues to compile 72 with system harfbuzz and graphite2 again.

Comment 69

[Thaodan]

Looks like the commit that introduced that this was added here:
https://phabricator.services.mozilla.com/D47388
See: #1584000

Comment 70

[Thaodan]

(In reply to s.zharkoff from comment #68)

Looks like the only issue is the addition of 2 functions in the graphite2 library. I've moved 1 cpp and 1 header file into sytem graphite2 source and nothing so far is broken in the system when system graphite2 compiled with this addition. So technically the best solution would be to contribute those additions to main graphite2 library and then no more issues to compile 72 with system harfbuzz and graphite2 again.

I've created a patch to include changes mentioned to the upstream package see here if anyone wants to look into it:
https://aur.archlinux.org/cgit/aur.git/tree/add_mozilla_geckoextra.patch?h=graphite-mozilla

Comment 71

[Thaodan]

Gentoo's graphite patch no avoids needing to link to patched version of graphite.