Use moz.build to build freetype2 library

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: brsun, Assigned: brsun)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

2 years ago
Integrate freetype2 with moz.build
Assignee

Comment 4

2 years ago
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)
Attachment #8862787 - Flags: review?(mh+mozilla)
Assignee

Comment 5

2 years ago
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?
Attachment #8862787 - Flags: feedback?(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.
Attachment #8862787 - Flags: feedback?(jfkthame) → feedback+
Assignee: nobody → brsun
(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 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.
Attachment #8862787 - Flags: review?(mh+mozilla) → feedback+
Assignee

Comment 12

2 years ago
Attachment #8862787 - Attachment is obsolete: true
Attachment #8866610 - Flags: review?(mh+mozilla)
Assignee

Comment 13

2 years ago
Attachment #8866611 - Flags: review?(mh+mozilla)
Duplicate of this bug: 1262163
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.
Attachment #8866610 - Flags: review?(mh+mozilla) → review+
Attachment #8866611 - Flags: review?(mh+mozilla) → review+
(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).
Assignee

Comment 17

2 years ago
address comment 15, add r= commit message, carry over r+ flag
Attachment #8866610 - Attachment is obsolete: true
Attachment #8870229 - Flags: review+
Assignee

Comment 18

2 years ago
add r= commit message, carry over r+ flag
Attachment #8866611 - Attachment is obsolete: true
Attachment #8870230 - Flags: review+
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 19

2 years ago
Pushed by ihsiao@mozilla.com:
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
Keywords: checkin-needed

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5ef43ba36c8c
https://hg.mozilla.org/mozilla-central/rev/337fc12b15d0
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee

Updated

2 years ago
Blocks: 1368948
Blocks: 1369658
Assignee

Updated

2 years ago
Blocks: 1378608
You need to log in before you can comment on or make changes to this bug.