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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

Like what we do for lots of other external libs.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: wip patch

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

Comment 2

[Mike Hommey [:glandium]]

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.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Does that make sense to you?

Comment 6

[Mike Hommey [:glandium]]

I guess it does.

Comment 7

[(not currently active) Ted Mielczarek]

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?

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1284406 part 1 - Move warning suppression of C4577 to global level.

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

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1284406 part 3 - Export ICU headers in include/unicode.

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

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

This patchset is based on bug 1284179 which makes icu_sources_data.py run on Windows... because I'm developing on Windows.

Comment 12

[Mike Hommey [:glandium]]

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.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

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 14

[Mike Hommey [:glandium]]

Comment on attachment 8768603 [details]
Bug 1284406 part 3 - Export ICU headers in include/unicode.

https://reviewboard.mozilla.org/r/62762/#review60472

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02a57d8120c4
followup - Remove ICU includes from js/src/moz.build to fix the bustage.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1284406 part 2 - Avoid including ICU header when Intl API is disabled.

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

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

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 21

[Xidorn Quan [:xidorn] UTC+11]

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/

Comment 22

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 24

[Jeff Walden [:Waldo]]

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.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/42f884f30bc3
https://hg.mozilla.org/mozilla-central/rev/10ac75b9846c
https://hg.mozilla.org/mozilla-central/rev/4a4bbb7e9e79