Re: [PATCH v2] configure.ac: move the private git m4 macros to a dedicated directory

From: Stefano Lattarini <stefano.lattarini@gmail.com>
Date: Sep 11, 2013, 18:03 UTC
Message-ID: <5230B08D.6080109@gmail.com>
In-Reply-To: <1378914417-32605-1-git-send-email-gitter.spiros@gmail.com>

Hi Elia.  Sorry, but I have to give my NAK to this patch.

On 09/11/2013 04:46 PM, Elia Pinto wrote:

Show 13 quoted lines

> Git use, as many project that use autoconf, private m4 macros.
>
> When not using automake, and just relying on autoconf, the macro
> files are not picked up by default.
>
> A possibility, as git do today, is to put the private m4 macro
> in the configure.ac file, so they will copied over the final configure
> when calling autoreconf(that call also autoconf).
> But this makes configure.ac difficult to read and maintain,
> especially if you want to introduce new macros later. By separating
> the definitions of the macros from configure.ac file the build system
> would be more modular.
>

In which sense are we being more modular exactly?  After all:

   - the configure.ac of Git is the only user of these macros,

   - using m4_include doesn't offer any performance improvement, and

   - m4 doesn't offer any namespace granularity anyway.

So it seems to me that this patch only adds extra indirections without adding any real benefit.

Show 17 quoted lines

> Starting from version 2.58, autoconf provide the macro AC_CONFIG_MACRO_DIR
> to declare where additional macro files are to be put and found by aclocal.
> The argument passed to this macro is commonly m4. Despite the documentation,
> autoconf do nothing with it, only aclocal can use directly if invoked by
> -I m4 or indirectly using automake. But autoreconf don't invoke aclocal
> in this way. So in summary you can not use this macro in a useful
> way if you only use autoconf, as git does.
>
> Another historical possibility is to list all your macros in acinclude.m4.
> This file will be included in aclocal.m4 when you run aclocal, and its macro(s)
> will henceforth be visible to autoconf. However if it contains numerous macros,
> it will rapidly become difficult to maintain, and for git this don't provide
> any benefits or very little.
>
> The actual autotool documentation recommend to write each
> macro in its own file and gather all these files in a separate directory.
>

Where exactly id you find that recommendation? If the autotools docs tell to do so *unconditionally*, they are wrong and should be fixed. In fact, even the configure.ac from Automake itself keeps definition of private macros in configure.ac...

Show 33 quoted lines

> Given the limitations i mentioned earlier, the only possibility is to use the m4_include
> for including every macro file. The m4_include directive works quite like the
> #include directive of the C programming language, and simply copies over the content
> of the file(s).
>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> This is a second version of this patch http://article.gmane.org/gmane.comp.version-control.git/231984.
> The first was plain wrong, my bad. I am sorry for the long delay.
> Sure it is something low-hanging fruit
>
>
>   configure.ac                      |  148 +++----------------------------------
>   m4/git_arg_set_path.m4            |   14 ++++
>   m4/git_check_func.m4              |   13 ++++
>   m4/git_conf_append_path.m4        |   30 ++++++++
>   m4/git_conf_subst.m4              |   10 +++
>   m4/git_conf_subst_init.m4         |   15 ++++
>   m4/git_parse_with.m4              |   22 ++++++
>   m4/git_parse_with_set_make_var.m4 |   20 +++++
>   m4/git_stash_flags.m4             |   15 ++++
>   m4/git_unstash_flags.m4           |   13 ++++
>   10 files changed, 162 insertions(+), 138 deletions(-)
>   create mode 100644 m4/git_arg_set_path.m4
>   create mode 100644 m4/git_check_func.m4
>   create mode 100644 m4/git_conf_append_path.m4
>   create mode 100644 m4/git_conf_subst.m4
>   create mode 100644 m4/git_conf_subst_init.m4
>   create mode 100644 m4/git_parse_with.m4
>   create mode 100644 m4/git_parse_with_set_make_var.m4
>   create mode 100644 m4/git_stash_flags.m4
>   create mode 100644 m4/git_unstash_flags.m4
>

 > [SNIP]
 >

Regards,
   Stefano