1080910 - Mac build error, using "ac_add_options --without-intl-api" in mozconfig, with "Undefined symbols for architecture x86_64: "_ucol_close_52", referenced from: nsCollationMacUC::CleanUpCollator()" and similar

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Try to build with this in your .mozconfig:

ac_add_options --enable-debug --disable-optimize
ac_add_options --without-intl-api

ACTUAL RESULTS:
Build error:
{
ld: warning: could not create compact unwind for _ffi_call_unix64: does not use RBP or RSP based frame
Undefined symbols for architecture x86_64:
  "_ucol_close_52", referenced from:
      nsCollationMacUC::CleanUpCollator() in Unified_cpp_intl_locale_mac0.o
  "_ucol_getSortKey_52", referenced from:
      nsCollationMacUC::AllocateRawSortKey(int, nsAString_internal const&, unsigned char**, unsigned int*) in Unified_cpp_intl_locale_mac0.o
  "_ucol_open_52", referenced from:
      nsCollationMacUC::EnsureCollator(int) in Unified_cpp_intl_locale_mac0.o
  "_ucol_setAttribute_52", referenced from:
      nsCollationMacUC::EnsureCollator(int) in Unified_cpp_intl_locale_mac0.o
  "_ucol_strcoll_52", referenced from:
      nsCollationMacUC::CompareString(int, nsAString_internal const&, nsAString_internal const&, int*) in Unified_cpp_intl_locale_mac0.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[5]: *** [XUL] Error 1
make[4]: *** [toolkit/library/target] Error 2
}

I can build successfully if I drop "ac_add_options --without-intl-api" from my mozconfig.

I can also build successfully on linux with this mozconfig option, so this seems to be Mac-only.

Comment 1

[Daniel Holbert [:dholbert]]

The file whose code is mentioned in the build error is:
 http://mxr.mozilla.org/mozilla-central/source/intl/locale/mac/nsCollationMacUC.cpp

Needinfo'ing josh, since it looks like he's reviewed changes to this file most recently, and hopefully he knows who might be able to take a look at this.

Comment 2

[Josh Aas]

CC'd :arai, this is likely a regression from his change.

Comment 3

[Daniel Holbert [:dholbert]]

Agreed -- looks like that patch added some calls to ucol_close etc. (the undefined symbols mentioned in comment 0)

Comment 4

[Tooru Fujisawa [:arai]]

Sorry, I didn't thought of --without-intl-api option.
I guess it wasn't a good idea to share _INTL_API variable to build files under intl/icu/.

Here is my plan to fix this:
  1. revert change to configure.in (setting _INTL_API=yes when MOZ_WIDGET_TOOLKIT==cocoa)
  2. add BUILD_ICU variable in build/autoconf/icu.m4, which is set to 1 when ENABLE_INTL_API==1 or MOZ_WIDGET_TOOLKIT==cocoa
  3. replace some use of ENABLE_INTL_API to BUILD_ICU, to build ICU when BUILD_ICU==1

Is it okay?

I'll post draft patch soon.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Add BUILD_ICU variable separated from ENABLE_INTL_API.

Patch noted in comment #4.

Now testing build locally with --with-intl-api/--with-intl-api=build/without-intl-api options.
I'll push to try server after those tests passed.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Add BUILD_ICU variable separated from ENABLE_INTL_API.

Sorry, posted wrong file.

Comment 7

[Tooru Fujisawa [:arai]]

Try run started: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=010f3714daab

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8502920 [details] [diff] [review]
Add BUILD_ICU variable separated from ENABLE_INTL_API.

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

::: build/autoconf/icu.m4
@@ +35,5 @@
>      _INTL_API=$withval)
>  
>  ENABLE_INTL_API=
>  EXPOSE_INTL_API=
> +BUILD_ICU=

Rename to USE_ICU. BUILD_ICU would imply we build icu, which we don't in the MOZ_NATIVE_ICU case. (for instance, the case for ffi is different because the BUILD_ variable is for CTYPES, not FFI)

@@ +53,5 @@
>  esac
>  
> +if test -n "$ENABLE_INTL_API" -o "$MOZ_WIDGET_TOOLKIT" = "cocoa"; then
> +    BUILD_ICU=1
> +fi

The MOZ_WIDGET_TOOLKIT" = "cocoa" part should be in configure.in.

@@ +65,5 @@
> +fi
> +
> +dnl Settings for the implementation of the ECMAScript Internationalization API
> +if test -n "$BUILD_ICU"; then
> +    AC_DEFINE(BUILD_ICU)

You don't need an AC_DEFINE.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Add USE_ICU variable separated from ENABLE_INTL_API.

Thank you for your feedback!

(In reply to Mike Hommey [:glandium] from comment #8)
> Comment on attachment 8502920 [details] [diff] [review]
> Add BUILD_ICU variable separated from ENABLE_INTL_API.
> 
> Review of attachment 8502920 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/autoconf/icu.m4
> @@ +35,5 @@
> >      _INTL_API=$withval)
> >  
> >  ENABLE_INTL_API=
> >  EXPOSE_INTL_API=
> > +BUILD_ICU=
> 
> Rename to USE_ICU. BUILD_ICU would imply we build icu, which we don't in the
> MOZ_NATIVE_ICU case. (for instance, the case for ffi is different because
> the BUILD_ variable is for CTYPES, not FFI)

Okay, fixed.

> @@ +53,5 @@
> >  esac
> >  
> > +if test -n "$ENABLE_INTL_API" -o "$MOZ_WIDGET_TOOLKIT" = "cocoa"; then
> > +    BUILD_ICU=1
> > +fi
> 
> The MOZ_WIDGET_TOOLKIT" = "cocoa" part should be in configure.in.

Added _USE_ICU variable to configure.in, which is "yes" for cocoa, and "auto" for others, and USE_ICU is set to 1 when ENABLE_INTL_API=1 or USE_ICU="yes". (so, "auto" value has no meaning)
If there is more appropriate name/value, or I mis-understood your comment, please tell me :)

> @@ +65,5 @@
> > +fi
> > +
> > +dnl Settings for the implementation of the ECMAScript Internationalization API
> > +if test -n "$BUILD_ICU"; then
> > +    AC_DEFINE(BUILD_ICU)
> 
> You don't need an AC_DEFINE.

Oh, yes, that macro is not used in C preprocessor.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8503045 [details] [diff] [review]
Add USE_ICU variable separated from ENABLE_INTL_API.

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

::: configure.in
@@ +9065,5 @@
>      _INTL_API=yes
>  fi
>  
> +if test "$MOZ_WIDGET_TOOLKIT" = "cocoa"; then
> +    _USE_ICU=yes

meh, just set USE_ICU=1 here, don't set it to nothing at the beginning of icu.m4, and just set it to 1 when ENABLE_INTL_API is set.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Add USE_ICU variable separated from ENABLE_INTL_API.

Thanks again!

(In reply to Mike Hommey [:glandium] from comment #10)
> Comment on attachment 8503045 [details] [diff] [review]
> Add USE_ICU variable separated from ENABLE_INTL_API.
> 
> Review of attachment 8503045 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.in
> @@ +9065,5 @@
> >      _INTL_API=yes
> >  fi
> >  
> > +if test "$MOZ_WIDGET_TOOLKIT" = "cocoa"; then
> > +    _USE_ICU=yes
> 
> meh, just set USE_ICU=1 here, don't set it to nothing at the beginning of
> icu.m4, and just set it to 1 when ENABLE_INTL_API is set.

Ah, that's a simple solution :)

Comment 12

[Tooru Fujisawa [:arai]]

Try is running: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5b88705f2c13

Comment 13

[Tooru Fujisawa [:arai]]

Almost green on try run.

I couldn't find related bug for following, but I guess it's not related to this patch.
  * Linux debug: R(R)
    maybe similar to bug 1073442 ?
  * Linux x64 asan: X
  * Mulet Linux x64 opt: M(5)

Comment 14

[Tooru Fujisawa [:arai]]

Ah sorry, about Linux debug R(R), it was bug 1080010.

Comment 15

[Tooru Fujisawa [:arai]]

Thanks!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e945f3ffeb7e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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