bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 3/7] iconv_open: reduce not-trivial repetition of file-list


From: Gary V. Vaughan
Subject: Re: [PATCH 3/7] iconv_open: reduce not-trivial repetition of file-list
Date: Wed, 13 Oct 2010 16:45:52 +0700

On 13 Oct 2010, at 03:35, Bruno Haible wrote:
>> By itself, this patch reduces unnecessary repetition, but also sets up
>> a later patch in this series to not need to make yet another copy of
>> the listed headers.
>> 
>> * modules/iconv_open (iconv_headers): New make macro to hold list of
>> iconv header files.
>> (BUILT_SOURCES, MAINTAINERCLEANFILES, EXTRA_DIST): Use it instead of
>> multiple hard-coded copies.
> 
> I don't see what this patch is supposed to do. This, together with the line
> 
>  nodist_include_HEADERS += $(iconv_headers)
> 
> from [PATCH 5/7], ought to be a no-op. These 5 .h files are private header
> files of the module; they are not meant to be installed by 'make install'.

Okay, thanks.

> I agree that it's good to modify the module descriptions in a way that
> distinguishes header files that need to be installed from private header
> files - since it would be too hacky if gnulib-tool or libposix/Makefile.am
> would contain a heuristic for this. But the default should be the private
> header file. Additional Makefile.am statements should be needed for the
> public, installed header files.

Okay, good.  I'm not familiar enough with the individual modules to know
what headers should be installed and what headers are private to the
module.  But I'm glad that you agree that marking them up with the correct
automake syntax in the Makefile.am snippet of each module file is a good
way to straighten that out.

Probably my [PATCH 5/7] needs a thorough review for each module to see
whether I guessed correctly in each case.

> In summary, this module does not need modifications.
> 
> Additionally, this patch would introduce a pitfall: it would be easy to think
> that adding a file name to $(iconv_headers) is enough, and forget about
> MOSTLYCLEANFILES.

Hmm... it's a pity that there isn't an Automake syntax for transforming
lists of filenames (that works with portable make anyway), since having
four near-identical lists of filenames bothers me a (tiny) little bit.

If you see no value in it, let's drop this patch then.

Cheers,
-- 
Gary V. Vaughan (address@hidden)

Attachment: PGP.sig
Description: This is a digitally signed message part


reply via email to

[Prev in Thread] Current Thread [Next in Thread]