Note: There are a few cases of duplicates in user autocompletion which are being worked on.

integrate ICU into the mozilla build

RESOLVED FIXED in mozilla23

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: jfkthame, Assigned: Norbert Lindenberg)

Tracking

(Blocks: 4 bugs)

Trunk
mozilla23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 12 obsolete attachments)

2.05 KB, patch
glandium
: review+
Details | Diff | Splinter Review
16.61 KB, patch
Norbert Lindenberg
: review+
till
: checkin+
Details | Diff | Splinter Review
7.83 KB, patch
glandium
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
No longer blocks: 724531
Depends on: 724531
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?
Blocks: 632977
(Assignee)

Updated

5 years ago
Assignee: nobody → mozillabugs
(Assignee)

Updated

5 years ago
Blocks: 769871
(Assignee)

Updated

5 years ago
Blocks: 769872
Blocks: 805081
(Assignee)

Comment 2

5 years ago
Created attachment 699990 [details] [diff] [review]
Make ICU build with Mozilla build for Windows (WIP)
(Assignee)

Comment 3

5 years ago
Created attachment 699991 [details] [diff] [review]
Integrate ICU into the Mozilla build (WIP)
(Assignee)

Updated

5 years ago
Blocks: 837963
(Assignee)

Updated

5 years ago
No longer blocks: 769872
(Assignee)

Comment 4

5 years ago
Created attachment 712051 [details] [diff] [review]
Make ICU build with Mozilla build for Windows (WIP)
Attachment #699990 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 712056 [details] [diff] [review]
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.
Attachment #699991 - Attachment is obsolete: true
Attachment #712056 - Flags: feedback?(ted)
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.
Attachment #712056 - Flags: feedback?(ted) → feedback-
(Assignee)

Comment 7

5 years ago
Created attachment 718607 [details] [diff] [review]
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.
Attachment #718607 - Flags: review?(ted)
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
(Assignee)

Updated

5 years ago
Blocks: 837957
(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

4 years ago
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)
And the mix of pymake/make is usually another kind of world of hurt.
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.
(Assignee)

Comment 12

4 years ago
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.
(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.
(Assignee)

Comment 14

4 years ago
Created attachment 722065 [details] [diff] [review]
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.
Attachment #718607 - Attachment is obsolete: true
Attachment #718607 - Flags: review?(ted)
Attachment #722065 - Flags: review?(mh+mozilla)
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?
Attachment #722065 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 16

4 years ago
Created attachment 723337 [details] [diff] [review]
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.
Attachment #722065 - Attachment is obsolete: true
Attachment #723337 - Flags: review?(mh+mozilla)
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.
Attachment #723337 - Flags: review?(mh+mozilla) → review+
(Assignee)

Updated

4 years ago
Attachment #723337 - Flags: checkin?(jwalden+bmo)
(Assignee)

Comment 18

4 years ago
(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.
(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*.
Blocks: 850379
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 on attachment 723337 [details] [diff] [review]
Make ICU headers available to SpiderMonkey build

https://hg.mozilla.org/integration/mozilla-inbound/rev/082bdc8fcc2a
Attachment #723337 - Flags: checkin?(jwalden+bmo)
https://hg.mozilla.org/mozilla-central/rev/082bdc8fcc2a
(Assignee)

Updated

4 years ago
Blocks: 851992
(Assignee)

Comment 23

4 years ago
(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.
(Assignee)

Comment 24

4 years ago
Created attachment 727470 [details] [diff] [review]
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
Attachment #712051 - Attachment is obsolete: true
Attachment #727470 - Flags: review?(ted)
(Assignee)

Updated

4 years ago
Blocks: 853301
(Assignee)

Comment 25

4 years ago
(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.
(Assignee)

Comment 26

4 years ago
Created attachment 729629 [details] [diff] [review]
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
Attachment #712056 - Attachment is obsolete: true
Attachment #729629 - Flags: review?(ted)
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.
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.

Updated

4 years ago
Attachment #729629 - Flags: review?(ted) → review?(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?
Attachment #729629 - Flags: review?(gps) → feedback+
(Assignee)

Comment 30

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

Comment 32

4 years ago
(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?
(Assignee)

Comment 33

4 years ago
(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.
(Assignee)

Comment 34

4 years ago
Created attachment 731053 [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?

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.
Attachment #729629 - Attachment is obsolete: true
Attachment #729629 - Flags: review?(mh+mozilla)
Attachment #731053 - Flags: review?(mh+mozilla)
Attachment #731053 - Flags: review?(gps)
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.
Attachment #731053 - Flags: review?(mh+mozilla) → review-
(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 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!
Attachment #727470 - Flags: review?(ted) → review+
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.
Attachment #731053 - Flags: review?(gps)
(Assignee)

Comment 39

4 years ago
Created attachment 731516 [details] [diff] [review]
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.
Attachment #727470 - Attachment is obsolete: true
Attachment #731516 - Flags: review+
Attachment #731516 - Flags: checkin?(tschneidereit)
Comment on attachment 731516 [details] [diff] [review]
Make ICU build with Mozilla build for Windows

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c3ee54085e1
Attachment #731516 - Flags: checkin?(tschneidereit) → checkin+
(Assignee)

Comment 41

4 years ago
Created attachment 731695 [details] [diff] [review]
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...
Attachment #731053 - Attachment is obsolete: true
Attachment #731695 - Flags: review?(mh+mozilla)
Attachment #731695 - Flags: review?(gps)
(Assignee)

Comment 42

4 years ago
Try run:
https://tbpl.mozilla.org/?tree=Try&rev=16f2b806f476
(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 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.
Attachment #731695 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 45

4 years ago
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/4c3ee54085e1
(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.
(Assignee)

Comment 49

4 years ago
(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.
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.
(Assignee)

Updated

4 years ago
Depends on: 857450
(Assignee)

Comment 51

4 years ago
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.
This is a showstopper:
https://bugzilla.mozilla.org/show_bug.cgi?id=857450#c2
The ICU build system also "conveniently" hides the actual command lines it uses to build object files.
(Assignee)

Comment 54

4 years ago
Created attachment 733535 [details] [diff] [review]
Integrate ICU into the Mozilla build

Updated per comment 44 and bug 857450 comment 4.
Attachment #731695 - Attachment is obsolete: true
Attachment #731695 - Flags: review?(gps)
Attachment #733535 - Flags: review?(mh+mozilla)
Attachment #733535 - Flags: review?(gps)
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.
Attachment #733535 - Flags: review?(mh+mozilla) → feedback+
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.
Attachment #733535 - Flags: review?(gps)
(Assignee)

Comment 57

4 years ago
Created attachment 734738 [details] [diff] [review]
Integrate ICU into the Mozilla build

Updated per comment 55.
Attachment #733535 - Attachment is obsolete: true
Attachment #734738 - Flags: review?(mh+mozilla)
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.
Attachment #734738 - Flags: review?(mh+mozilla) → feedback+
(Assignee)

Comment 59

4 years ago
Created attachment 735593 [details] [diff] [review]
Integrate ICU into the Mozilla build

Updated per comment 58.
Attachment #734738 - Attachment is obsolete: true
Attachment #735593 - Flags: review?(mh+mozilla)
Attachment #735593 - Flags: review?(mh+mozilla) → review+
(Assignee)

Updated

4 years ago
Attachment #735593 - Flags: checkin?(jwalden+bmo)
(Assignee)

Updated

4 years ago
Whiteboard: [leave open]
Target Milestone: --- → mozilla23
Comment on attachment 735593 [details] [diff] [review]
Integrate ICU into the Mozilla build

https://hg.mozilla.org/integration/mozilla-inbound/rev/029a6cc8ca0a
Attachment #735593 - Flags: checkin?(jwalden+bmo) → checkin+
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
Attachment #735593 - Flags: checkin+
Looks like the b2g Windows desktop bustage was something else unrelated.
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.
(Assignee)

Updated

4 years ago
Blocks: 864843
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.
(Assignee)

Comment 65

4 years ago
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.
Attachment #735593 - Flags: checkin?(jwalden+bmo)
Comment on attachment 735593 [details] [diff] [review]
Integrate ICU into the Mozilla build

https://hg.mozilla.org/integration/mozilla-inbound/rev/944a223513c5
Attachment #735593 - Flags: checkin?(jwalden+bmo)
(Assignee)

Updated

4 years ago
Blocks: 866301
(Assignee)

Updated

4 years ago
Blocks: 866305
https://hg.mozilla.org/mozilla-central/rev/944a223513c5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Depends on: 853208
Depends on: 869659
Depends on: 905271
Blocks: 1215247
Blocks: 1215252
You need to log in before you can comment on or make changes to this bug.