Re: [PATCH] upgrade icu to 51.1

On 25/03/13 13:21, Eike Rathke wrote:
> Hi Peter,
>
> On Saturday, 2013-03-23 21:01:45 +0000, Peter Foley (via Code Review) wrote:
>
> > --- a/RepositoryExternal.mk
> > +++ b/RepositoryExternal.mk
> > @@ -1081,9 +1081,11 @@
> >  endif
> >  
> >  define gb_LinkTarget__use_icu_headers
> > -$(call gb_LinkTarget_use_package,$(1),icu)
> > +$(call gb_LinkTarget_use_unpacked,$(1),icu)

i haven't checked it but if ICU generates header files then "unpacked"
is not sufficient.

> >  $(call gb_LinkTarget_set_include,$(1),\
> > -    $(ISYSTEM)$(OUTDIR)/inc/external \
> > +    $(ISYSTEM)$(call gb_UnpackedTarball_get_dir,icu)/source \
> > +    $(ISYSTEM)$(call gb_UnpackedTarball_get_dir,icu)/source/i18n \
> > +    $(ISYSTEM)$(call gb_UnpackedTarball_get_dir,icu)/source/common \
> >      $$(INCLUDE) \
> >  )
>
> I'm not convinced of this change and the corresponding removal of all
> to-be-delivered header files in icu/ExternalPackage_icu.mk
>
> Doing so gives access to almost all icu header files, including the
> internal ones, whereas previously only those were delivered that icu
> would also install in a real installation, namely source/layout/* ->
> layout/* and source/common/unicode/* -> unicode/* and
> source/i18n/unicode/* -> unicode/*

there is another potential problem beyond being able to include internal
headers directly:  the internal headers may have names that conflict
with header names from other libraries, causing the wrong headers to be
included.

> I'd much prefer if we found a different solution for this.

ideally ICU would have a top-level include directory containing only
public headers.  since that is apparently not the case...

> @All:
> Shouldn't we actually install external libraries into our workdir for
> such purposes? We'd then have a clean set of what a package thinks
> should be installed.

generally copying header files around is an annoyance.

for most of the externals we support a "system" library too, and in that
case the internal headers will not be available.  so a build on a
tinderbox configured --with-system-libs would hopefully find all
accidental uses of internal headers.

the other case of name clashes between different libraries due to same
header names would most likely also be detected immediately via build
breakage.

so i don't think the risk of breaking stuff is really that big.