Closed Bug 960505 Opened 6 years ago Closed 6 years ago

Make it easier for Mozilla code that uses ICU to be compiled with the necessary ICU include paths

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

I have a patch for this that I'll clean up and attach shortly.
No longer blocks: 915735
Depends on: 915735
Blocks: 924851
Depends on: 958404
Turns out my patch didn't really generalize.

Ideally I wanted to put the ICU headers under a dist/include/icu directory and include headers with a path like:

  #include "icu/i18n/unicode/decimfmt.h"

That would provide clean paths and would allow any mozilla file to use these headers without requiring extra include paths to be added to its moz.build file. Unfortunately that doesn't work because, for example, decimfmt.h contains:

  #include "unicode/dcfmtsym.h"

It might seem like it would be possible to export the headers to dist/include/icu anyway and then add $(DIST)/include/icu/i18n etc. to GENERATED_INCLUDES (or something) in the moz.build of any files that need to use the ICU headers in order to get the include directives in ICU headers working. There are several reasons not to do that though. First we need to support system ICU, and it expects those headers to be used from /usr/include after make install has been run. So the paths we need to use in include directives are pretty much forced on us (if only ICU had a nice clean include structure like protobuf, which has a structure that puts everything under a nice clean "google/protobuf" directory so you end up with #include <google/protobuf/stubs/common.h> etc.). Second, if moz.build files are going to need to set GENERATED_INCLUDES (or something) then they might as well set LOCAL_INCLUDES (or something similar) which is what spidermonkey does now. Third, it would be undesirable to allow ICU headers to be included using two different include paths.

Exporting the ICU headers isn't the right thing to do then, IMO. Rather we should simply make it easier for parts of the code that need ICU headers to add the necessary include paths.
Summary: Export headers from ICU so that Mozilla code can use it → Make it easier for Mozilla code that uses ICU to be compiled with the necessary ICU include paths
Attached patch patch (obsolete) — Splinter Review
Attachment #8367802 - Flags: review?(mh+mozilla)
Attached patch patchSplinter Review
Attachment #8367802 - Attachment is obsolete: true
Attachment #8367802 - Flags: review?(mh+mozilla)
Attachment #8367807 - Flags: review?(mh+mozilla)
Comment on attachment 8367807 [details] [diff] [review]
patch

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

::: configure.in
@@ +3927,5 @@
>      PKG_CHECK_MODULES(MOZ_ICU, icu-i18n >= 50.1)
>      MOZ_JS_STATIC_LIBS="$MOZ_JS_STATIC_LIBS $MOZ_ICU_LIBS"
>      MOZ_SHARED_ICU=1
> +else
> +    MOZ_ICU_CFLAGS=" -I$_topsrcdir/intl/icu/source/common -I$_topsrcdir/intl/icu/source/i18n"

Make that MOZ_ICU_CFLAGS='-I$(topsrcdir)/intl/icu/source/common -I$(topsrcdir)/intl/icu/source/i18n' (with single quotes and $(topsrcdir) instead of $_topsrcdir) and that'll likely fix your build errors on windows. r+ with this change.
Attachment #8367807 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/fc948d7583a5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.