Closed Bug 899722 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript: Internationalization API, defect)

x86_64
OpenBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: gaston, Assigned: gaston)

References

Details

Attachments

(7 files, 6 obsolete files)

1.52 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.40 KB, patch
glandium
: review+
Details | Diff | Splinter Review
9.43 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
19.95 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.40 KB, patch
glandium
: review+
Details | Diff | Splinter Review
33.04 KB, text/plain
Details
49.50 KB, text/plain
Details
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 ?
Depends on: 853301
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.
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 :(
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.
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 ?
Assignee: general → landry
Summary: Bug 853301 broke non-tier1 platforms → Bug 853301 broke non-tier1 platforms | enable ECMAScript intl API on BSDs too
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.
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.
(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.
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....
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.
Why not borrow from mh-linux ?

  # Remove shared library 's'
  STATIC_PREFIX_WHEN_USED =
  STATIC_PREFIX =
oh, good catch. Will try that.
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 ?
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 ?
Attachment #783331 - Attachment is obsolete: true
Attachment #784265 - Flags: review?(mozillabugs)
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
Attachment #784265 - Flags: review?(mozillabugs) → feedback+
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.)
(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 ?
Attachment #784265 - Attachment is obsolete: true
Attachment #785366 - Flags: review?(mozillabugs)
Attachment #785367 - Flags: review?(mozillabugs)
Just for the record: part 1 reported upstream as http://bugs.icu-project.org/trac/ticket/10290
Attachment #785368 - Flags: review?(mh+mozilla)
Attachment #785368 - Flags: review?(mh+mozilla) → review+
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.
Attachment #785367 - Flags: review?(mozillabugs) → review+
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.
Attachment #785366 - Flags: review?(mozillabugs)
Attachment #785366 - Flags: review?(mh+mozilla)
Attachment #785366 - Flags: review?(jwalden+bmo)
Meanwhile, the build is still broken on all BSDs, and the next uplift is coming..
(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 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.
Attachment #785366 - Flags: review?(mh+mozilla)
Attachment #785366 - Flags: review?(jwalden+bmo)
Attachment #785366 - Flags: review-
(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
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!
Attachment #788704 - Flags: review?(mh+mozilla)
Add it as a separate patch too, and make update-icu.sh reapply it.
Attachment #785366 - Attachment is obsolete: true
Attachment #788705 - Flags: review?(mh+mozilla)
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.
Attachment #785367 - Attachment is obsolete: true
Attachment #788706 - Flags: review?(jwalden+bmo)
Attachment #788705 - Flags: review?(mh+mozilla) → review+
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.
Attachment #788706 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #788704 - Flags: review?(mh+mozilla) → review-
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.
Attachment #788704 - Attachment is obsolete: true
Attachment #790118 - Flags: review?(mh+mozilla)
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.
Attachment #790118 - Flags: review?(mh+mozilla) → review+
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
Attachment #790417 - Flags: review?(mh+mozilla)
Attachment #790417 - Flags: review?(mh+mozilla) → review+
Attachment #790538 - Flags: review?(mh+mozilla)
Attached file make icu (normal)
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?
Attachment #790538 - Flags: review?(mh+mozilla)
(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
(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?
(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
I can't reproduce, and i see no reason for this behaviour.
part 4 is yet to land
Keywords: checkin-needed
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/16eed749e8ab
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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..
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]
Component: JavaScript Engine → JavaScript: Internationalization API
You need to log in before you can comment on or make changes to this bug.