Re-export ICU headers in include/unicode/ to make them accessible everywhere

RESOLVED FIXED in Firefox 50

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

unspecified
mozilla50
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(4 attachments)

Like what we do for lots of other external libs.
Posted patch wip patchSplinter Review
This is the wip patch. I'm not quite familiar with the build system... so if someone can help finish this up, that would be great.

There are several warnings as error still block the building. I don't have a great idea about how to fix them...
Component: Internationalization → Build Config
By removing the
    CXXFLAGS += CONFIG['MOZ_ICU_CFLAGS']	
    LOCAL_INCLUDES += CONFIG['MOZ_ICU_INCLUDES']

lines, you're breaking building against system ICU.

And by making the headers available in dist/include, you're hiding future breakage where those would be missing in other directories.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
I added
> CFLAGS="$CFLAGS $MOZ_ICU_CFLAGS"
> CXXFLAGS="$CXXFLAGS $MOZ_ICU_CFLAGS"
to build/autoconf/icu.m4 in expection that building against system ICU won't be broken.
The main idea is that we want to ICU headers accessible everywhere so that we don't need to add
> CXXFLAGS += CONFIG['MOZ_ICU_CFLAGS']	
> LOCAL_INCLUDES += CONFIG['MOZ_ICU_INCLUDES']
and
> if CONFIG['_MSC_VER']:
>   CXXFLAGS += ['-wd4577']
in every directory which would somehow use ICU code.

When we replace our internal implementation of bidi engine with ICU one (bug 924851), ICU code would have to be exposed to more directory, which makes me think we should instead make it available everywhere rather than adding those things repeatedly.
Does that make sense to you?
Flags: needinfo?(mh+mozilla)
I guess it does.
Status: RESOLVED → REOPENED
Flags: needinfo?(mh+mozilla)
Resolution: WONTFIX → ---
This seems like a lot of work. Why don't we just put `-I$(topsrcdir)/intl/icu/source/common -I$(topsrcdir)/intl/icu/source/i18n` in CFLAGS/CXXFLAGS for the non-system-ICU case, instead of copying a bunch of headers around?
One reason is that there are lots of other headers inside those two directories which are not something we want our code to use, so I'm worried about if that would become a problem. The other reason is that I found we've been doing so for lots of other third-party libraries, though I have no idea why they are done that way.

But anyway... I don't have strong opinion about that.
This patchset is based on bug 1284179 which makes icu_sources_data.py run on Windows... because I'm developing on Windows.
Comment on attachment 8768602 [details]
Bug 1284406 part 1 - Move warning suppression of C4577 to global level.

https://reviewboard.mozilla.org/r/62760/#review60468

I wonder why it's not a problem for js/src.
Attachment #8768602 - Flags: review?(mh+mozilla) → review+
Attachment #8768603 - Flags: review?(mh+mozilla) → review+
Assignee: nobody → xidorn+moz
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b186c22ef65f
part 1 - Move warning suppression of C4577 to global level. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcfaa7cf4118
part 2 - Export ICU headers in include/unicode. r=glandium
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02a57d8120c4
followup - Remove ICU includes from js/src/moz.build to fix the bustage.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a73f1ed7dab0
followup 2 - Add back ICU flags in js/src/moz.build for system ICU to fix bustage on CLOSED TREE.
All four changesets are backed out in
https://hg.mozilla.org/integration/mozilla-inbound/rev/db8491cc618f

Set ni? myself to investigate. Looks like building with system icu is broken.
Flags: needinfo?(xidorn+moz)
Review commit: https://reviewboard.mozilla.org/r/64822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64822/
Attachment #8768603 - Attachment description: Bug 1284406 part 2 - Export ICU headers in include/unicode. → Bug 1284406 part 3 - Export ICU headers in include/unicode.
Attachment #8771861 - Flags: review?(jwalden+bmo)
Comment on attachment 8768602 [details]
Bug 1284406 part 1 - Move warning suppression of C4577 to global level.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62760/diff/1-2/
Comment on attachment 8768603 [details]
Bug 1284406 part 3 - Export ICU headers in include/unicode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62762/diff/1-2/
So the failure was not about building with system icu, but about building with icu disabled.

A js header unconditionally includes ICU header which causes this issue. That code originally works because we expose ICU headers as local includes even if it is disabled.
Flags: needinfo?(xidorn+moz)
Comment on attachment 8771861 [details]
Bug 1284406 part 2 - Avoid including ICU header when Intl API is disabled.

https://reviewboard.mozilla.org/r/64822/#review62366

::: js/src/builtin/Intl.cpp:84
(Diff revision 1)
>  
> +typedef bool UBool;
> +typedef char16_t UChar;
> +typedef double UDate;
> +
> +enum UErrorCode {

Are you sure you can't just have

  enum UErrorCode {
    U_ZERO_ERROR,
    U_BUFFER_OVERFLOW_ERROR,
  };

instead of listing a big huge thing?  I think you can get away only listing the couple you need, without their actual values.

::: js/src/builtin/Intl.cpp:250
(Diff revision 1)
> +};
> +
> +static inline UBool
> +U_FAILURE(UErrorCode code)
> +{
> +    return code>U_ZERO_ERROR;

And this can just MOZ_CRASH like all the others.

::: js/src/builtin/Intl.cpp:256
(Diff revision 1)
> +}
> +
> +inline const UChar*
> +Char16ToUChar(const char16_t* chars)
> +{
> +    return chars;

This too.

::: js/src/builtin/Intl.cpp:260
(Diff revision 1)
> +{
> +    return chars;
> +}
> +
> +inline UChar*
> +Char16ToUChar(char16_t* chars)

This too.
Attachment #8771861 - Flags: review?(jwalden+bmo) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42f884f30bc3
part 1 - Move warning suppression of C4577 to global level. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/10ac75b9846c
part 2 - Avoid including ICU header when Intl API is disabled. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a4bbb7e9e79
part 3 - Export ICU headers in include/unicode. r=glandium
https://hg.mozilla.org/mozilla-central/rev/42f884f30bc3
https://hg.mozilla.org/mozilla-central/rev/10ac75b9846c
https://hg.mozilla.org/mozilla-central/rev/4a4bbb7e9e79
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1307358
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.