1358023 - Use moz.build to build freetype2 library

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Description

[Bruce Sun [:brsun]]

Integrate freetype2 with moz.build

Comment 1

[Bruce Sun [:brsun]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=218d9590194a

Comment 2

[Bruce Sun [:brsun]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0ac81b59d10

Comment 3

[Bruce Sun [:brsun]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2deb29909817

Comment 4

[Bruce Sun [:brsun]]

Attachment: bug1358023_freetype2_mozbuild.patch

Use moz.build to replace original build scripts of freetype2:
 - Add /modules/freetype2/moz.build for building freetype2
 - Remove freetype2 configurations from /old-configure.in
 - Remove /config/external/freetype2/Makefile.in
 - Define 'FT_CONFIG_OPTION_USE_PNG' in libpng when building with 'MOZ_TREE_FREETYPE' configuration
 - Remove the checking for 'ANDROID' because we always have 'MOZ_TREE_FREETYPE' configuration on Android platform (refer to 'tree_freetype' within /toolkit/moz.configure)

Comment 5

[Bruce Sun [:brsun]]

Comment on attachment 8862787 [details] [diff] [review]
bug1358023_freetype2_mozbuild.patch

Hi Jonathan,

The patch would remove one checking within libpng which is added by you. Does this modification make sense to you?

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8862787 [details] [diff] [review]
bug1358023_freetype2_mozbuild.patch

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

Seems OK to me. My one question re doing this for a third-party lib like freetype is whether it'll add a bit to the effort of taking future freetype updates where for example we might need to manually update the file lists. But if the benefits of moving to moz.build outweigh that, I guess it's fine.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Jonathan Kew (:jfkthame) from comment #6)
> Comment on attachment 8862787 [details] [diff] [review]
> bug1358023_freetype2_mozbuild.patch
> 
> Review of attachment 8862787 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems OK to me. My one question re doing this for a third-party lib like
> freetype is whether it'll add a bit to the effort of taking future freetype
> updates where for example we might need to manually update the file lists.

We do that for most other third party code already. It's usually trivial.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8862787 [details] [diff] [review]
bug1358023_freetype2_mozbuild.patch

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

::: media/libpng/moz.build
@@ +55,5 @@
>          'powerpc/powerpc_init.c'
>      ]
>  
> +if CONFIG['MOZ_TREE_FREETYPE']:
> +    DEFINES['FT_CONFIG_OPTION_USE_PNG'] = True

Please separate out the libpng change.

::: modules/freetype2/moz.build
@@ +52,5 @@
> +# auxiliary modules
> +SOURCES += [
> +    'src/bzip2/ftbzip2.c',      # Support for streams compressed with bzip2 (files with suffix .bz2).
> +    'src/cache/ftcache.c',      # FreeType's cache sub-system.
> +#    'src/gxvalid/gxvalid.c',    # TrueType GX/AAT table validation.

Please remove commented-out sources.

@@ +88,5 @@
> +CFLAGS += CONFIG['MOZ_ZLIB_CFLAGS']
> +USE_LIBS += ['zlib']
> +
> +# bzip library
> +#DEFINES['FT_CONFIG_OPTION_USE_BZIP2'] = True

Please remove

@@ +96,5 @@
> +CFLAGS += CONFIG['MOZ_PNG_CFLAGS']
> +USE_LIBS += ['mozpng']
> +
> +# harfbuzz library
> +#DEFINES['FT_CONFIG_OPTION_USE_HARFBUZZ'] = True

please remove.

Comment 9

[Bruce Sun [:brsun]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c451a715bf7e

Comment 10

[Bruce Sun [:brsun]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1de89f9e27cf4c057593b9b3279b0167111dde5

Comment 11

[Bruce Sun [:brsun]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88a5f2bf5e45

Comment 12

[Bruce Sun [:brsun]]

Attachment: Part 1: Refector the dependency between libpng and freetype2

Comment 13

[Bruce Sun [:brsun]]

Attachment: Part 2: moz.build for freetype2

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8866610 [details] [diff] [review]
Part 1: Refector the dependency between libpng and freetype2

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

::: media/libpng/pnglibconf.h
@@ +95,1 @@
>     and boot animation code (Gonk) */

You can remove this line, we removed gonk from the tree.

Comment 16

[(not currently active) Ted Mielczarek]

(In reply to Jonathan Kew (:jfkthame) from comment #6)
> Seems OK to me. My one question re doing this for a third-party lib like
> freetype is whether it'll add a bit to the effort of taking future freetype
> updates where for example we might need to manually update the file lists.
> But if the benefits of moving to moz.build outweigh that, I guess it's fine.

For ICU I added a bit to the update script to generate new SOURCES lists from the new version:
https://dxr.mozilla.org/mozilla-central/rev/85e5d15c31691c89b82d6068c26260416493071f/intl/update-icu.sh#85
https://dxr.mozilla.org/mozilla-central/source/intl/icu_sources_data.py

For NSPR I just punted and assumed we'd update the moz.build files if the list of sources changed. I believe we did the same thing with libffi (which doesn't get updated very often anyway).

Comment 17

[Bruce Sun [:brsun]]

Attachment: Part 1: Refector the dependency between libpng and freetype2

address comment 15, add r= commit message, carry over r+ flag

Comment 18

[Bruce Sun [:brsun]]

Attachment: Part 2: moz.build for freetype2

add r= commit message, carry over r+ flag

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef43ba36c8c
(1/2) Refactor the dependency between libpng and freetype2; f=jfkthame, r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/337fc12b15d0
(2/2) Build freetype2 by using moz.build; r=glandium

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ef43ba36c8c
https://hg.mozilla.org/mozilla-central/rev/337fc12b15d0