899722 - Bug 853301 broke non-tier1 platforms | enable ECMAScript intl API on BSDs too

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[Landry Breuil (:gaston)]

As of now configure fails with:

configure: error: ECMAScript Internationalization API is not yet supported on this platform

cf http://buildbot.rhaalovely.net/builders/mozilla-central-i386/builds/377/steps/configure/logs/stdio

I know it can be disabled via a configure arg probably, but can we have sane defaults for everyone instead of an error message on poor third-world oses ?

Comment 1

[Landry Breuil (:gaston)]

The fix is probably easy, enabling it on *BSD should just be a matter of adding OS names to js/src/configure.in line 4281.

Comment 2

[Landry Breuil (:gaston)]

Ok, not so straightforward, since it also needs support to run icu's configure which sets ICU_TARGET depending on the os, which also bails out on *BSD.

So, either make --disable-intl-api default for non-tier1 platforms in configure.in :

ie change
if test "$MOZ_BUILD_APP" != "browser"; then
for
if test "$MOZ_BUILD_APP" != "browser" || OS_TARGET not in WINNT Linux Darwin; then

or fiddle with icu build options in js/src/configure.in.

Nice timing for landing this & breaking us just a few days before an uplift :(

Comment 3

[Landry Breuil (:gaston)]

intl/icu/source/runConfigureICU claims support for FreeBSD but does nothing for it (no case in the switch) and probably bails out early, and has no support for the other BSDs.

Guess it'll have to go the --disable-intl-api route. Of course ICU works fine on all BSDs, we have ports for it but bug 851992 wasnt commited yet... and the build/configure system looks not-so-standard.

Comment 4

[Landry Breuil (:gaston)]

Attachment: Enable ecmascript intl api on BSDs

Missed the *BSD case in runconfigureICU.. with that patch, configure passes on OpenBSD. I have to resort to the "SomeBSD" hack for dragonfly, otherwise i'd just pass OS_TARGET.

Will do a build (to ensure the bundled icu actually builds) before asking for review, if this is the way to go ?

Comment 5

[Jeff Walden [:Waldo]]

If that patch works, that's totally cool -- just as long as we're not breaking the tier1 platforms, and ideally not unduly disturbing how they do stuff (and this clearly doesn't).

We'll need to futz with this to support bug 864843 and bug 866301, which will break this, but I'll poke you when I start working on that so we can roughly coordinate or so.  I'm focusing on desktop for now; those others are much secondary.

Comment 6

[Landry Breuil (:gaston)]

Made some progress but it doesnt work yet :

- intl/icu/source/tools/genrb/derb.c has a conflicting 'truncate' declaration which tries to override the one from unistd.h : int        truncate(const char *, off_t);
I've renamed the boolean to 'dotruncate', dunno if that's acceptable.
- for some reason icu builds ./intl/icu/lib/libsicui18n.a instead of libicui18n.a (note the extra s) and linking libjs_static.a fails, it cant find the 3 icu libs. I'll have to look deeper into it.

Comment 7

[Norbert Lindenberg]

(In reply to Landry Breuil (:gaston) from comment #6)

> - for some reason icu builds ./intl/icu/lib/libsicui18n.a instead of
> libicui18n.a (note the extra s) and linking libjs_static.a fails, it cant
> find the 3 icu libs. I'll have to look deeper into it.

I had a similar problem on Windows; fixed by ICU_LIB_RENAME in js/src/Makefile.in.

Comment 8

[Landry Breuil (:gaston)]

Are you sure it's the same ? I dont see anything that could cause that added 's' between the prefix and the actual lib name. The ICU_LIB_RENAME thing seems to be here to remove an extra 'd' after the lib name..

/usr/obj/m-c/js/src/config.status:    (''' LIB_PREFIX ''', r''' lib '''),
/usr/obj/m-c/js/src/config.status:    (''' ICU_LIB_NAMES ''', r''' icui18n icuuc icudata '''),
/usr/obj/m-c/js/src/config.status:    (''' ICU_LIBS ''', r''' $(call EXPAND_LIBNAME_PATH,$(ICU_LIB_NAMES),$(DEPTH)/intl/icu/lib) '''),

i doubt it'd be EXPAND_LIBNAME_PATH, since it's used everywhere....

Comment 9

[Norbert Lindenberg]

I said "similar", not "same": The ICU build system is creating files with extra characters that the Mozilla build system doesn't expect. ICU_LIB_RENAME is my hack to get rid of such characters. The ICU_LIB_RENAME for BSD apparently would have to look different from the one for Windows, but the idea may work.

Comment 10

[Jan Beich]

Why not borrow from mh-linux ?

  # Remove shared library 's'
  STATIC_PREFIX_WHEN_USED =
  STATIC_PREFIX =

Comment 11

[Landry Breuil (:gaston)]

oh, good catch. Will try that.

Comment 12

[Landry Breuil (:gaston)]

looking at config/mh-bsd-gcc, this file looks unmaintained upstream, not updated since 2009. Should we copy mh-linux over it, cherrypick lines from it (ie i'd comment the -soname line like in mh-linux), or use directly mh-linux ?

Comment 13

[Landry Breuil (:gaston)]

Attachment: Enable ecmascript intl api on BSDs

That version builds the correct libicu* libs, and js shell links against it. Now, to debate :
- is s/truncate/dotruncate/ acceptable ?
- how are patches against icu handled ? directly in the tree, or saved as separate patches to be reapplied after each update from upstream (like skia, harfbuzz, cairo, etc..)
- does the mh-bsd-gcc change looks sane enough, or should i copy over mh-linux ?

Comment 14

[Norbert Lindenberg]

Comment on attachment 784265 [details] [diff] [review]
Enable ecmascript intl api on BSDs

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

I'm afraid we need separate patches per file, because each one will go a separate path.

For the build system changes, you'll want a reviewer who actually understands build systems - Gregory Szorc and Mike Hommey have reviewed similar changes for me.

For the changes in mh-bsd-gcc, once reviewed, you should file a ticket against ICU:
http://bugs.icu-project.org/trac/newticket
Include the patch in the bug report - this should let the ICU team accept it without legal paperwork. Make sure you CC yourself (ICU trac strangely doesn't do that automatically), and please CC "norbert" as well.

To track our patches to ICU, I've been adding warnings to the update script, intl/update-icu.sh. Your warning for mh-bsd-gcc will be similar to the first one, the warning for derb.c similar to the second one. Yes, that's patch #4...

To what extent have you tested on the four platforms you're adding? A minimal test is to run the conformance test suite for the internationalization API:
http://test262.ecmascript.org/testcases_intl402.html
There are two known test failures - see bug 853704.

::: intl/icu/source/config/mh-bsd-gcc
@@ +62,5 @@
>  	$(RM) $@ && ln -s ${<F} $@
>  %.$(SO): %.$(SO).$(SO_TARGET_VERSION_MAJOR)
>  	$(RM) $@ && ln -s ${*F}.$(SO).$(SO_TARGET_VERSION) $@
>  
> +## End BSD-specific setup

Should be at the end of the file.

@@ +74,5 @@
> +## Remove shared library 's'
> +STATIC_PREFIX_WHEN_USED = 
> +STATIC_PREFIX = 
> +
> +## End Linux-specific setup

Remove the Linux comment - BSD isn't part of the Linux family.

::: intl/icu/source/tools/genrb/derb.c
@@ +53,5 @@
>  static UConverter *defaultConverter = 0;
>  
>  static const int32_t indentsize = 4;
>  static int32_t truncsize = DERB_DEFAULT_TRUNC;
> +static UBool dotruncate = FALSE;

I found this bug is already fixed in the ICU repository; they used opt_truncate for the variable name, so we should do the same.
http://bugs.icu-project.org/trac/changeset?reponame=&new=32937%40icu%2Ftrunk%2Fsource%2Ftools%2Fgenrb%2Fderb.c&old=32179%40icu%2Ftrunk%2Fsource%2Ftools%2Fgenrb%2Fderb.c
http://bugs.icu-project.org/trac/ticket/9786

Comment 15

[Jeff Walden [:Waldo]]

If you're just directly importing fixes from ICU upstream, I think we can live with a warning.  If you're doing an entirely distinct patch that's not upstream yet, I think we need to make our ICU update script apply a patch, as a post-step, that makes the changes being made.  (Although, the latter is definitely best avoided, and maybe we can even get away without it entirely, but I don't think we know that yet.)

Comment 16

[Landry Breuil (:gaston)]

(In reply to Norbert Lindenberg from comment #14)

> To what extent have you tested on the four platforms you're adding? A
> minimal test is to run the conformance test suite for the
> internationalization API:
> http://test262.ecmascript.org/testcases_intl402.html
> There are two known test failures - see bug 853704.

Same failures on OpenBSD/amd64, all tests passing but 10.1.1_13 & 10.1.1_19_c.

I cant test for the others, but as the build is broken there too, and i've been usually 'in charge of fixing all the BSDs altogether', i'm pretty sure the results will be the same. Jan, care to test FreeBSD ?

Comment 17

[Landry Breuil (:gaston)]

Attachment: Part 1 : mh-bsd-gcc fixes to strip extra 's' in lib name

Comment 18

[Landry Breuil (:gaston)]

Attachment: Part 2 : backport upstream r32937

Comment 19

[Landry Breuil (:gaston)]

Attachment: Part 3 : build system goo to properly enable ecmascript intl api on *bsd

Just for the record: part 1 reported upstream as http://bugs.icu-project.org/trac/ticket/10290

Comment 20

[Norbert Lindenberg]

Comment on attachment 785367 [details] [diff] [review]
Part 2 : backport upstream r32937

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

The changes look good. It would be better, however, to have them in two separate patches in case the fix to derb.c actually needs to be reapplied - the change to update-icu.sh would fail in that case. Also, mention ICU bug 9786 in the patch description to make it easy to figure out which patch to reapply.

Comment 21

[Norbert Lindenberg]

Comment on attachment 785366 [details] [diff] [review]
Part 1 : mh-bsd-gcc fixes to strip extra 's' in lib name

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

The change to the ICU build system should be reviewed by Mike; for update-icu.sh, Waldo had higher expectations.

Comment 22

[Landry Breuil (:gaston)]

Meanwhile, the build is still broken on all BSDs, and the next uplift is coming..

Comment 23

[Jan Beich]

(In reply to Landry Breuil (:gaston) from comment #16)
> I cant test for the others, but as the build is broken there too, and i've
> been usually 'in charge of fixing all the BSDs altogether', i'm pretty sure
> the results will be the same. Jan, care to test FreeBSD ?

I have two issues:

CC/CXX are ignored by runConfigureICU. The build would fail if gcc binary
either doesn't exist or not recent enough to build ICU. A few platforms
are exempted from such bitrot, e.g. Linux and OS X. Bug 869659 may or may
not be related. To avoid patching yet another file we can jump on
ICU_TARGET=Linux bandwagon.

And there's a STATIC_O issue

js/src/builtin/Intl.cpp:(.text._ZN2js19intl_CompareStringsEP9JSContextjPN2JS5ValueE+0x200): undefined reference to `ucol_strcoll_50'
...
/usr/local/bin/ld: js: hidden symbol `ucol_strcoll_50' isn't defined
/usr/local/bin/ld: final link failed: Bad value
c++: error: linker command failed with exit code 1 (use -v to see invocation)
gmake[1]: *** [js] Error 1
gmake[1]: Leaving directory `js/src/shell'

where the build succeeds on the 2nd gmake run. A quick search reveals:

  # 1st run
  intl/icu/lib/libicui18n.a:ucol.ao

  # 2nd run
  intl/icu/lib/libicui18n.a:ucol.ao
  intl/icu/lib/libicui18n.a:ucol.o
  libjs_static.a:ucol.o

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 785366 [details] [diff] [review]
Part 1 : mh-bsd-gcc fixes to strip extra 's' in lib name

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

::: intl/update-icu.sh
@@ +29,5 @@
> +# build on BSD. Check http://bugs.icu-project.org/trac/ticket/10290
> +# whether this has been addressed in the version you're updating to.
> +# If not, obtain the patch from https://bugzilla.mozilla.org/show_bug.cgi?id=899722
> +# and reapply it after running update-icu.sh (additional updates may be needed).
> +# If the bug has been addressed, please delete this warning.

It would just be better to do like with other third party code we have in the tree, and put the patch in the tree, and make update-icu apply all patches in a given directory.

Comment 25

[Landry Breuil (:gaston)]

(In reply to Jan Beich from comment #23)
> (In reply to Landry Breuil (:gaston) from comment #16)
> > I cant test for the others, but as the build is broken there too, and i've
> > been usually 'in charge of fixing all the BSDs altogether', i'm pretty sure
> > the results will be the same. Jan, care to test FreeBSD ?
> 
> I have two issues:
> 
> CC/CXX are ignored by runConfigureICU. The build would fail if gcc binary
> either doesn't exist or not recent enough to build ICU. A few platforms
> are exempted from such bitrot, e.g. Linux and OS X. Bug 869659 may or may
> not be related. To avoid patching yet another file we can jump on
> ICU_TARGET=Linux bandwagon.

Yuck. Oh well, since mh-bsd-gcc isnt really maintained.. does using ICU_TARGET=Linux fixes it ? And if on the long term runConfigureICU will be dropped in favor of directly running configure (yay, wrappers on top of wrappers!)

> And there's a STATIC_O issue

Can you fix that ? I dont understand the issue you're seeing.

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

> It would just be better to do like with other third party code we have in
> the tree, and put the patch in the tree, and make update-icu apply all
> patches in a given directory.

*cries* NOOOES. Oh well. modeled after which update-foo.sh script ? gfx/skia has nothing to apply patches for example (and the README in patches/ is outdated) js/src/ctypes/libffi has nothing either it seems. Like gfx/ycbcr ? Make a new intl/icu/patches dir, and run patch -p1 from hg patches in there ?

And why am i always the one doing that fscking tedious work ? :p

Comment 26

[Landry Breuil (:gaston)]

Attachment: Part 0 : seperate patches for bugs 724533 & 853701 into separate patches and make update-icu.sh apply them

So lets do it this way, extracting the first already applied patches, and removing the blurbs from update-icu.sh. patch -d -p1 < foo will stash them all!

Comment 27

[Landry Breuil (:gaston)]

Attachment: Part 1 : mh-bsd-gcc fixes to strip extra 's' in lib name

Add it as a separate patch too, and make update-icu.sh reapply it.

Comment 28

[Landry Breuil (:gaston)]

Attachment: Part 2 : backport upstream r32937

Same patch as att 785367, but add it as a separate patch too, and make update-icu.sh apply it. Carrying forward norbert's r+, but reasking waldo for confirmation.

Tested locally without issues on OpenBSD with intl api enabled in configure.in, js shell builds & all tests pass.

Comment 29

[Jeff Walden [:Waldo]]

Comment on attachment 788706 [details] [diff] [review]
Part 2 : backport upstream r32937

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

::: intl/icu/patches/bug-899722-2
@@ +1,1 @@
> +Bug 899722 Part 2 - Backport upstream r32937 to fix symbol collision w/ truncate on BSD r=norbert

Might be worth a link to the ticket to save someone the time of tracking it down if they wanted it, but it's not end-of-the-world important.

Comment 30

[Mike Hommey [:glandium]]

Comment on attachment 788704 [details] [diff] [review]
Part 0 : seperate patches for bugs 724533 & 853701 into separate patches and make update-icu.sh apply them

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

::: intl/update-icu.sh
@@ +40,5 @@
>  
>  hg addremove ${icu_dir}
> +
> +patch -d ${icu_dir}/source -p1 < ${icu_dir}/patches/bug-724533
> +patch -d ${icu_dir}/source -p1 < ${icu_dir}/patches/bug-853706

Considering the script starts by removing intl/icu, the patches are gone when you get here, and this doesn't work. You may want to put them under intl/icu-patches.

Also, the patches are relative to topsrcdir, so -p1 doesn't work.

Finally, hg addremove needs to happen after the patching.

Comment 31

[Landry Breuil (:gaston)]

Attachment: Part 0 : seperate patches for bugs 724533 & 853701 into separate patches and make update-icu.sh apply them

okay, lets retry.. put the patches in intl/icu-patches, and apply them before running hg addremove. I didnt use patch -d since i assumed intl/update-icu.sh would be run from topsrcdir (but it could be -d ${icu_dir}/../..) , and -p1 is needed since the patches contain the a/ b/ dirs from hg git.

The two other patches on top of this one adding the two other patches will of course be changed accordingly.

Comment 32

[Mike Hommey [:glandium]]

Comment on attachment 790118 [details] [diff] [review]
Part 0 : seperate patches for bugs 724533 & 853701 into separate patches and make update-icu.sh apply them

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

::: intl/update-icu.sh
@@ +37,5 @@
>  
>  # Record `svn info`
>  svn info $1 > ${icu_dir}/SVN-INFO
>  
> +patch -p1 < ${icu_dir}/../icu-patches/bug-724533

It would be better to use -d.

Comment 33

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d3fa47e4dcb
https://hg.mozilla.org/integration/mozilla-inbound/rev/93c54132c3cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/02017a95970f
https://hg.mozilla.org/integration/mozilla-inbound/rev/65822b58fa9b

With those patches, intl api works on OpenBSD. Putting [leave open] in the whiteboard so  other BSDs can pile on potentials fixes.

Comment 34

[Landry Breuil (:gaston)]

Just for the record : used -d ${icu_dir}/../../ as per comment 32, and added a link to the upstream icu bz as per comment 29

Comment 35

[Jan Beich]

Attachment: Part 4 : inherit CC/CXX like on Linux

Comment 36

[Jan Beich]

Attachment: Part 5 : clobber workaround (Try run required)

Comment 37

[Jan Beich]

Attachment: make icu (after clobber)

Comment 38

[Jan Beich]

Attachment: make icu (normal)

Comment 39

[Mike Hommey [:glandium]]

Comment on attachment 790538 [details] [diff] [review]
Part 5 : clobber workaround (Try run required)

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

::: js/src/Makefile.in
@@ +257,5 @@
>  #   will discard all files with a non-standard suffix (bug 857450).
>  # - Options for genrb: -k strict parsing; -R omit collation tailoring rules.
>  export::
> +	echo "STATIC_O=$(OBJ_SUFFIX)" > intl/icu/icudefs.local
> +	$(GMAKE) $(ICU_GMAKE_OPTIONS) -C intl/icu GENRBOPTS='-k -R'

What is this trying to solve?

Comment 40

[Jan Beich]

(In reply to Mike Hommey [:glandium] from comment #39)
> What is this trying to solve?

#2 issue in comment 23. I've provided the logs in hope
you can reproduce or give advice to a different fix.

  $ diff icu_clobber.log icu_normal.log

Comment 41

[Mike Hommey [:glandium]]

(In reply to Jan Beich from comment #40)
> (In reply to Mike Hommey [:glandium] from comment #39)
> > What is this trying to solve?
> 
> #2 issue in comment 23. I've provided the logs in hope
> you can reproduce or give advice to a different fix.
> 
>   $ diff icu_clobber.log icu_normal.log

Starting from a fresh m-c clone, and with an inexistent objdir, how do you reproduce?

Comment 42

[Jan Beich]

(In reply to Mike Hommey [:glandium] from comment #41)
> Starting from a fresh m-c clone, and with an inexistent objdir, how do you
> reproduce?

The logs were captured after interrupting the build and doing

  $ cd $objdir/js/src; gmake clean -C intl/icu
  $ gmake -C intl/icu STATIC_O=o GENRBOPTS='-k -R' # icu_clobber.log
  ^C
  $ ls -1 intl/icu/**/*.o
  zsh: no matches found: intl/icu/**/*.o
  $ ls -1 intl/icu/**/*.ao
  ...
  intl/icu/stubdata/stubdata.ao
  $ gmake -C intl/icu STATIC_O=o GENRBOPTS='-k -R' # icu_normal.log
  ^C
  $ ls -1 intl/icu/**/*.o
  ...
  intl/icu/stubdata/stubdata.o

Comment 43

[Mike Hommey [:glandium]]

I can't reproduce, and i see no reason for this behaviour.

Comment 44

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9d3fa47e4dcb
https://hg.mozilla.org/mozilla-central/rev/93c54132c3cb
https://hg.mozilla.org/mozilla-central/rev/02017a95970f
https://hg.mozilla.org/mozilla-central/rev/65822b58fa9b

Comment 45

[Jan Beich]

Comment on attachment 790538 [details] [diff] [review]
Part 5 : clobber workaround (Try run required)

I've fixed
http://buildbot.rhaalovely.net/builders/mozilla-central-freebsd-amd64/builds/66/steps/build/logs/stdio
with
http://www.openbsd.org/cgi-bin/cvsweb/~checkout~/ports/devel/gmake/patches/patch-main_c?rev=1.1;content-type=text%2Fplain

Comment 46

[Jan Beich]

part 4 is yet to land

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16eed749e8ab

Comment 48

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/16eed749e8ab

Comment 49

[Landry Breuil (:gaston)]

Interestingly now my builds fail with :

clang++ -fPIC -W -Wall -pedantic -Wpointer-arith -Wwrite-strings -Wno-long-long -std=c++11   -o ../../bin/makeconv makeconv.o ucnvstat.o ge
nmbcs.o gencnvex.o -L../../lib -licutu -L../../lib -licui18n -L../../lib -licuuc -L../../stubdata -licudata -lpthread -lm  
local symbol 0: discarded in section `.text._ZNKSt9type_infoeqERKS_' from ../../lib/libicui18n.a(calendar.o)
local symbol 1: discarded in section `.text._ZNK6icu_508Calendar7getTimeER10UErrorCode' from ../../lib/libicui18n.a(calendar.o)
local symbol 2: discarded in section `.text._ZN6icu_508Calendar7setTimeEdR10UErrorCode' from ../../lib/libicui18n.a(calendar.o)
.....
local symbol 1598: discarded in section `.text._ZNK6icu_507UVector4sizeEv' from ../../lib/libicuuc.a(usetiter.o)
local symbol 1599: discarded in section `.text._ZN6icu_5013UnicodeString5setToEi' from ../../lib/libicuuc.a(usetiter.o)
local symbol 1600: discarded in section `.text._ZNK6icu_5013UnicodeString6lengthEv' from ../../lib/libicuuc.a(usetiter.o)
clang-3.3: error: linker command failed with exit code 1 (use -v to see invocation)

Puzzling..

Comment 50

[Landry Breuil (:gaston)]

Just FTR, part1/attachment 788705 [details] [diff] [review] has been merged upstream in http://bugs.icu-project.org/trac/changeset/34247/icu/trunk/source/config/mh-bsd-gcc

Comment 51

[Gordon P. Hemsley [:GPHemsley]]

Mass-moving existing Intl-related bugs to the new Core :: JavaScript: Internationalization API component.

If you think this bug has been moved in error, feel free to move it back to Core :: JavaScript Engine.

[Mass change filter: core-js-intl-api-move]