1054154 - MOZ_ICU_CFLAGS are passed to late, conflict with system ICU

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jan Beich]

devel/icu port is at 53.1 since 2014-06-11 while intl/icu/source is still at 52.1. However, www/firefox-nightly uses --with-system-icu while the buildbot had 52.1 installed until recently.

intl/unicharutil/util/internal/libintl_unicharutil_util_internal.a(Unified_cpp_util_internal0.o): In function `ICUUtils::LocalizeNumber(double, ICUUtils::LanguageTagIterForContent&, nsAString_internal&)':
intl/unicharutil/util/ICUUtils.cpp:107: undefined reference to `unum_open_53'
/usr/bin/ld: intl/unicharutil/util/internal/libintl_unicharutil_util_internal.a(Unified_cpp_util_internal0.o): relocation R_X86_64_PC32 against `unum_open_53' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: Bad value

http://buildbot.rhaalovely.net/builders/mozilla-central-freebsd-amd64/builds/386/steps/build/logs/stdio

Looking at the commit replacing LOCAL_INCLUDES with CXXFLAGS seems wrong especially if the value may sometime contain a path to bundled library include. With cpp(1) the first existing include path wins and that happens to be whatever OS_*FLAGS contain.

Comment 1

[Jan Beich]

Attachment: workaround

js/src/Makefile.in also still has

  LOCAL_INCLUDES = $(MOZ_FFI_CFLAGS)
  ...
  LOCAL_INCLUDES += $(MOZ_ICU_CFLAGS)

Comment 2

[Jan Beich]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=376657cd54a8

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8473500 [details] [diff] [review]
workaround

Apologies for the questions on such a simple patch; ICU building is somewhat of a mystery to me.

My understanding of your explanation of the problem is that before, we had:

  $CC ... -I/system/icu/path ... -I/moz/bundled/icu/path

and now we have:

  $CC ... -I/moz/bundled/icu/path ... -I/system/icu/path

and then that means some symbols in libxul are ending up with the wrong visibility...Is that correct?  Even reading it, that seems off.  Seems really odd that -I/moz/bundled/icu/path would be ending up in cflags when we're configuring --with-system-icu, but maybe that is my ignorance of ICU.

This patch is obviously correct insofar as it restores buildability, but why not move the addition from CXXFLAGS to LOCAL_INCLUDES in the moz.build file?

Comment 4

[Jan Beich]

(In reply to Nathan Froyd (:froydnj) from comment #3)
> My understanding of your explanation of the problem is that before, we had:
> 
>   $CC ... -I/system/icu/path ... -I/moz/bundled/icu/path

No, that's how current include flags look like.

> 
> and now we have:
> 
>   $CC ... -I/moz/bundled/icu/path ... -I/system/icu/path

And that was before bug 1042878. Remember, first include path wins.

  $ mkdir -p system/unicode bundled/unicode
  $ echo >system/unicode/unum.h
  $ echo >bundled/unicode/unum.h
  $ echo '#include "unicode/unum.h"' | cc -E - -I system -I bundled | fgrep unum.h
  # 1 "system/unicode/unum.h" 1
  $ echo '#include "unicode/unum.h"' | cc -E - -I bundled -I system | fgrep unum.h
  # 1 "bundled/unicode/unum.h" 1

> 
> and then that means some symbols in libxul are ending up with the wrong
> visibility...Is that correct?

No, symbol visibility is supposed to protect against symbols with the same name clashing at runtime. The issue here is about wrong symbols being used in the first place.

ICU tends to encode ABI version in symbol names. When ICU declarations are picked up from a header under an unexpected path it may cause an error at link time e.g., unum_open_52 vs. unum_open_53. For other libraries one may not notice until the application crashes.

> Even reading it, that seems off.  Seems
> really odd that -I/moz/bundled/icu/path would be ending up in cflags when
> we're configuring --with-system-icu, but maybe that is my ignorance of ICU.

--with-system-icu builds fine and is not affected. I don't usually
test without it.

-I/system/icu/path is actually -I/usr/local/include on FreeBSD and DragonFly. Pretty much anything not included in somewhat minimalistic base system ends up installed under /usr/local by default. The include is added by configure.in

  case "$target_os" in
  freebsd*|openbsd*)
  # for stuff like -lXshm
      CPPFLAGS="${CPPFLAGS} ${X_CFLAGS}"
      ;;
  esac

where X_CFLAGS default value are specified in autoconf-2.13 itself.

> 
> This patch is obviously correct insofar as it restores buildability, but why
> not move the addition from CXXFLAGS to LOCAL_INCLUDES in the moz.build file?

Does LOCAL_INCLUDES in moz.build support actual CPPFLAGS ? It appears not.

if CONFIG['ENABLE_INTL_API']:
    LOCAL_INCLUDES += CONFIG['MOZ_ICU_CFLAGS']

results in

$ fgrep LOCAL_ obj-*/intl/unicharutil/util/internal/backend.mk
LOCAL_INCLUDES += -I$(srcdir)/..
LOCAL_INCLUDES += -I$(srcdir)/../../src
LOCAL_INCLUDES += -I$(srcdir)/-I$(topsrcdir)/intl/icu/source/common
LOCAL_INCLUDES += -I$(srcdir)/-I$(topsrcdir)/intl/icu/source/i18n

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8473500 [details] [diff] [review]
workaround

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

Can you file a followup bug, CC'ing :glandium, about sorting this out, please?  My impression is that LOCAL_INCLUDES isn't for shoehorning in includes from system libraries.

Comment 6

[Nathan Froyd [:froydnj]]

And thank you for the explanation!

Comment 7

[Jan Beich]

Comment on attachment 8473500 [details] [diff] [review]
workaround

Not sure if there's a better way to fix. Let's ask a build peer this time.

(In reply to Nathan Froyd (:froydnj) from comment #5)
> Can you file a followup bug, CC'ing :glandium, about sorting this out,
> please?  My impression is that LOCAL_INCLUDES isn't for shoehorning in
> includes from system libraries.

If ICU headers were exported like other bundled libs do then there'd be no issue because -I ../../dist/include is early enough before OS_*FLAGS. Unfortunately, bug 960505 comment 1 decided against the idea.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8944f233d14b

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8944f233d14b