Closed Bug 1080910 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Internationalization, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: dholbert, Assigned: arai)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
Flags: needinfo?(joshmoz)
CC'd :arai, this is likely a regression from his change.
Flags: needinfo?(joshmoz)
Agreed -- looks like that patch added some calls to ucol_close etc. (the undefined symbols mentioned in comment 0)
Flags: needinfo?(arai_a)
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.
Flags: needinfo?(arai_a) → needinfo?(mh+mozilla)
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.
Attachment #8502919 - Flags: feedback?(mh+mozilla)
Sorry, posted wrong file.
Attachment #8502919 - Attachment is obsolete: true
Attachment #8502919 - Flags: feedback?(mh+mozilla)
Attachment #8502920 - Flags: feedback?(mh+mozilla)
Blocks: 1045958
Flags: needinfo?(mh+mozilla)
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.
Attachment #8502920 - Flags: feedback?(mh+mozilla) → feedback+
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.
Attachment #8502920 - Attachment is obsolete: true
Attachment #8503045 - Flags: review?(mh+mozilla)
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.
Attachment #8503045 - Flags: review?(mh+mozilla) → feedback+
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 :)
Attachment #8503045 - Attachment is obsolete: true
Attachment #8503060 - Flags: review?(mh+mozilla)
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)
Ah sorry, about Linux debug R(R), it was bug 1080010.
Attachment #8503060 - Flags: review?(mh+mozilla) → review+
Thanks!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e945f3ffeb7e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.