Like what we do for lots of other external libs.

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...

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.

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?

I guess it does.

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.

Review commit: https://reviewboard.mozilla.org/r/62760/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/62760/

Review commit: https://reviewboard.mozilla.org/r/62762/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/62762/

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.

https://reviewboard.mozilla.org/r/62760/#review60468 Because js/src already has this in moz.build: https://dxr.mozilla.org/mozilla-central/rev/214884d507ee369c1cf14edb26527c4f9a97bf48/js/src/moz.build#700-703

Comment on attachment 8768603 [details] Bug 1284406 part 3 - Export ICU headers in include/unicode. https://reviewboard.mozilla.org/r/62762/#review60472

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.

Review commit: https://reviewboard.mozilla.org/r/64822/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64822/

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.

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=969f82552c49

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.

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