Closed Bug 1228208 Opened 4 years ago Closed 4 years ago

MOZ_ICU_CFLAGS are passed to late, conflict with system ICU

Categories

(Core :: JavaScript: Internationalization API, defect)

Unspecified
FreeBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- affected
firefox43 --- affected
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: jbeich, Assigned: jbeich)

References

Details

Attachments

(1 file, 4 obsolete files)

Bug 1175413 likely regressed bug 1054154. FreeBSD remains sensitive to -I order where -isystem should have been used. pkg-config and ${X_CFLAGS} from autoconf are oblivious to it, so the build fails similar to bug 1218027.

  c++ -o Unified_cpp_util_internal0.o -c -I../../../../dist/stl_wrappers -I../../../../dist/system_wrappers -include /mozilla-central/config/gcc_hidden.h -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/mozilla-central/intl/unicharutil/util/internal -I. -I/mozilla-central/intl/unicharutil/util -I../../../../dist/include  -I/objdir/dist/include/nspr -I/objdir/dist/include/nss       -fPIC  -DMOZILLA_CLIENT -include ../../../../mozilla-config.h -MD -MP -MF .deps/Unified_cpp_util_internal0.o.pp -Qunused-arguments   -I/usr/local/include -Qunused-arguments -Wno-unused-local-typedef -Wall -Wempty-body -Woverloaded-virtual -Wsign-compare -Wwrite-strings -Wno-invalid-offsetof -Wno-inline-new-delete -Wno-unused-local-typedef -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pipe  -DNDEBUG -DTRIMMED -g -O -fomit-frame-pointer  -I/mozilla-central/intl/icu/source/common -I/mozilla-central/intl/icu/source/i18n   /objdir/intl/unicharutil/util/internal/Unified_cpp_util_internal0.cpp
  In file included from /objdir/intl/unicharutil/util/internal/Unified_cpp_util_internal0.cpp:11:
  In file included from /mozilla-central/intl/unicharutil/util/ICUUtils.cpp:8:
  In file included from /mozilla-central/intl/unicharutil/util/ICUUtils.h:18:
  In file included from /usr/local/include/unicode/unum.h:20:
  In file included from /usr/local/include/unicode/uloc.h:25:
  In file included from /usr/local/include/unicode/uenum.h:24:
  In file included from /usr/local/include/unicode/strenum.h:14:
  In file included from /usr/local/include/unicode/unistr.h:31:
  In file included from /usr/local/include/unicode/std_string.h:33:
  In file included from ../../../../dist/stl_wrappers/string:50:
  In file included from ../../../../dist/system_wrappers/string:3:
  In file included from /usr/include/c++/v1/string:439:
  In file included from ../../../../dist/stl_wrappers/algorithm:50:
  In file included from ../../../../dist/system_wrappers/algorithm:3:
  In file included from /usr/include/c++/v1/algorithm:628:
  In file included from ../../../../dist/stl_wrappers/memory:50:
  In file included from ../../../../dist/system_wrappers/memory:3:
  In file included from /usr/include/c++/v1/memory:600:
  In file included from ../../../../dist/system_wrappers/typeinfo:3:
  /usr/include/c++/v1/typeinfo:72:7: error: visibility does not match previous
	declaration
  class _LIBCPP_EXCEPTION_ABI type_info
	^
  /usr/include/c++/v1/__config:221:47: note: expanded from macro
	'_LIBCPP_EXCEPTION_ABI'
  #define _LIBCPP_EXCEPTION_ABI __attribute__ ((__visibility__("default")))
						^
  /mozilla-central/config/gcc_hidden.h:6:13: note: previous attribute is here
  #pragma GCC visibility push(hidden)
	      ^
  1 error generated.

To fix this -I/mozilla-central/intl/icu/source/common, etc. should come before -I/usr/local/include effectively restoring LOCAL_INCLUDES behavior.
Attached patch ugly workaround (obsolete) — Splinter Review
typeinfo issue moved to bug 1228227. -I order still matters to avoid linking errors due to ABI mismatch between system and bundled ICU or their visibility.
Attached patch ugly workaround, v1 (obsolete) — Splinter Review
Missed quite a few places which resulted in linking errors e.g.,

  intl/unicharutil/util/nsUnicodeProperties.cpp:137: error: undefined reference to 'u_charMirror_55'
  intl/unicharutil/util/nsUnicodeProperties.cpp:147: error: undefined reference to 'u_isMirrored_55'
  intl/unicharutil/util/nsUnicodeProperties.cpp:157: error: undefined reference to 'u_getCombiningClass_55'
  intl/unicharutil/util/nsUnicodeProperties.cpp:304: error: undefined reference to 'u_getIntPropertyValue_55'
  intl/unicharutil/util/nsUnicodeProperties.cpp:304: error: undefined reference to 'u_getIntPropertyValue_55'
  c++: error: linker command failed with exit code 1 (use -v to see invocation)
Attachment #8692327 - Attachment is obsolete: true
Assignee: nobody → jbeich
Status: NEW → ASSIGNED
Assignee: jbeich → nobody
Status: ASSIGNED → NEW
Attachment #8692464 - Attachment is obsolete: true
Less ugly than attachment 8692392 [details] [diff] [review] but I'm not sure it's the right approach.
Green on Try in comment 6 (via bzpost).
Attachment #8692888 - Flags: review?(mh+mozilla)
Attachment #8692392 - Attachment is obsolete: true
Assignee: nobody → jbeich
Status: NEW → ASSIGNED
Comment on attachment 8692888 [details] [diff] [review]
Make sure ICU flags are prepended before system flags, v0

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

::: build/autoconf/icu.m4
@@ -20,5 @@
>      PKG_CHECK_MODULES(MOZ_ICU, icu-i18n >= 50.1)
>      MOZ_SHARED_ICU=1
> -else
> -    MOZ_ICU_CFLAGS="-I$_topsrcdir/intl/icu/source/common -I$_topsrcdir/intl/icu/source/i18n"
> -    AC_SUBST_LIST(MOZ_ICU_CFLAGS)

A simpler solution, IMHO, would be to use a different variable in this case, leaving MOZ_ICU_CFLAGS empty. So you can do something like:

  MOZ_ICU_INCLUDES="/intl/icu/source/common /intl/icu/source/i18n"
  AC_SUBST_LIST(MOZ_ICU_INCLUDES)

(yes, without -I$_topsrcdir and starting with a / for each) and in moz.build files:
  LOCAL_INCLUDES += CONFIG['MOZ_ICU_INCLUDES']

which will also work in the native ICU case since that would expand to LOCAL_INCLUDES += []
Attachment #8692888 - Flags: review?(mh+mozilla) → feedback-
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49d6733d5d40
And green local build with --with-system-icu.
Attachment #8695075 - Flags: review?(mh+mozilla)
Attachment #8692888 - Attachment is obsolete: true
Attachment #8695075 - Flags: review?(mh+mozilla) → review+
Keywords: checkin-needed
This changeset seems to have introduced a mozbuild error, when building js/src stand-alone. I get the following error in changeset 35ac64d68809, but not in its parent 9b6075887a72

Reticulating splines...
Traceback (most recent call last):
  File "./config.status", line 449, in <module>
    config_status(**args)
  File "/home/jimb/moz/dbg/python/mozbuild/mozbuild/config_status.py", line 178, in config_status
    the_backend.consume(definitions)
  File "/home/jimb/moz/dbg/python/mozbuild/mozbuild/backend/base.py", line 118, in consume
    for obj in objs:
  File "/home/jimb/moz/dbg/python/mozbuild/mozbuild/frontend/emitter.py", line 165, in emit
    for out in output:
  File "/home/jimb/moz/dbg/python/mozbuild/mozbuild/frontend/reader.py", line 1062, in read_mozbuild
    raise bre
mozbuild.frontend.reader.BuildReaderError: 
==============================
ERROR PROCESSING MOZBUILD FILE
==============================

The error occurred while processing the following file:

    /home/jimb/moz/dbg/js/src/moz.build

The error was triggered on line 670 of this file:

    LOCAL_INCLUDES += CONFIG['MOZ_ICU_INCLUDES']

An error was encountered as part of executing the file itself. The error appears to be the fault of the script.

The error as reported by Python is:

    ["TypeError: 'NoneType' object is not iterable\n"]
Flags: needinfo?(jbeich)
Flags: needinfo?(mh+mozilla)
https://hg.mozilla.org/mozilla-central/rev/35ac64d68809
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
I had neglected to rebuild 'js/src/configure'. Problem solved.
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(jbeich)
You need to log in before you can comment on or make changes to this bug.