1239083 - Stop using ICU's autoconf build system

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

ICU's build system is terrible:
* It's not parallel-safe, so we have to build with -j1.
* For cross-compiles we build a host and a target version to build some tools for managing ICU data files.
* Invoking extra configure files adds to our build time, especially on Windows.

We should replace it with something else. Chromium has GYP files:
https://chromium.googlesource.com/chromium/deps/icu/+/42c58d4e49f2250039f0e98d43e0b76e8f5ca024/icu.gyp

...but they're also transitioning to GN files, so the GYP files are likely to become unmaintained at some point:
https://chromium.googlesource.com/chromium/deps/icu/+/42c58d4e49f2250039f0e98d43e0b76e8f5ca024/BUILD.gn

I looked into the data file issue, and it turns out that if we are willing to commit the (~10MB, gzips to ~3MB) icudt.dat file to our tree we don't need the host build at all:
http://userguide.icu-project.org/howtouseicu#TOC-C-With-Your-Own-Build-System
http://userguide.icu-project.org/icudata

This would also let us fix bug 926980.

Comment 1

[(not currently active) Ted Mielczarek]

glandium rightly suggested that a good first step would be to fix bug 926980.

Comment 2

[(not currently active) Ted Mielczarek]

Daniel Bratell (from Opera) linked me to a patch set he wrote to do the data generation via gyp:
https://codereview.chromium.org/1000163003/

This might be a viable path forward.

Comment 3

[(not currently active) Ted Mielczarek]

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

Comment 4

[(not currently active) Ted Mielczarek]

That try push contains my WIP:
https://hg.mozilla.org/try/rev/38c4a3fd7b3a

It doesn't handle generating the data file, instead I just have an extra commit that adds it to the srcdir for now:
https://hg.mozilla.org/try/rev/4760038147e6

This built OK locally on Linux/Mac/Win.

Comment 5

[(not currently active) Ted Mielczarek]

After talking with gps and Waldo on IRC I decided we're just going to commit the generated data file to the source tree. I'll change the update-icu script to do a local ICU build with configure+make to generate a new data file when updating to a new ICU.

Currently we're wasting developer CPU cycles rebuilding this data file for every clobber build when it only actually changes with every ICU update.

Comment 6

[(not currently active) Ted Mielczarek]

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

Comment 7

[(not currently active) Ted Mielczarek]

Patch is getting closer to finished, I was able to remove the hacky bits in the JS shell from bug 926980.

Comment 8

[(not currently active) Ted Mielczarek]

That try push burned most platforms because I forgot to include `ALLOW_COMPILER_WARNINGS = True`, and it broke Android because Android doesn't build ICU by default and I unconditionally included USE_LIBS for it.

Comment 9

[(not currently active) Ted Mielczarek]

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

Comment 10

[(not currently active) Ted Mielczarek]

Attachment: MozReview Request: bug 1239083 - use moz.build files to build ICU. r?glandium,waldo

Stop invoking ICU's autoconf build system. Instead, have hand-authored
moz.build files under config/external/icu to build what we need. In addition,
we'll commit a pre-built copy of the ICU data file (currently icudt56l.dat)
under config/external/icu/data to avoid having to build ICU host tools to
generate it. config/external/icu/data also contains some assembly files
which can generate an object file containing the ICU data file contents
so that the JS shell (or standalone JS builds) can be linked directly to
the data without having to deal with the external data file. This requires
yasm or GNU as.

The update-icu.sh script has been modified to read the list of C/C++ source
files out of the ICU Makefiles and update `sources.mozbuild` files under
config/external/icu, as well as build a local copy of ICU using its
autoconf build system to generate the ICU data file to be committed in-tree.

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

Comment 11

[(not currently active) Ted Mielczarek]

Waldo: this patch should address your concerns from bug 926980. It undoes all of the JS shell/jit-test code changes by using some assembly to link the ICU data directly into the JS shell and related programs, as well as libmozjs when doing a standalone JS build. I think these two patches are complicated enough to keep them separate, so consider the awfulness in the previous patch as merely a band-aid to ensure that that commit is usable by itself. I intend to land both commits together, so we shouldn't actually have that badness present in the tree at any time.

glandium: this patch is ready for review, but not quite ready to land. The last known issue I have is that I made icu.m4 check for yasm or GNU as to ensure we can build the assembly files, but that gets invoked from the JS subconfigure, which doesn't have the yasm checks like the top-level configure. This builds on Linux currently because $GNU_AS is true, but it fails on Mac/Win. My plan is to put patches in my stack right before this one that move all the yasm checks into pyconfigure and also move all of icu.m4 to pyconfigure, and then share the yasm checks with js/src/configure (probably overkill to solve this specific problem, but it moves us closer to that goal as well).

Comment 12

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11)
> My plan is to put patches in my stack right before this
> one that move all the yasm checks into pyconfigure and also move all of
> icu.m4 to pyconfigure, and then share the yasm checks with js/src/configure
> (probably overkill to solve this specific problem, but it moves us closer to
> that goal as well).

You should avoid moving icu.m4 to pyconfigure before this here lands, because it's complex to get the subconfigure invocations right. You can however move the yasm check and make its result available to both old-configure and js/src/old-configure, that'd be way simpler.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8731305 [details]
MozReview Request: bug 1239083 - use moz.build files to build ICU. r?glandium,waldo

https://reviewboard.mozilla.org/r/40483/#review38421

I'll need to do another pass. At this point, I'm not sure what you gain from having this separate from bug 926980

::: build/autoconf/icu.m4:82
(Diff revision 1)
>      MOZ_ICU_VERSION="$version"
>  
> -    AC_SUBST(MOZ_ICU_VERSION)
> -
> -    if test -z "$MOZ_SYSTEM_ICU"; then
> -        case "$OS_TARGET" in
> +    #TODO: the l is actually endian-dependent, but we don't currently
> +    # have an endianness check in configure. If someone adds one,
> +    # we could make this set as 'l' or 'b' for little or big, respectively.
> +    ICU_DATA_FILE="icudt${version}l.dat"

I think we shouldn't have a version for the file in tree for two reasons:
- it will allow version control to display a log of the file updates in a straightforward matter (not having to realize that you need to use the directory because the file is different every time it is updated)
- it possibly allows version control to better store the data (I don't know if mercurial does binary diffs between revisions of binary files, but git certainly does)

::: config/external/icu/common/moz.build:26
(Diff revision 1)
> +#FIXME: should probably stop including mozilla-config.h
> +DEFINES['LOCALE_SNAME'] = 0x5c
> +
> +LOCAL_INCLUDES += CONFIG['MOZ_ICU_INCLUDES']
> +
> +include('/config/external/icu/defs.mozbuild')

../defs.mozbuild

::: config/external/icu/data/icudata_gas.S:9
(Diff revision 1)
> +
> +.global ICU_DATA_SYMBOL
> +.data
> +.balign 16
> +ICU_DATA_SYMBOL:
> +        .incbin ICU_DATA_FILE

You need the GNU-stack thing in this file too. The syntax is different, though.

::: config/external/icu/defs.mozbuild:8
(Diff revision 1)
> +    U_USING_ICU_NAMESPACE = 0,
> +    U_NO_DEFAULT_INCLUDE_UTF_HEADERS = 1,
> +    UCONFIG_NO_LEGACY_CONVERSION = True,
> +    UCONFIG_NO_TRANSLITERATION = True,
> +    UCONFIG_NO_REGULAR_EXPRESSIONS = True,
> +    UCONFIG_NO_BREAK_ITERATION = True,
> +    U_CHARSET_IS_UTF8 = True,

Can you add the corresponding comments from icu.m4?

::: config/external/icu/defs.mozbuild:23
(Diff revision 1)
> +
> +# ICU requires RTTI
> +if CONFIG['GNU_CXX']:
> +    CXXFLAGS += ['-frtti']
> +elif CONFIG['OS_TARGET'] == 'WINNT':
> +    CXXFLAGS += ['-GR']

does MSVC do the right thing when fed with both -GR- and -GR?

::: intl/update-icu.sh:75
(Diff revision 1)
> -hg addremove ${icu_dir}
> +
> +hg addremove ${icu_dir} ${topsrcdir}/config/external/icu
> +
> +# Update our moz.build files in config/external/icu, and
> +# build a new ICU data file.
> +python `dirname $0`/icu_sources_data.py $topsrcdir

You want this before hg addremove

Comment 14

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/40483/#review38421

I wrote that patch first because I wanted to try to split the work up because they're both pretty involved, but you're right, it doesn't buy us much in terms of the implementation. Would you like me to just squash the two patches and see what the combined diff looks like? Alternately, I could break the patch from that bug down into just the bits that don't get reverted by this patch, which means it would not be buildable standalone, but might make both of them more easily reviewable, and I could squash them on landing.

> I think we shouldn't have a version for the file in tree for two reasons:
> - it will allow version control to display a log of the file updates in a straightforward matter (not having to realize that you need to use the directory because the file is different every time it is updated)
> - it possibly allows version control to better store the data (I don't know if mercurial does binary diffs between revisions of binary files, but git certainly does)

So, if we want to use `u_setDataDirectory`, we need to have the version number in the filename, because that's how ICU looks for it. That means we'd either have to grow a moz.build capability to rename files in `FINAL_TARGET_FILES` to put the version number in there, or we'd have to switch to reading the whole .dat file on startup and using `udata_setCommonData` http://icu-project.org/apiref/icu4c/udata_8h.html#a467bda719595adb58f959dde735e1153 .

Thoughts? (I'm pretty sure gps has said that hg doesn't do binary diffs.)

> does MSVC do the right thing when fed with both -GR- and -GR?

I'm not entirely sure, but this does build on Windows. I will check.

Comment 15

[Mike Hommey [:glandium]]

I think I'd rather try to review a squashed patch.

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 8731305 [details]
MozReview Request: bug 1239083 - use moz.build files to build ICU. r?glandium,waldo

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

Comment 17

[(not currently active) Ted Mielczarek]

I squashed this patch with the one from bug 926980, and fixed all the review comments except for the version number in the .dat filename, and checking whether MSVC does the right thing with -GR- -GR.

Comment 18

[(not currently active) Ted Mielczarek]

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

Comment 19

[(not currently active) Ted Mielczarek]

The try results are mostly OK, except cppunittests are busted on Mac/Windows, failing to JS_Init presumably because the icu data file isn't packaged with cppunittests.

Part of this is because of course we have C++ unittests that init xpcom without using the test harness helpers:
https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/compiled/TestCertDB.cpp#15

Comment 20

[(not currently active) Ted Mielczarek]

https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/security/manager/ssl/tests/compiled/TestCertDB.cpp#15
https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/netwerk/test/TestCookie.cpp#189

Comment 21

[(not currently active) Ted Mielczarek]

https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/xpcom/tests/TestTArray.cpp#1197

Comment 22

[(not currently active) Ted Mielczarek]

Pushed with another change to make those tests use ScopedXPCOM:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e9cd92bb1d4

Comment 23

[(not currently active) Ted Mielczarek]

(Everything else on that Try push looked OK except the cppunittests.)

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8731305 [details]
MozReview Request: bug 1239083 - use moz.build files to build ICU. r?glandium,waldo

https://reviewboard.mozilla.org/r/40483/#review39025

One pass for the nits. I'll need one last pass.

::: build/autoconf/icu.m4:85
(Diff revision 2)
> -
> -    if test -z "$MOZ_SYSTEM_ICU"; then
> -        case "$OS_TARGET" in
> -            WINNT)
> -                ICU_LIB_NAMES="icuin icuuc"
> -                ICU_DATA_LIB=icudt
> +    # have an endianness check in configure. If someone adds one,
> +    # we could make this set as 'l' or 'b' for little or big, respectively.
> +    ICU_DATA_FILE="icudt${version}l.dat"
> +
> +    dnl We won't build ICU data as a separate file when building
> +    dnl JS standalone so that embeddors don't have to deal with it.

typo: embedders

::: build/autoconf/icu.m4:87
(Diff revision 2)
> -        case "$OS_TARGET" in
> -            WINNT)
> -                ICU_LIB_NAMES="icuin icuuc"
> -                ICU_DATA_LIB=icudt
> -                MOZ_ICU_DBG_SUFFIX=
> -                if test -n "$MOZ_DEBUG" -a -z "$MOZ_NO_DEBUG_RTL"; then
> +    ICU_DATA_FILE="icudt${version}l.dat"
> +
> +    dnl We won't build ICU data as a separate file when building
> +    dnl JS standalone so that embeddors don't have to deal with it.
> +    if test -z "$JS_STANDALONE" -a -z "$MOZ_SYSTEM_ICU"; then
> +        MOZ_ICU_DATA_ARCHIVE=1

you should explicitly set it to nothing in the other case

::: config/external/icu/data/icudata.s:11
(Diff revision 2)
> +    %define DATA_SYMBOL _ %+ ICU_DATA_SYMBOL
> +%else
> +    %define DATA_SYMBOL ICU_DATA_SYMBOL
> +%endif
> +
> +%define FORMAT_ELF 0

I'd put that in a %else

::: config/external/icu/data/moz.build:22
(Diff revision 2)
> +    '-DICU_DATA_FILE="' + CONFIG['ICU_DATA_FILE'] + '"',
> +    '-DICU_DATA_SYMBOL=icudt' + CONFIG['MOZ_ICU_VERSION'] + '_dat',

Use %s

::: intl/icu_sources_data.py:29
(Diff revision 2)
> +    base = os.path.splitext(filename)[0]
> +    for ext in ('.cpp', '.c'):
> +        f = os.path.join(dir, base + ext)
> +        if os.path.isfile(f):
> +            return f
> +    raise Exception('Couldn\'t find source file for: %s' % filename)

You might as well use double quotes to avoid the escaping.

Comment 25

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/40483/#review39025

> I'd put that in a %else

I cargo-culted this all from some media code, but OK.

> Use %s

I definitely wish I could use DEFINES here, but I didn't want to open that can of worms. (I've got enough worms from this patch series already.)

Comment 26

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/40483/#review38421

> So, if we want to use `u_setDataDirectory`, we need to have the version number in the filename, because that's how ICU looks for it. That means we'd either have to grow a moz.build capability to rename files in `FINAL_TARGET_FILES` to put the version number in there, or we'd have to switch to reading the whole .dat file on startup and using `udata_setCommonData` http://icu-project.org/apiref/icu4c/udata_8h.html#a467bda719595adb58f959dde735e1153 .
> 
> Thoughts? (I'm pretty sure gps has said that hg doesn't do binary diffs.)

Still looking for feedback on whether you really want this changed.

Comment 27

[(not currently active) Ted Mielczarek]

Comment on attachment 8731305 [details]
MozReview Request: bug 1239083 - use moz.build files to build ICU. r?glandium,waldo

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

Comment 28

[Jeff Walden [:Waldo]]

Comment on attachment 8731305 [details]
MozReview Request: bug 1239083 - use moz.build files to build ICU. r?glandium,waldo

https://reviewboard.mozilla.org/r/40483/#review40883

This is an r+, assuming the comments don't expose some deep-seated issue.  But I dunno if autoland or something won't go to town on this if I mark "Ship It" or fail to "Open an Issue" for the little nitpicks in it.  Scumbag mozreview UI.  :-)

::: build/autoconf/icu.m4:79
(Diff revision 3)
>      if test x"$version" = x; then
>         AC_MSG_ERROR([cannot determine icu version number from uvernum.h header file $lineno])
>      fi
>      MOZ_ICU_VERSION="$version"
>  
> -    AC_SUBST(MOZ_ICU_VERSION)
> +    #TODO: the l is actually endian-dependent, but we don't currently

Space before TODO.

::: config/external/icu/data/moz.build:12
(Diff revision 3)
> +if CONFIG['MOZ_ICU_DATA_ARCHIVE']:
> +    # Copy the pre-built ICU data file to dist/bin.
> +    FINAL_TARGET_FILES += [CONFIG['ICU_DATA_FILE']]
> +
> +# Build a library containing the ICU data for use in the JS shell, so that
> +# JSAPI consumers don't have to deal with setting ICU's data path.

\o/

::: config/external/icu/defs.mozbuild:9
(Diff revision 3)
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DEFINES.update(
> +    # Don't use icu namespace automatically in client code.
> +    U_USING_ICU_NAMESPACE = 0,

Let's add blank lines between the separate argument-blocks here -- so one after the ICU_NAMESPACE line, one after the UTF_HEADERS line, one after ITERATION.

::: config/external/icu/i18n/sources.mozbuild:3
(Diff revision 3)
> +# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> +# vim: set filetype=python:
> +# THIS FILE IS GENERATED BY /intl/icusources.py DO NOT EDIT

We have intl/icu/source for imported stuffs, so I wonder if we should move some of this ICU-relevant stuff into intl/icu, out of intl/ or intl/icu-patches.  Maybe sometime.

::: intl/icu_sources_data.py:51
(Diff revision 3)
> +    with open(mozbuild, 'wb') as f:
> +        f.write(('# -*- Mode: python; c-basic-offset: 4; ' +
> +                 'indent-tabs-mode: nil; tab-width: 40 -*-'))
> +        f.write('''
> +# vim: set filetype=python:
> +# THIS FILE IS GENERATED BY /intl/icusources.py DO NOT EDIT

This file name is wrong.

::: js/src/moz.build:622
(Diff revision 3)
>  
>  if CONFIG['ENABLE_INTL_API']:
> +    if not CONFIG['MOZ_ICU_DATA_ARCHIVE']:
> -    USE_LIBS += [
> +        USE_LIBS += [
> -        'icu',
> +            'icu',
> -    ]
> +        ]

Er, are those >>>> really in the patch, or is this reviewboard being unhelpful/misleading?

::: js/src/shell/moz.build:40
(Diff revision 3)
>  
>  OS_LIBS += CONFIG['EDITLINE_LIBS']
>  OS_LIBS += CONFIG['MOZ_ZLIB_LIBS']
>  
> +if CONFIG['ENABLE_INTL_API'] and CONFIG['MOZ_ICU_DATA_ARCHIVE']:
> +    # The ICU libraries linked into libjs will not include the ICU data,

If memory serves, we don't ship libjs, we ship libmozjs -- s/libjs/libmozjs/g on all your changes, please.

Comment 29

[Jeff Walden [:Waldo]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #26)
> > So, if we want to use `u_setDataDirectory`, we need to have the version number
> > in the filename, because that's how ICU looks for it.

Bah.  Okay, good enough for now.

Comment 30

[Mike Hommey [:glandium]]

Comment on attachment 8731305 [details]
MozReview Request: bug 1239083 - use moz.build files to build ICU. r?glandium,waldo

https://reviewboard.mozilla.org/r/40483/#review40949

::: build/autoconf/icu.m4:79
(Diff revision 3)
> -    AC_SUBST(MOZ_ICU_VERSION)
> -
> +    #TODO: the l is actually endian-dependent, but we don't currently
> +    # have an endianness check in configure. If someone adds one,

Note this is not true anymore. We *do* have endianness in configure (not exposed in old-configure, but it's a add_old_configure_assignment away).

::: config/external/icu/data/moz.build:25
(Diff revision 3)
> +
> +ASFLAGS += [
> +    '-DICU_DATA_FILE="%s"' % CONFIG['ICU_DATA_FILE'],
> +    '-DICU_DATA_SYMBOL=icudt%s_dat' % CONFIG['MOZ_ICU_VERSION'],
> +]
> +LOCAL_INCLUDES += ['.']

you shouldn't need this, since srcdir and CURDIR are both in INCLUDES in config.mk

::: intl/icu_sources_data.py:47
(Diff revision 3)
> +        f.write(('# -*- Mode: python; c-basic-offset: 4; ' +
> +                 'indent-tabs-mode: nil; tab-width: 40 -*-'))
> +        f.write('''
> +# vim: set filetype=python:

if the file is generated, why add emacs and vim mode lines?

::: intl/icu_sources_data.py:96
(Diff revision 3)
> +        'CFLAGS': '-DU_STATIC_IMPLEMENTATION',
> +        'CXXFLAGS': '-DU_STATIC_IMPLEMENTATION',
> +        'CPPFLAGS': ('-DU_USING_ICU_NAMESPACE=0 ' +
> +                     '-DU_NO_DEFAULT_INCLUDE_UTF_HEADERS=1 ' +
> +                     '-DUCONFIG_NO_LEGACY_CONVERSION ' +
> +                     '-DUCONFIG_NO_TRANSLITERATION ' +
> +                     '-DUCONFIG_NO_REGULAR_EXPRESSIONS ' +
> +                     '-DUCONFIG_NO_BREAK_ITERATION ' +
> +                     '-DU_CHARSET_IS_UTF8')

This would feel better if it was somehow shared with moz.build. Followup?

::: intl/icu_sources_data.py:96
(Diff revision 3)
> +        'CFLAGS': '-DU_STATIC_IMPLEMENTATION',
> +        'CXXFLAGS': '-DU_STATIC_IMPLEMENTATION',

Presumably, that's redundant with --enable-static.

::: xpcom/build/XPCOMInit.cpp:695
(Diff revision 3)
>    // the pointer argument when a size of 0 is passed in, so we need
>    // the special version of the counting realloc.
>    nestegg_set_halloc_func(NesteggReporter::CountingFreeingRealloc);
>  #endif
>  
> +#if EXPOSE_INTL_API && defined(MOZ_ICU_DATA_ARCHIVE)

The define is only used in this one place, let's not make it an AC_DEFINE just for that.

::: xpcom/build/XPCOMInit.cpp:695
(Diff revision 3)
> +#if EXPOSE_INTL_API && defined(MOZ_ICU_DATA_ARCHIVE)
> +  nsCOMPtr<nsIFile> greDir;
> +  nsDirectoryService::gService->Get(NS_GRE_DIR,
> +                                    NS_GET_IID(nsIFile),
> +                                    getter_AddRefs(greDir));
> +  MOZ_ASSERT(greDir);
> +  nsAutoCString nativeGREPath;
> +  greDir->GetNativePath(nativeGREPath);
> +  u_setDataDirectory(nativeGREPath.get());
> +#endif

Please file a followup bug for the android support, because the file won't be there, but in the apk.

Comment 31

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/40483/#review40883

> We have intl/icu/source for imported stuffs, so I wonder if we should move some of this ICU-relevant stuff into intl/icu, out of intl/ or intl/icu-patches.  Maybe sometime.

Not a bad idea, I just stuck everything in config/external/icu because that's where the existing Makefile was. (I also did the same thing when I switched the NSPR build over, all those moz.build files are in config/external/nspr).

> Er, are those >>>> really in the patch, or is this reviewboard being unhelpful/misleading?

That's just reviewboard's way of indicating that indentation has changed.

Comment 32

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/40483/#review40949

> Note this is not true anymore. We *do* have endianness in configure (not exposed in old-configure, but it's a add_old_configure_assignment away).

I'll remove that from the comment, but obviously I'm not going to fix that because a) I don't have a big-endian machine handy and b) I don't really think it's worth committing a big-endian ICU data file to the tree. I suppose we could use that and force big-endian builds to use `--with-system-icu` or `--without-intl-api`.

> you shouldn't need this, since srcdir and CURDIR are both in INCLUDES in config.mk

Except for some reason the SOBJS build rule only uses `$(LOCAL_INCLUDES)` and not `$(INCLUDES)`:
https://dxr.mozilla.org/mozilla-central/rev/9bd90088875399347b05d87c67d3709e31539dcd/config/rules.mk#913

> if the file is generated, why add emacs and vim mode lines?

I think I just copy/pasted a header from an existing moz.build file. Kinda silly.

> This would feel better if it was somehow shared with moz.build. Followup?

Filed bug 1262101.

> Presumably, that's redundant with --enable-static.

I don't actually think it is, but I also think it might be irrelevant, since this define only controls the visibility/linkage of the public API symbols, so it's only relevant for our code that calls ICU APIs. I'll just remove it.

> The define is only used in this one place, let's not make it an AC_DEFINE just for that.

I think that was leftover from my previous patch that was using that in various places in js/src.

> Please file a followup bug for the android support, because the file won't be there, but in the apk.

y'know, I spent a bunch of time making sure everything built for Android `--with-intl-api`, but I never actually tried running the resulting build. The simplest fix here would just be to link in the ICU data file we generate for the JS shell. Filed bug 1262102 on this.

Comment 33

[(not currently active) Ted Mielczarek]

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

Comment 34

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b65d504944ef45c9dd33ae45b922f5f438c12867
bug 1239083 - use moz.build files to build ICU. r=glandium,waldo

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b65d504944ef

Comment 36

[Steven R. Loomis]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #0)
> ICU's build system is terrible:
> * It's not parallel-safe, so we have to build with -j1.

Wait, what? I build parallel all the time… Do you have any more details on this?

Comment 37

[Steven R. Loomis]

also you shouldn't have to scrape the makefiles… I'd think icu/icu4c/source/test/depstest/dependencies.txt would be a better machine readable source for building.

you can also just build *.cpp - that's what I do for node, with certain files opted out.

Comment 38

[(not currently active) Ted Mielczarek]

Hi Steven, thanks for the info! Sorry if it sounds like I'm being harsh on the ICU build system, it's just that I hate all build systems. :)

(In reply to Steven R. Loomis from comment #36)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #0)
> > ICU's build system is terrible:
> > * It's not parallel-safe, so we have to build with -j1.
> 
> Wait, what? I build parallel all the time… Do you have any more details on
> this?

Huh! I had to dig into the details, and it looks like we added this in bug 869406. This is what the build invocation looked like before that patch:
https://hg.mozilla.org/mozilla-central/annotate/a4f96de49668/js/src/Makefile.in#l260

and after:
https://hg.mozilla.org/mozilla-central/annotate/1a1968da61b3/js/src/Makefile.in#l208

We had been forcing -j1 on Windows due to known issues with parallel builds with the msys gmake we were using, but apparently we were still hitting issues on other platforms. Now granted, that was 4 years ago, so it's possible there were issues that have since been fixed.

(In reply to Steven R. Loomis from comment #37)
> also you shouldn't have to scrape the makefiles… I'd think
> icu/icu4c/source/test/depstest/dependencies.txt would be a better machine
> readable source for building.
> 
> you can also just build *.cpp - that's what I do for node, with certain
> files opted out.

Thanks, I just checked and the output of scraping the Makefiles does in fact produce the same output as just listing *.cpp and *.c from the {common,i18n} directories. I'll file a bug to change the update script to do that since it's much simpler.