From: Stefano Lattarini Date: Wed, 11 Sep 2013 18:03:57 GMT Subject: Re: [PATCH v2] configure.ac: move the private git m4 macros to a dedicated directory 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: > 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. > 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... > 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 > --- > 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