724533 - integrate ICU into the mozilla build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jonathan Kew [:jfkthame]]

ICU has its own pretty large and complex build system, including support for cross-compilation, custom tools that need to be built and then run on the host as part of the build process, etc. Not sure how tricky it'll be to integrate this into our build...

Note that both JS and other parts of Gecko (intl/*, gfx, etc) will eventually depend on ICU.

Also need to decide how we'll package ICU for our products. By default, it gets built as a half-dozen DLLs, but we may prefer to try and reduce that number.

Distros may want us to offer a --with-system-icu option, too.

Comment 1

[Benjamin Smedberg]

After reading bug 724531, I don't think it's worth worrying about exactly where to put it and how we'd build it in general until the initial investigation phase is done. In the meantime I'd just build it separately and add it manually to the makefiles. Unless you're trying to collaborate with others on the patch series or something?

Comment 2

[Norbert Lindenberg]

Attachment: Make ICU build with Mozilla build for Windows (WIP)

Comment 3

[Norbert Lindenberg]

Attachment: Integrate ICU into the Mozilla build (WIP)

Comment 4

[Norbert Lindenberg]

Attachment: Make ICU build with Mozilla build for Windows (WIP)

Comment 5

[Norbert Lindenberg]

Attachment: Integrate ICU into the Mozilla build (WIP)

This version builds ICU libraries and links them into the JavaScript library on Mac and Windows - other platforms still to come.

The main portions are in the configure.in and Makefile.in files in js/src. This works and is semi-reasonable because at this point only JavaScript uses ICU. However, it also indicates a design problem: Normally, instructions for installing the header files and building the libraries should be in configure.in and Makefile.in files in the ICU source directory, intl/icu. The reason they are not is that ICU is used for two separate and somewhat different builds: The overall build whose main product is Firefox, and the SpiderMonkey build whose main product is the js shell. I have not been able to figure out how to make the second work with separate configure.in and Makefile.in files.

I'd appreciate help in this regard; as well as any other feedback on this patch.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 712056 [details] [diff] [review]
Integrate ICU into the Mozilla build (WIP)

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

Thought I had published this.

::: browser/app/Makefile.in
@@ +62,5 @@
>  	$(EXTRA_DSO_LIBS) \
>  	$(XPCOM_STANDALONE_GLUE_LDOPTS) \
>  	$(NULL)
>  
> +EXTRA_LIBS += $(ICU_LIBS)

I don't think you really want to link ICU here. We intentionally avoid linking directly against most things in the browser app so we can optimize library loading.

::: config/config.mk
@@ +662,5 @@
>  
> +# ECMAScript Internationalization API
> +ifdef ENABLE_INTL_API
> +DEFINES += -DENABLE_INTL_API -DU_STATIC_IMPLEMENTATION
> +endif

I'd just put these as AC_DEFINEs in configure.

::: configure.in
@@ +4129,5 @@
> +[  --enable-intl-api       Enable ECMAScript Internationalization API],
> +    ENABLE_INTL_API=1 )
> +
> +if test -n "$ENABLE_INTL_API"; then
> +    case "$target_os" in

It's usually saner to use OS_TARGET, which is sanitized to a set of known values.

@@ +4132,5 @@
> +if test -n "$ENABLE_INTL_API"; then
> +    case "$target_os" in
> +        mingw*)
> +            ICU_LIB_NAMES="icuin icuuc icudt"
> +            ICU_LIBS='$(foreach libname,$(ICU_LIB_NAMES),$(DIST)/lib/s$(libname).lib)'

EXPAND_LIBNAME_PATH should do exactly this on Windows.

@@ +4138,5 @@
> +        *)
> +            ICU_LIB_NAMES="icui18n icuuc icudata"
> +            ICU_LIBS='$(call EXPAND_LIBNAME_PATH,$(ICU_LIB_NAMES),$(DIST)/lib)'
> +    esac
> +    ICU_BUILD=intl/icu

I don't think you need to put the build directory in a variable, it's a hardcoded thing.

::: js/src/Makefile.in
@@ +633,5 @@
> +		$(INSTALL) $(ICU_LIB_BUILDS) $(DIST)/lib
> +
> +distclean clean::
> +		$(call SUBMAKE,$@,$(ICU_BUILD))
> +endif

This is really messy. I don't have a great idea for how to make the standalone JS build do this easily in a cleaner fashion, but I don't think I want to do something like this.

::: js/src/configure.in
@@ +4321,5 @@
> +[  --enable-intl-api       Enable ECMAScript Internationalization API],
> +    ENABLE_INTL_API=1 )
> +
> +if test -n "$ENABLE_INTL_API"; then
> +    case "$target_os" in

OS_TARGET

::: toolkit/library/Makefile.in
@@ +367,5 @@
>    $(NSS_LIBS) \
>    $(MOZ_CAIRO_OSLIBS) \
>    $(MOZ_APP_EXTRA_LIBS) \
>    $(SQLITE_LIBS) \
> +  $(ICU_LIBS) \

Is there a reason you're linking both JS and libxul to ICU_LIBS? Seems redundant.

Comment 7

[Norbert Lindenberg]

Attachment: Make ICU headers available to SpiderMonkey build

This is just a minimal patch to make ICU headers available to the SpiderMonkey build, so that we can start landing ICU based code for the ECMAScript Internationalization API. Note that the API is still disabled, and I'm stubbing out ICU functions in the default build so that we won't see a jump in the size of the binaries yet.

Comment 8

[Landry Breuil (:gaston)]

(In reply to Jonathan Kew (:jfkthame) from comment #0)

> Distros may want us to offer a --with-system-icu option, too.

Yes, definitely - should this be done as an additional patch in this bug so that it's here from the start, or better filed as a separate bug ? As with most other --with-system-XXX options, i suppose it'll only be tested by distributors (ie no tbpl build with it..) ?

Comment 9

[Gregory Szorc [:gps]]

If we attempt to build ICU on Windows with pymake, we're in for a world of hurt:

SyntaxError: /Users/gps/src/mozilla-central-hg/intl/icu/source/data/Makefile.in:485:38:order-only prerequisites not implemented
SyntaxError: /Users/gps/src/mozilla-central-hg/intl/icu/source/extra/uconv/Makefile.in:150:38:order-only prerequisites not implemented

(pymake doesn't support order-only prerequisites)

Comment 10

[Mike Hommey [:glandium]]

And the mix of pymake/make is usually another kind of world of hurt.

Comment 11

[Gregory Szorc [:gps]]

FWIW, these are now the only occurrences of order-only prerequisite rules in the tree AFAICT. We've had previous discussions about supporting order-only prerequisites in pymake and a few people were strongly opposed to the idea.

Comment 12

[Norbert Lindenberg]

Mike, can you elaborate on the "world of hurt"? In my current build files, I simply treat ICU as a separate build, and explicitly invoke gmake. It's not elegant, but it seems to work.

Comment 13

[Gregory Szorc [:gps]]

(In reply to Norbert Lindenberg from comment #12)
> Mike, can you elaborate on the "world of hurt"? In my current build files, I
> simply treat ICU as a separate build, and explicitly invoke gmake. It's not
> elegant, but it seems to work.

a) GNU make and pymake expect a different style of paths in Makefiles (Windows native vs POSIX).

b) gmake doesn't like to run in parallel on Windows. Compiling things not in parallel slows down the build.

Comment 14

[Norbert Lindenberg]

Attachment: Make ICU headers available to SpiderMonkey build

Minimal patch to make ICU headers available to the SpiderMonkey build, so that we can start landing ICU based code for the ECMAScript Internationalization API. Also includes an AC_DEFINE to stop ICU from making its clients use its namespace by default. Note that the API is still disabled, and I'm stubbing out ICU functions in the default build so that we won't see a jump in the size of the binaries yet.

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 722065 [details] [diff] [review]
Make ICU headers available to SpiderMonkey build

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

::: js/src/Makefile.in
@@ +591,5 @@
> +export::
> +	mkdir -p $(DIST)/include/unicode
> +	cp $(topsrcdir)/../../intl/icu/source/common/unicode/*.h $(DIST)/include/unicode
> +	cp $(topsrcdir)/../../intl/icu/source/i18n/unicode/*.h $(DIST)/include/unicode
> +

Please directly add appropriate -Ipath arguments to LOCAL_INCLUDES.

::: js/src/configure.in
@@ +4327,5 @@
> +dnl ========================================================
> +
> +dnl Source files that use ICU should have control over which parts of the ICU
> +dnl namespace they want to use.
> +AC_DEFINE(U_USING_ICU_NAMESPACE,0)

I'm not sure I like that, from the perspective of optionally using a system ICU (which would use a given namespace). What was your plan for the source files using ICU?

Comment 16

[Norbert Lindenberg]

Attachment: Make ICU headers available to SpiderMonkey build

Updated per comment 15.

(In reply to Mike Hommey [:glandium] from comment #15)

> > +dnl Source files that use ICU should have control over which parts of the ICU
> > +dnl namespace they want to use.
> > +AC_DEFINE(U_USING_ICU_NAMESPACE,0)
> 
> I'm not sure I like that, from the perspective of optionally using a system
> ICU (which would use a given namespace). What was your plan for the source
> files using ICU?

Without this definition, ICU injects a "using namespace icu;" into its include files. Not even the ICU team thinks that's a good idea; they just do it for compatibility with old compilers:
http://source.icu-project.org/repos/icu/icu/trunk/readme.html#RecBuild

I'm using "using icu::<type>;" just for those types I need:
https://bug837957.bugzilla.mozilla.org/attachment.cgi?id=722073
I think that should work just as well for anybody building with a system ICU. The ICU headers actually mangle that into a namespace name that includes the ICU version used, so for me currently icu_50. Developers building with a system ICU would have to use the header files for their version of ICU so that they get the matching namespace. So, the build files would have to change, but the C++ source files wouldn't.

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 723337 [details] [diff] [review]
Make ICU headers available to SpiderMonkey build

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

r+ but this is going to make standalone releases harder.

Comment 18

[Norbert Lindenberg]

(In reply to Mike Hommey [:glandium] from comment #17)

> r+ but this is going to make standalone releases harder.

Do you mean harder in the sense that now ICU headers are required, or something else in this patch that I could have done better? How to integrate the full ICU into the SpiderMonkey build is a puzzle I still have to solve, and I'll probably have some more questions for you in that process.

Comment 19

[Mike Hommey [:glandium]]

(In reply to Norbert Lindenberg from comment #18)
> (In reply to Mike Hommey [:glandium] from comment #17)
> 
> > r+ but this is going to make standalone releases harder.
> 
> Do you mean harder in the sense that now ICU headers are required, or

In the sense that the ICU headers are required *in the mozilla tree*.

Comment 20

[Jeff Walden [:Waldo]]

I filed bug 850379 to fix the standalone story.  Right now I don't think we want to block intl stuff on that.  Plus it really requires input from embedders as to what exactly is the most desirable solution, which would be even more of a blocker right now to forward motion.  We have a couple release cycles to figure it all out before the start of the ESR cutoff, and I think we can deal with it after intl stuff lands, but before then, without too much trouble.

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 723337 [details] [diff] [review]
Make ICU headers available to SpiderMonkey build

https://hg.mozilla.org/integration/mozilla-inbound/rev/082bdc8fcc2a

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/082bdc8fcc2a

Comment 23

[Norbert Lindenberg]

(In reply to Landry Breuil (:gaston) from comment #8)
> (In reply to Jonathan Kew (:jfkthame) from comment #0)
> 
> > Distros may want us to offer a --with-system-icu option, too.
> 
> Yes, definitely - should this be done as an additional patch in this bug so
> that it's here from the start, or better filed as a separate bug ?

I've created bug 851992 for this.

Comment 24

[Norbert Lindenberg]

Attachment: Make ICU build with Mozilla build for Windows

Ted, can you please rubberstamp, oops, review this patch? This started with your suggestion at
https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.builds/Vx0XwdAYaz4

The real review is going to happen when I work with the ICU team on integrating the patch into ICU:
http://bugs.icu-project.org/trac/ticket/9985

Comment 25

[Norbert Lindenberg]

(In reply to Gregory Szorc [:gps] from comment #9)
> If we attempt to build ICU on Windows with pymake, we're in for a world of
> hurt:
> 
> SyntaxError:
> /Users/gps/src/mozilla-central-hg/intl/icu/source/data/Makefile.in:485:38:
> order-only prerequisites not implemented
> SyntaxError:
> /Users/gps/src/mozilla-central-hg/intl/icu/source/extra/uconv/Makefile.in:
> 150:38:order-only prerequisites not implemented
> 
> (pymake doesn't support order-only prerequisites)

I have to admit that I have no idea what a fix for that would look like - would it be some local changes or major surgery? If you can propose a reasonably-sized fix, I do have a ticket open with the ICU team to add support for the MSYS/MSVC build environment; we could probably extend that a bit to address additional Mozilla build issues.

Comment 26

[Norbert Lindenberg]

Attachment: Integrate ICU into the Mozilla build

I've reworked the previous patch to a more traditional format for the Firefox build, ignoring for now how to build SpiderMonkey stand-alone with the internationalization API and ICU.

I also addressed comment 6, with one exception:

> ::: toolkit/library/Makefile.in
> @@ +367,5 @@
> >    $(NSS_LIBS) \
> >    $(MOZ_CAIRO_OSLIBS) \
> >    $(MOZ_APP_EXTRA_LIBS) \
> >    $(SQLITE_LIBS) \
> > +  $(ICU_LIBS) \
> 
> Is there a reason you're linking both JS and libxul to ICU_LIBS? Seems
> redundant.

Removing this made the Mac build fail - apparently ICU isn't included in the JavaScript library.

Try build with internationalization API and ICU enabled for desktop Firefox:
https://tbpl.mozilla.org/?tree=Try&rev=e10d2afc62c5

Comment 27

[Jeff Walden [:Waldo]]

Ted, was there a reason ICU wasn't put into mfbt to sidestep the ICU-not-in-JS issue?  That seems like the right place for it, given it's a thing used by both Gecko and JS.

Comment 28

[(not currently active) Ted Mielczarek]

It feels like an awfully large thing to shoehorn into MFBT. Per bug 724531 comment 8 and 9, we settled on intl/icu because it felt like a sane place and brendan agreed.

Comment 29

[Gregory Szorc [:gps]]

Comment on attachment 729629 [details] [diff] [review]
Integrate ICU into the Mozilla build

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

I didn't look at all the existing review comments. Forgive me if I hit on something that was already mentioned.

I'd like Mike Hommey to look at the new configure script. He has a better eye for certain things than I do.

::: configure.in
@@ +4159,5 @@
> +dnl ========================================================
> +MOZ_ARG_ENABLE_BOOL(intl-api,
> +[  --enable-intl-api       Enable ECMAScript Internationalization API],
> +    ENABLE_INTL_API=1 )
> +                                         

Nit: trailing whitespace

::: intl/icu/Makefile.in
@@ +10,5 @@
> +VPATH           = @srcdir@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +ifdef ENABLE_INTL_API

Why do we need the ifdef? If ICU isn't enabled, we should never traverse into this directory and Makefile.

@@ +30,5 @@
> +    endif
> +else
> +    ICU_LIB_RENAME = $(foreach libname,$(ICU_LIB_NAMES),\
> +                       cp obj/lib/lib$(libname).a lib/lib$(libname).a;)
> +endif

I'm pretty sure we can combine some of this logic. Are you aware of $(LIB_SUFFIX) (search for it in config.status in your objdir)? We could also stuff the source file in a variable and probably reduce this down to 1 assignment of ICU_LIB_RENAME.

@@ +33,5 @@
> +                       cp obj/lib/lib$(libname).a lib/lib$(libname).a;)
> +endif
> +
> +libs::
> +		$(GMAKE) -C obj GENRBOPTS='-k -R'

I can't remember the default behavior for passing -j, -s, etc down to sub makes. If we pass -j, then gmake -j > 1 on Windows is buggy and gmake -j < # cores is slow everywhere else. Either way, we have a problem to address.

@@ +35,5 @@
> +
> +libs::
> +		$(GMAKE) -C obj GENRBOPTS='-k -R'
> +		$(ICU_LIB_RENAME)
> +		$(INSTALL) $(ICU_LIB_BUILDS) $(DIST)/lib

It looks like there are 2 tabs here. You only need 1.

::: intl/icu/configure
@@ +1,1 @@
> +#!/bin/sh

I'd like Mike Hommey to look at this.

::: intl/moz.build
@@ +12,5 @@
>      'unicharutil',
>  ]
>  
>  DIRS += [
> +    'icu',

You already reference intl/icu in toolkit.mozbuild. You only need 1 reference.

::: js/src/configure.in
@@ +3188,5 @@
> +AC_SUBST(ICU_LIBS)
> +
> +dnl Source files that use ICU should have control over which parts of the ICU
> +dnl namespace they want to use.
> +AC_DEFINE(U_USING_ICU_NAMESPACE,0)

I /think/ this is the same chunk of m4 that we have in the main configure.in. If so, we should extract it into a standalone m4 file in build/autoconf/. See ccache.m4 for an example.

If it's not the same, it looks close enough that we could at least parametrize it. What I'm trying to say is I prefer small, function specific m4 files over large monolithic configure.in.

::: js/src/gdb/Makefile.in
@@ +28,5 @@
>  DEFINES += -DEXPORT_JS_API -DIMPL_MFBT
>  
>  LIBS = $(DEPTH)/$(LIB_PREFIX)js_static.$(LIB_SUFFIX) $(NSPR_LIBS) $(MOZ_ZLIB_LIBS)
>  
> +EXTRA_LIBS += $(ICU_LIBS)

I wonder how often the JS logic for "link with these sets of libraries" is reimplemented and whether it's worth factoring out somehow. Follow-up I reckon.

::: js/src/jsversion.h
@@ -175,5 @@
>       MOZ_NOT_REACHED("don't call this!  to be used in the new object representation")
>  #endif
>  
> -/* ECMAScript Internationalization API isn't fully implemented yet. */
> -#define ENABLE_INTL_API 0

I have no clue what this does and don't feel comfortable reviewing it. Shouldn't this be conditional on ENABLE_INTL_ICU?

Comment 30

[Norbert Lindenberg]

Comment on attachment 729629 [details] [diff] [review]
Integrate ICU into the Mozilla build

Mike, can you please look at intl/icu/configure, as requested by Gregory in comment 29?

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 727470 [details] [diff] [review]
Make ICU build with Mozilla build for Windows

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

mh-msys-msvc scares me. I'm willing to rs+ the rest of the files.

::: intl/icu/source/config/mh-msys-msvc
@@ +1,1 @@
> +## MSYS with Microsoft Visual C++ compiler specific setup

Can you give this file a .mk extension?

@@ +212,5 @@
> +
> +# Include the version information in the shared library
> +ENABLE_SO_VERSION_DATA=1
> +
> +## End MSYS-specific setup

If Ted or somebody else has already reviewed parts of this file, I'd *really* like them to finish it. Reviewing this file properly involves a lot of context - very little of which is currently paged into my brain.

::: intl/icu/source/data/Makefile.in
@@ +350,5 @@
>  ifeq ($(PKGDATA_MODE),dll)
>  SO_VERSION_DATA = $(OUTTMPDIR)/icudata.res
>  $(SO_VERSION_DATA) : $(MISCSRCDIR)/icudata.rc
> +        # fixme: need to tell whether to use - or /, $(SOURCEFILE) or $<
> +	rc.exe -i$(srcdir)/../common -i$(top_builddir)/common -fo$@ $(CPPFLAGS) $<

Why doesn't rc.exe like Windows style /arguments?

Comment 32

[Norbert Lindenberg]

(In reply to Gregory Szorc [:gps] from comment #29)

> ::: js/src/jsversion.h
> > -/* ECMAScript Internationalization API isn't fully implemented yet. */
> > -#define ENABLE_INTL_API 0
> 
> I have no clue what this does and don't feel comfortable reviewing it.
> Shouldn't this be conditional on ENABLE_INTL_ICU?

We use ENABLE_INTL_API everywhere, since the ECMAScript Internationalization API is the feature that all this enables. We used the simple #define in jsversion.h while landing all the C++ and JavaScript code, but now that the build system is also involved, the AC_DEFINE(ENABLE_INTL_API) in configure.in replaces it.

Jeff, can you sign off on this bit?

Comment 33

[Norbert Lindenberg]

(In reply to Gregory Szorc [:gps] from comment #31)

> mh-msys-msvc scares me. I'm willing to rs+ the rest of the files.

I guess this needs more context: ICU already has a pile of intl/icu/source/config/mh-* files for different build environments. The Mozilla build environment, MSYS with MSVC, is similar to Cygwin with MSVC, so this file is derived from mh-cygwin-msvc. It's probably best to review a diff between the two rather than mh-msys-msvc by itself.

The diff is long, but has only three kinds of changes:

- Replace Windows-style /options with Unix-style -options. MSYS tries to be more Unix like, so / is a path separator, not an option indicator.

- Replace use of cygpath, which in Cygwin maps Unix-style paths to Windows-style paths, with nothing because MSYS handles this internally.

- Remove CURR_FULL_DIR and CURR_SRCCODE_FULL_DIR, which aren't used anywhere.

> ::: intl/icu/source/config/mh-msys-msvc
> 
> Can you give this file a .mk extension?

The convention in ICU is not to do that.

> ::: intl/icu/source/data/Makefile.in
> > +        # fixme: need to tell whether to use - or /, $(SOURCEFILE) or $<
> > +	rc.exe -i$(srcdir)/../common -i$(top_builddir)/common -fo$@ $(CPPFLAGS) $<
> 
> Why doesn't rc.exe like Windows style /arguments?

Same as above - MSYS interprets / as a path separator.

Comment 34

[Norbert Lindenberg]

Attachment: Integrate ICU into the Mozilla build

Mike, can you please look at intl/icu/configure, as requested by Gregory in comment 29?

Patch updated per comment 29.

(In reply to Gregory Szorc [:gps] from comment #29)

> ::: intl/moz.build
> >  DIRS += [
> > +    'icu',
> 
> You already reference intl/icu in toolkit.mozbuild. You only need 1
> reference.

That what I was thinking too, but then the build failed when trying to make intl/icu because it hadn't bothered to convert Makefile.in into Makefile first. What's the correct way to get it to do that?

> ::: intl/icu/Makefile.in
> > +ifdef ENABLE_INTL_API
> 
> Why do we need the ifdef? If ICU isn't enabled, we should never traverse
> into this directory and Makefile.

Because of the addition in intl/moz.build it does traverse. A better fix for the above should make this ifdef unnecessary.

> > +		$(GMAKE) -C obj GENRBOPTS='-k -R'
> 
> I can't remember the default behavior for passing -j, -s, etc down to sub
> makes. If we pass -j, then gmake -j > 1 on Windows is buggy and gmake -j < #
> cores is slow everywhere else. Either way, we have a problem to address.

It's all black magic to me, but it seems the ICU build system is doing something right: While building on my Mac, the 4 pseudo-cores are busy to about 80%, while the Windows build on try produces usable output.

Comment 35

[Mike Hommey [:glandium]]

Comment on attachment 731053 [details] [diff] [review]
Integrate ICU into the Mozilla build

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

::: intl/icu/configure
@@ +3,5 @@
> +# 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/.
> +
> +# Default srcdir to this directory
> +SRC_DIR=.

Does that actually work? The current directory when running configure is in the objdir, not the source directory.

@@ +60,5 @@
> +chmod +x $SRC_DIR/source/runConfigureICU
> +if ! test -e obj; then
> +    mkdir obj
> +fi
> +(cd obj; \

O_o

::: toolkit/library/Makefile.in
@@ +364,5 @@
>    $(NSS_LIBS) \
>    $(MOZ_CAIRO_OSLIBS) \
>    $(MOZ_APP_EXTRA_LIBS) \
>    $(SQLITE_LIBS) \
> +  $(ICU_LIBS) \

You're effectively linking ICU twice on windows. Once in mozjs.dll, and once in xul.dll. Although, since nothing uses ICU in xul.dll (right?), it's possible that the linker throws it away, and it goes unnoticed. Until something in xul.dll does use ICU.

Now, why exactly are we making things so complicated by having ICU outside js/src again? Because having ICU under js/src would mean that
- you'd link it in mozjs/static_js and be done with it (with SHARED_LIBRARY_LIBS instead of EXTRA_LIBS)
- xul.dll would still be able to use like it does use the JS API, if it ever needs to use it
- it would avoid the standalone js problem altogether.

Comment 36

[Jeff Walden [:Waldo]]

(In reply to Norbert Lindenberg from comment #32)
> (In reply to Gregory Szorc [:gps] from comment #29)
> 
> > ::: js/src/jsversion.h
> > > -/* ECMAScript Internationalization API isn't fully implemented yet. */
> > > -#define ENABLE_INTL_API 0
> > 
> > I have no clue what this does and don't feel comfortable reviewing it.
> > Shouldn't this be conditional on ENABLE_INTL_ICU?
> 
> We use ENABLE_INTL_API everywhere, since the ECMAScript Internationalization
> API is the feature that all this enables. We used the simple #define in
> jsversion.h while landing all the C++ and JavaScript code, but now that the
> build system is also involved, the AC_DEFINE(ENABLE_INTL_API) in
> configure.in replaces it.
> 
> Jeff, can you sign off on this bit?

Yeah, this should be good.

Comment 37

[Gregory Szorc [:gps]]

Comment on attachment 727470 [details] [diff] [review]
Make ICU build with Mozilla build for Windows

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

Looking at the diff between mh-msys-msvc and the existing script made this a relatively straightforward review.

Do you think it is worth running hg cp on the original file then applying the changes?

This review is 80% rubber stamp. I don't pretend to know what's fully going on in ICU's build system. But, we seem to be following conventions well enough. I look forward to getting these patches upstreamed!

Comment 38

[Gregory Szorc [:gps]]

Comment on attachment 731053 [details] [diff] [review]
Integrate ICU into the Mozilla build

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

This looks mostly good. If the dual linking problem is resolved, I may let other issues slide or defer them to follow-ups and grant this r+ as is. ICU has value and shipping a working feature with sub-par build system integration (for the short term - with the intent of making it work better over time) is better than waiting an extra cycle to ship this. And, if we get everything really wrong, disabling ICU is just a flip of mozconfig away. I'm not too worried about the minor issues.

Linking, however, is not a minor issue. We can't have ICU linked into both libjs and libxul. Fix that and this likely gets r+.

::: intl/icu/Makefile.in
@@ +10,5 @@
> +VPATH           = @srcdir@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +ifdef ENABLE_INTL_API

Let's remove this ifdef and conditionally include the directory from the calling moz.build file. See other comment.

@@ +20,5 @@
> +# libraries on Windows, and builds at least one extra library we don't need.
> +# The foreach expression tries to catch exactly the required files.
> +ifeq ($(OS_ARCH),WINNT)
> +    ICU_LIB_PREFIX=s
> +    ifeq ($(MOZ_DEBUG),1)

ifdef MOZ_DEBUG

@@ +32,5 @@
> +	$(GMAKE) -C obj GENRBOPTS='-k -R'
> +	$(foreach libname,$(ICU_LIB_NAMES),\
> +		cp obj/lib/$(ICU_LIB_PREFIX)$(libname)$(ICU_LIB_SUFFIX).$(LIB_SUFFIX) \
> +		lib/$(LIB_PREFIX)$(libname).$(LIB_SUFFIX);)
> +	$(INSTALL) $(ICU_LIB_BUILDS) $(DIST)/lib

We have automagical make rules for file copying. But, I'm willing to let it slide.

We should also ideally create separate rules for invoking make and for installing the files. The latter should have proper dependencies to avoid unnecessary file copy.

What happens if ICU is a no-op build? Does the file copy bump the mtime and trigger a relink? That would be bad. If so, that would be an r- if it's identified before checkin and likely a mozconfig backout if discovered after checkin.

::: intl/moz.build
@@ +17,3 @@
>      'uconv',
>      'build',
>  ]

if CONFIG['ENABLE_INTL_API']:
    DIRS += ['icu']

Actually, since this directory isn't using any moz.build primitives and has its own configure, this should be EXTERNAL_MAKE_DIRS += ['icu']. Then, you can remove the moz.build file from intl/icu/.

That being said, I still hold that this shouldn't be needed at all since toolkit.mozbuild pulls it in. If the Makefile didn't exist, then configure wasn't creating it. AC_OUTPUT_SUBDIRS from the main configure should result in the Makefile being generated. If it isn't, there's a bug in ICU's configure script.

::: toolkit/library/Makefile.in
@@ +364,5 @@
>    $(NSS_LIBS) \
>    $(MOZ_CAIRO_OSLIBS) \
>    $(MOZ_APP_EXTRA_LIBS) \
>    $(SQLITE_LIBS) \
> +  $(ICU_LIBS) \

The dual linking is a deal breaker.

Comment 39

[Norbert Lindenberg]

Attachment: Make ICU build with Mozilla build for Windows

Updated per comment 37. Carrying r+gps.

> Do you think it is worth running hg cp on the original file then applying
> the changes?

Yes, absolutely - this makes it a lot clearer what's going on. Sorry I didn't know about this option.

Comment 40

[Till Schneidereit [:till]]

Comment on attachment 731516 [details] [diff] [review]
Make ICU build with Mozilla build for Windows

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c3ee54085e1

Comment 41

[Norbert Lindenberg]

Attachment: Integrate ICU into the Mozilla build

(In reply to Gregory Szorc [:gps] from comment #38)

> This looks mostly good. If the dual linking problem is resolved, I may let
> other issues slide or defer them to follow-ups and grant this r+ as is. ICU
> has value and shipping a working feature with sub-par build system
> integration (for the short term - with the intent of making it work better
> over time) is better than waiting an extra cycle to ship this. And, if we
> get everything really wrong, disabling ICU is just a flip of mozconfig away.
> I'm not too worried about the minor issues.

Thank you very much for your understanding! I really appreciate it.

Major changes in this update:

I reverted to the structure I used before March 26, so ICU is built from js/src without separate configure and Makefile in intl/icu, because:

- This enables the stand-alone SpiderMonkey build, which is very important to the JavaScript team.

- Trying to address Gregory's comments around moz.build files led to various issues because of all the dependencies within the build system (intl/icu/Makefile.in needs INSTALL, therefore requires rules.mk, which in turn requires backend.mk, which requires that intl/icu is listed in DIRS, not EXTERNAL_MAKE_DIRS, which requires a moz.build file - and after all this, the Windows build was still failing mysteriously). It just doesn't seem worth the trouble.

- It gets rid of all the extras within intl/icu that Mike didn't like, and it seems moving ICU into js/src is the general direction you want to see.

The relevant contents of intl/icu/configure moved into intl-api.m4, those of intl/icu/Makefile.in into js/src/Makefile.in.

I fixed the double-linking issue by making the addition of ICU_LIBS for linking XUL platform dependent. On Mac OS X and Linux, adding ICU_LIBS is necessary. On Mac OS X I traced this back to the ar tool simply not including the contents of the ICU static libraries into the libjs_static.a library, even when the are provided via SHARED_LIBRARY_LIBS. I don't know why it doesn't include them, and couldn't find a way to make it do so. On Windows, adding ICU_LIBS is redundant, although currently not harmful (I see no impact on download size).

I also changed the Makefile rules for building and installing the ICU libraries to minimize relinking.

(In reply to Mike Hommey [:glandium] from comment #35)

> Now, why exactly are we making things so complicated by having ICU outside
> js/src again? Because having ICU under js/src would mean that
> - you'd link it in mozjs/static_js and be done with it (with
> SHARED_LIBRARY_LIBS instead of EXTRA_LIBS)
> - xul.dll would still be able to use like it does use the JS API, if it ever
> needs to use it
> - it would avoid the standalone js problem altogether.

Having ICU within js/src would certainly make things easier for anything JavaScript related; it might seem an odd location though when other Mozilla components start using it. We should discuss this under bug 850379.

(In reply to Gregory Szorc [:gps] from comment #38)

> @@ +32,5 @@
> > +	$(GMAKE) -C obj GENRBOPTS='-k -R'
> > +	$(foreach libname,$(ICU_LIB_NAMES),\
> > +		cp obj/lib/$(ICU_LIB_PREFIX)$(libname)$(ICU_LIB_SUFFIX).$(LIB_SUFFIX) \
> > +		lib/$(LIB_PREFIX)$(libname).$(LIB_SUFFIX);)
> > +	$(INSTALL) $(ICU_LIB_BUILDS) $(DIST)/lib
> 
> We have automagical make rules for file copying. But, I'm willing to let it
> slide.

Where can I find them? Searching for cp or copy in the Mozilla sources is hopeless...

Comment 42

[Norbert Lindenberg]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=16f2b806f476

Comment 43

[Mike Hommey [:glandium]]

(In reply to Norbert Lindenberg from comment #41)
> I fixed the double-linking issue by making the addition of ICU_LIBS for
> linking XUL platform dependent. On Mac OS X and Linux, adding ICU_LIBS is
> necessary. On Mac OS X I traced this back to the ar tool simply not
> including the contents of the ICU static libraries into the libjs_static.a
> library, even when the are provided via SHARED_LIBRARY_LIBS.

That's certainly not expected. Can you add --verbose to EXPAND_AR (before --) in config/config.mk and paste the ar command line and output?

> Where can I find them? Searching for cp or copy in the Mozilla sources is hopeless...

Search for INSTALL_TARGETS in config/rules.mk

Comment 44

[Mike Hommey [:glandium]]

Comment on attachment 731695 [details] [diff] [review]
Integrate ICU into the Mozilla build

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

::: configure.in
@@ +4167,5 @@
> +
> +dnl ECMAScript Internationalization API Support (uses ICU)
> +dnl ========================================================
> +
> +MOZ_INTL_API(false)

I don't think that's needed in the top level configure anymore. And I don't think you need the shared intl-api.m4 as a consequence. If we ever add ICU support to non-js, it won't need all of it anyways (it will only need the --enable-intl-api flag).

::: js/src/Makefile.in
@@ +606,5 @@
> +
> +.PHONY: ICU
> +
> +ICU:
> +	$(GMAKE) -C intl/icu GENRBOPTS='-k -R'

I doubt this works, since intl/icu is not under js/src.

@@ +633,5 @@
> +
> +.PRECIOUS: $(call EXPAND_LIBNAME_PATH,%,intl/icu/lib)
> +
> +$(call EXPAND_LIBNAME_PATH,%,$(DIST)/lib) : $(call EXPAND_LIBNAME_PATH,%,intl/icu/lib)
> +	$(INSTALL) $< $(DIST)/lib

You don't need all this. I think you don't need more than:

export::
        $(GMAKE) -C ../../intl/icu GENRBOPTS='-k -R' -j1

distclean clean::
        $(call SUBMAKE,$@,../../intl/icu)

then make ICU_LIBS point directly in intl/icu instead of $(DIST)/lib.

Comment 45

[Norbert Lindenberg]

(In reply to Mike Hommey [:glandium] from comment #44)

> ::: configure.in
> @@ +4167,5 @@
> > +
> > +dnl ECMAScript Internationalization API Support (uses ICU)
> > +dnl ========================================================
> > +
> > +MOZ_INTL_API(false)
> 
> I don't think that's needed in the top level configure anymore. And I don't
> think you need the shared intl-api.m4 as a consequence. If we ever add ICU
> support to non-js, it won't need all of it anyways (it will only need the
> --enable-intl-api flag).

The top level configure also needs the steps leading to the definition of ICU_LIBS, unless we can convince ar to include all the contents of the ICU libraries. Using a separate m4 file was suggested by Gregory in comment 29. The build system requires that m4 files that exist in both build/autoconf and js/src/build/autoconf are identical.

> ::: js/src/Makefile.in
> @@ +606,5 @@
> > +
> > +.PHONY: ICU
> > +
> > +ICU:
> > +	$(GMAKE) -C intl/icu GENRBOPTS='-k -R'
> 
> I doubt this works, since intl/icu is not under js/src.

See comment 42. Since the configure script is run from js/src, the object directory for intl/icu is js/src/intl/icu.

> @@ +633,5 @@
> > +
> > +.PRECIOUS: $(call EXPAND_LIBNAME_PATH,%,intl/icu/lib)
> > +
> > +$(call EXPAND_LIBNAME_PATH,%,$(DIST)/lib) : $(call EXPAND_LIBNAME_PATH,%,intl/icu/lib)
> > +	$(INSTALL) $< $(DIST)/lib
> 
> You don't need all this. I think you don't need more than:
> 
> export::
>         $(GMAKE) -C ../../intl/icu GENRBOPTS='-k -R' -j1
> 
> distclean clean::
>         $(call SUBMAKE,$@,../../intl/icu)
> 
> then make ICU_LIBS point directly in intl/icu instead of $(DIST)/lib.

The INSTALL step is necessary so that the linker finds the libraries in a known place; the ICU step leaves them in intl/icu/lib for the SpiderMonkey build and in js/src/intl/icu/lib for the Firefox build. The difference in ICU build directories is a natural consequence of running configure from js/src; it's also good because ../../intl/icu would be outside the object directory for the SpiderMonkey build, with potentially bad consequences.

The .PRECIOUS is necessary because otherwise gmake deletes the files as intermediate files, and INSTALL only creates a link, which points to the deleted files. I've seen the build fail because of that.

The Windows file renaming is necessary so that the files have the names expected by later steps in the Mozilla build process.

Separating the steps and making sure that accidental changes in file modification dates don't trigger relinking was suggested by Gregory in comment 38.

The only part that may not be necessary is .LOW_RESOLUTION_TIME - I don't know for all operating systems Mozilla builds on whether they have the problem that this special built-in target addresses.

Comment 46

[Mike Hommey [:glandium]]

(In reply to Norbert Lindenberg from comment #45)
> (In reply to Mike Hommey [:glandium] from comment #44)
> 
> > ::: configure.in
> > @@ +4167,5 @@
> > > +
> > > +dnl ECMAScript Internationalization API Support (uses ICU)
> > > +dnl ========================================================
> > > +
> > > +MOZ_INTL_API(false)
> > 
> > I don't think that's needed in the top level configure anymore. And I don't
> > think you need the shared intl-api.m4 as a consequence. If we ever add ICU
> > support to non-js, it won't need all of it anyways (it will only need the
> > --enable-intl-api flag).
> 
> The top level configure also needs the steps leading to the definition of
> ICU_LIBS, unless we can convince ar to include all the contents of the ICU
> libraries.

Thus comment 43.

> Using a separate m4 file was suggested by Gregory in comment 29.
> The build system requires that m4 files that exist in both build/autoconf
> and js/src/build/autoconf are identical.
> 
> > ::: js/src/Makefile.in
> > @@ +606,5 @@
> > > +
> > > +.PHONY: ICU
> > > +
> > > +ICU:
> > > +	$(GMAKE) -C intl/icu GENRBOPTS='-k -R'
> > 
> > I doubt this works, since intl/icu is not under js/src.
> 
> See comment 42.

Comment 42 is a try build url.

> Since the configure script is run from js/src, the object
> directory for intl/icu is js/src/intl/icu.

What would be creating js/src/intl/icu/Makefile.in in the object directory? I see nothing that would. And nothing that would tell that the corresponding source directory is intl/icu.

> > @@ +633,5 @@
> > > +
> > > +.PRECIOUS: $(call EXPAND_LIBNAME_PATH,%,intl/icu/lib)
> > > +
> > > +$(call EXPAND_LIBNAME_PATH,%,$(DIST)/lib) : $(call EXPAND_LIBNAME_PATH,%,intl/icu/lib)
> > > +	$(INSTALL) $< $(DIST)/lib
> > 
> > You don't need all this. I think you don't need more than:
> > 
> > export::
> >         $(GMAKE) -C ../../intl/icu GENRBOPTS='-k -R' -j1
> > 
> > distclean clean::
> >         $(call SUBMAKE,$@,../../intl/icu)
> > 
> > then make ICU_LIBS point directly in intl/icu instead of $(DIST)/lib.
> 
> The INSTALL step is necessary so that the linker finds the libraries in a
> known place; the ICU step leaves them in intl/icu/lib for the SpiderMonkey
> build and in js/src/intl/icu/lib for the Firefox build.

The point is you shouldn't need the libraries for the Firefox build, as they would be in spidermonkey.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c3ee54085e1

Comment 48

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #46)
> What would be creating js/src/intl/icu/Makefile.in in the object directory?
> I see nothing that would. And nothing that would tell that the corresponding
> source directory is intl/icu.

Ah, intl-api.m4.

Comment 49

[Norbert Lindenberg]

(In reply to Mike Hommey [:glandium] from comment #43)
> (In reply to Norbert Lindenberg from comment #41)
> > I fixed the double-linking issue by making the addition of ICU_LIBS for
> > linking XUL platform dependent. On Mac OS X and Linux, adding ICU_LIBS is
> > necessary. On Mac OS X I traced this back to the ar tool simply not
> > including the contents of the ICU static libraries into the libjs_static.a
> > library, even when the are provided via SHARED_LIBRARY_LIBS.
> 
> That's certainly not expected. Can you add --verbose to EXPAND_AR (before
> --) in config/config.mk and paste the ar command line and output?

Using "SHARED_LIBRARY_LIBS += $(ICU_LIBS)" in js/src/Makefile.in I get:

Executing: ar cr libjs_static.a jsalloc.o jsanalyze.o jsapi.o jsarray.o jsatom.o jsbool.o jsclone.o jscntxt.o jscompartment.o jsdate.o jsdbgapi.o jsdhash.o jsdtoa.o jsexn.o jsfriendapi.o jsfun.o jsgc.o jscrashreport.o jsinfer.o jsinterp.o jsiter.o jslog2.o jsmath.o jsmemorymetrics.o jsnativestack.o jsnum.o jsobj.o json.o jsonparser.o jsopcode.o jsproxy.o jsprf.o jsprobes.o jspropertycache.o jspropertytree.o jsreflect.o jsscript.o jsstr.o jstypedarray.o jsutil.o jswatchpoint.o jsweakmap.o jsworkers.o ThreadPool.o Monitor.o ForkJoin.o jswrapper.o prmjtime.o sharkctl.o ArgumentsObject.o DateTime.o Debugger.o GlobalObject.o Object.o ObjectImpl.o PropertyKey.o ScopeObject.o Shape.o Stack.o String.o BytecodeCompiler.o BytecodeEmitter.o CharacterEncoding.o FoldConstants.o Intl.o NameFunctions.o ParallelDo.o ParallelArray.o ParseMaps.o ParseNode.o Parser.o SPSProfiler.o SelfHosting.o TokenStream.o TestingFunctions.o Profilers.o LifoAlloc.o Eval.o MapObject.o RegExpObject.o RegExpStatics.o RegExp.o RootMarking.o Marking.o Memory.o Statistics.o StoreBuffer.o Iteration.o Zone.o Verifier.o StringBuffer.o Unicode.o Xdr.o Module.o MethodJIT.o StubCalls.o Compiler.o FrameState.o FastArithmetic.o FastBuiltins.o FastOps.o LoopState.o StubCompiler.o MonoIC.o PolyIC.o ImmutableSync.o InvokeHelpers.o Retcon.o TrampolineCompiler.o MIR.o BacktrackingAllocator.o Bailouts.o BitSet.o C1Spewer.o CodeGenerator.o CodeGenerator-shared.o Ion.o IonAnalysis.o IonBuilder.o IonCaches.o IonFrames.o IonMacroAssembler.o IonSpewer.o JSONSpewer.o LICM.o LinearScan.o LIR.o LiveRangeAllocator.o Lowering.o Lowering-shared.o MCallOptimize.o MIRGraph.o MoveResolver.o EdgeCaseAnalysis.o RegisterAllocator.o Snapshots.o Safepoints.o StupidAllocator.o TypeOracle.o TypePolicy.o ValueNumbering.o RangeAnalysis.o VMFunctions.o ParallelFunctions.o AliasAnalysis.o ParallelArrayAnalysis.o UnreachableCodeElimination.o EffectiveAddressAnalysis.o AsmJS.o AsmJSLink.o AsmJSSignalHandlers.o CodeGenerator-x86-shared.o IonFrames-x86-shared.o MoveEmitter-x86-shared.o Assembler-x86-shared.o Lowering-x86-shared.o Lowering-x64.o CodeGenerator-x64.o Trampoline-x64.o Assembler-x64.o Bailouts-x64.o MacroAssembler-x64.o ExecutableAllocator.o PageBlock.o YarrInterpreter.o YarrPattern.o YarrSyntaxChecker.o YarrCanonicalizeUCS2.o Logging.o ExecutableAllocatorPosix.o OSAllocatorPosix.o ARMAssembler.o MacroAssemblerARM.o MacroAssemblerX86Common.o YarrJIT.o CTypes.o Library.o jsperf.o pm_stub.o tmpYgW1cE/closures.o tmpYgW1cE/darwin.o tmpYgW1cE/darwin64.o tmpYgW1cE/debug.o tmpYgW1cE/ffi.o tmpYgW1cE/ffi64.o tmpYgW1cE/java_raw_api.o tmpYgW1cE/prep_cif.o tmpYgW1cE/raw_api.o tmpYgW1cE/types.o tmpWqRAfS/icudt50l_dat.o
/usr/bin/ranlib: file: libjs_static.a(jslog2.o) has no symbols
/usr/bin/ranlib: file: libjs_static.a(StoreBuffer.o) has no symbols
/usr/bin/ranlib: file: libjs_static.a(ImmutableSync.o) has no symbols
/usr/bin/ranlib: file: libjs_static.a(ARMAssembler.o) has no symbols
/usr/bin/ranlib: file: libjs_static.a(MacroAssemblerARM.o) has no symbols
/usr/bin/ranlib: file: libjs_static.a(darwin.o) has no symbols
/usr/bin/ranlib: file: libjs_static.a(ffi.o) has no symbols
/usr/bin/ranlib: file: libjs_static.a(java_raw_api.o) has no symbols
/usr/bin/ranlib: file: libjs_static.a(raw_api.o) has no symbols
ranlib: file: libjs_static.a(jslog2.o) has no symbols
ranlib: file: libjs_static.a(StoreBuffer.o) has no symbols
ranlib: file: libjs_static.a(ImmutableSync.o) has no symbols
ranlib: file: libjs_static.a(ARMAssembler.o) has no symbols
ranlib: file: libjs_static.a(MacroAssemblerARM.o) has no symbols
ranlib: file: libjs_static.a(darwin.o) has no symbols
ranlib: file: libjs_static.a(ffi.o) has no symbols
ranlib: file: libjs_static.a(java_raw_api.o) has no symbols
ranlib: file: libjs_static.a(raw_api.o) has no symbols

Using "EXTRA_LIBS += $(ICU_LIBS)" in js/src/Makefile.in, the item tmpWqRAfS/icudt50l_dat.o is missing.

icudt50l_dat.o is not one of the elements of $(ICU_LIBS); it shows up earlier in the build log near the end of building ICU:

pkgdata: ar r -c ../lib/libicudata.a ./out/tmp/icudt50l_dat.o

%(DIST)lib/libicudata.a in turn is one of the elements of $(ICU_LIBS).

One segment (which may be the only one) from icudt50l_dat.o does indeed show up in libjs_static.a.

It looks like the problem is not in ar, but in the tool that's munging its input beforehand, expandlibs_exec.py.

Comment 50

[Mike Hommey [:glandium]]

AFAICT, this is exactly what is supposed to be happening, and I don't see a problem with it. In fact, you're saying that with SHARED_LIBRARY_LIBS += $(ICU_LIBS) you *are* getting ICU in libjs_static.

Comment 51

[Norbert Lindenberg]

I'm getting one segment of ICU in libjs_static. Everything else is missing.

I've filed bug 857450 on the expandlibs_exec.py issue.

Comment 52

[Mike Hommey [:glandium]]

This is a showstopper:
https://bugzilla.mozilla.org/show_bug.cgi?id=857450#c2

Comment 53

[Mike Hommey [:glandium]]

The ICU build system also "conveniently" hides the actual command lines it uses to build object files.

Comment 54

[Norbert Lindenberg]

Attachment: Integrate ICU into the Mozilla build

Updated per comment 44 and bug 857450 comment 4.

Comment 55

[Mike Hommey [:glandium]]

Comment on attachment 733535 [details] [diff] [review]
Integrate ICU into the Mozilla build

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

I didn't do an entirely thorough review here, so I'd like to see next iteration, in case I've let something slip, but this looks good.

::: build/autoconf/intl-api.m4
@@ +10,5 @@
> +dnl If --enable-intl-api, configures the environment to enable the ECMAScript
> +dnl Internationalization API, including settings to build ICU and to compile
> +dnl and link against it.
> +
> +AC_DEFUN([MOZ_INTL_API],

You don't need to put that in a separate file anymore. Just inline this in js/src/configure.in.

@@ +90,5 @@
> +
> +    chmod +x $srcdir/../../intl/icu/source/runConfigureICU
> +    mkdir -p $_objdir/intl/icu
> +    (cd $_objdir/intl/icu; \
> +     CPPFLAGS="$ICU_CPPFLAGS" CFLAGS="$DSO_PIC_CFLAGS" CXXFLAGS="$DSO_PIC_CFLAGS" \

Can you add a comment about the fact that DSO_PIC_CFLAGS are used to force the ICU static library to be position independent code?

::: js/src/Makefile.in
@@ +625,5 @@
> +endif
> +
> +# - Build ICU as part of the "export" target, so things get built
> +#   in the right order.
> +# - ICU requires GNU make.

because pymake doesn't support order only dependencies.

Comment 56

[Gregory Szorc [:gps]]

Comment on attachment 733535 [details] [diff] [review]
Integrate ICU into the Mozilla build

I don't think there's anything I can provide to the review process beyond what Mike Hommey will provide. I don't believe dual review is needed. So, I'm leaving this review in Mike Hommey's more than capable hands.

Comment 57

[Norbert Lindenberg]

Attachment: Integrate ICU into the Mozilla build

Updated per comment 55.

Comment 58

[Mike Hommey [:glandium]]

Comment on attachment 734738 [details] [diff] [review]
Integrate ICU into the Mozilla build

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

Mostly nits, and two issues that need fixing before landing.

::: js/src/Makefile.in
@@ +616,5 @@
> +
> +ifeq ($(OS_ARCH),WINNT)
> +# - Library names: On Windows, ICU uses modified library names for static
> +#   and debug libraries.
> +    ifdef MOZ_DEBUG

2-space indent. I know it doesn't match the rest, but we're slowly settling on 2-space indent in Makefiles.

@@ +631,5 @@
> +# - Force ICU to use the standard suffix for object files because expandlibs
> +#   will discard all files with a non-standard suffix (bug 857450).
> +# - Options for genrb: -k strict parsing; -R omit collation tailoring rules.
> +export::
> +	$(GMAKE) -C intl/icu STATIC_O=$(OBJ_SUFFIX) GENRBOPTS='-k -R'

you want to add a -j1 on this command line on windows.

::: js/src/configure.in
@@ +4444,5 @@
> +    # To reduce library size, use static linking
> +    ICU_LINK_OPTS="--enable-static --disable-shared"
> +    # Force the ICU static libraries to be position independent code
> +    ICU_CFLAGS="$ICU_CFLAGS $DSO_PIC_CFLAGS"
> +    ICU_CXXFLAGS="$ICU_CXXFLAGS $DSO_PIC_CFLAGS"

You could set these directly instead of setting them to an empty value first.

@@ +4454,5 @@
> +    if test -z "$MOZ_OPTIMIZE"; then
> +        ICU_BUILD_OPTS="$ICU_BUILD_OPTS --disable-release"
> +    fi
> +
> +    chmod +x $srcdir/../../intl/icu/source/runConfigureICU

Don't do this chmod, and run $(SHELL) $srcdir/../../intl/icu/source/runConfigureICU instead.

@@ +4458,5 @@
> +    chmod +x $srcdir/../../intl/icu/source/runConfigureICU
> +    mkdir -p $_objdir/intl/icu
> +    (cd $_objdir/intl/icu; \
> +     CFLAGS="$ICU_CFLAGS" CPPFLAGS="$ICU_CPPFLAGS" CXXFLAGS="$ICU_CXXFLAGS" \
> +            $srcdir/../../intl/icu/source/runConfigureICU \

$srcdir might not be absolute, and in such cases, this will fail.

Comment 59

[Norbert Lindenberg]

Attachment: Integrate ICU into the Mozilla build

Updated per comment 58.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 735593 [details] [diff] [review]
Integrate ICU into the Mozilla build

https://hg.mozilla.org/integration/mozilla-inbound/rev/029a6cc8ca0a

Comment 61

[Phil Ringnalda (:philor)]

Comment on attachment 735593 [details] [diff] [review]
Integrate ICU into the Mozilla build

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a18b89e5b3e6 - not only was bug 845190 blocking landing since these patches made it permaorange on Windows, but also the Windows b2g desktop build became permared with the unclear failure in https://tbpl.mozilla.org/php/getParsedLog.php?id=21778030&tree=Mozilla-Inbound

Comment 62

[Phil Ringnalda (:philor)]

Looks like the b2g Windows desktop bustage was something else unrelated.

Comment 63

[Jeff Walden [:Waldo]]

For what it's worth, I plan to look into bug 845190 on Monday.  Not that I think I have any particular expertise in diagnosing the issue -- if someone thinks they do they should absolutely parallelize on this -- but I probably have more Mozilla knowledge to bring to investigating it than Norbert does.

Comment 64

[Jeff Walden [:Waldo]]

Didn't get to looking into it on Monday, but others apparently did and made some progress.  Also word is the failure's gone such that it doesn't hold up anything here, which is more good news.

Comment 65

[Norbert Lindenberg]

Comment on attachment 735593 [details] [diff] [review]
Integrate ICU into the Mozilla build

This patch can be landed - the only one that really depends on getting those random test failures out of the way is bug 853301, which enables the internationalization API.

Comment 66

[Jeff Walden [:Waldo]]

Comment on attachment 735593 [details] [diff] [review]
Integrate ICU into the Mozilla build

https://hg.mozilla.org/integration/mozilla-inbound/rev/944a223513c5

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/944a223513c5