Quantcast

[PATCH] autoheader: check templates of all config headers

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] autoheader: check templates of all config headers

Daniel Elstner
Hi,

sorry for being annoying about this -- sending this here now since I
never got any response on autoconf-patches. I'd greatly appreciate if
someone could have a look at my patch and tell me whether it's
acceptable. I already completed the copyright assignment process.

Perhaps I should provide some more detail about the use case for this
change. In a nutshell:

Autoconf already allows for more than one config header. This feature is
useful e.g. for libraries which need to install a machine-specific
configuration header that is included by the library's public header
files. Apart from system configuration, this is also useful for version
number defines. I've been using this scheme successfully for many years
in a number of C and C++ libraries I've worked on.

However, only the first configuration header passed as argument to
AC_CONFIG_HEADERS() has its template generated by Autoheader. Secondary
configuration headers have to use hand-written template files. This is
just fine for the library use case, where it is important to tightly
control the symbols put into the installed config header, since user
code will be indirectly exposed to it.

So all would be just fine if it weren't for the problem that Autoheader
doesn't really handle the situation correctly. In order to appease
Autoheader's sanity checking (which cannot be disabled), it is currently
necessary to use the three-argument version of AC_DEFINE() even for
defines which already appear in a manually written header template file.
Using the three-argument AC_DEFINE() of course also means that the
define is duplicated into the first config header, too.

Even so it is still usable, and I've lived with that problem for years.
However, I've recently run into a situation which prompted me to create
this patch to fix Autoheader. The issue is with auto-generated version
defines, which may include a git revision hash and thus change with
every commit. Due to the duplication of all defines into the first
configuration header, which is included by every source file in the
project, this causes the entire project to be rebuild on every change.

If the version defines could be limited to the secondary config header,
the dependencies could be contained and the need to rebuild minimized.
Incidentally, the goal to reduce rebuilds is also the reason why a
proper configuration header is preferable over using AC_SUBST for this,
since the latter mechanism does not have the update-avoiding check for
changed content.

Long story short, my patch makes Autoheader behave like it probably
should have from the beginning. That is, the hand-written template files
of secondary config headers are considered during the sanity check
whether all defines have a corresponding template. This makes it
possible to use the two-argument version of AC_DEFINE() for defines with
a hand-written template, thereby avoiding the spill into the first
config header. This change should have no effect on existing code.

Is this acceptable? I'd be delighted if it could be part of the next
release.

Cheers,
--Daniel

* bin/autoheader.in: When checking for missing templates, take
all config headers into account, not just the one generated by
autoheader.  This makes it possible to use AC_DEFINE() for
secondary headers without duplicating the template into the
first header.
* tests/tools.at: Add a check for autoheader with multiple
config headers.
* NEWS: Document the new behavior.
---
 NEWS              |  5 +++++
 bin/autoheader.in | 37 ++++++++++++++++++++++++++-----------
 tests/tools.at    | 25 +++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/NEWS b/NEWS
index f691179..ceda4a0 100644
--- a/NEWS
+++ b/NEWS
@@ -27,6 +27,11 @@ GNU Autoconf NEWS - User visible changes.
    is now deprecated.  If you really need that behavior use
    AC_PREPROC_IFELSE.
 
+** When checking for missing templates, autoheader now takes any
+   templates defined in the inputs of secondary config headers into
+   account.  This makes it possible to use AC_DEFINE for secondary
+   headers without duplicating the template in the main config header.
+
 ** Macros
 
 - New macro AC_C__GENERIC.
diff --git a/bin/autoheader.in b/bin/autoheader.in
index 8c70663..fe06774 100644
--- a/bin/autoheader.in
+++ b/bin/autoheader.in
@@ -191,11 +191,21 @@ unless ($config_h)
     exit 1;
   }
 
-# We template only the first CONFIG_HEADER.
-$config_h =~ s/ .*//;
 # Support "outfile[:infile]", defaulting infile="outfile.in".
-($config_h, $config_h_in) = split (':', $config_h, 2);
-$config_h_in ||= "$config_h.in";
+sub templates_for_header ($)
+{
+  my ($spec) = @_;
+  my ($header, @templates) = split(':', $spec);
+
+  return @templates if (@templates);
+  return $header . '.in';
+}
+
+my @config_templates = map(templates_for_header($_), split(' ', $config_h));
+
+# We template only the first CONFIG_HEADER.
+$config_h_in = shift(@config_templates);
+$config_h =~ s/[ :].*//;
 
 # %SYMBOL might contain things like 'F77_FUNC(name,NAME)', but we keep
 # only the name of the macro.
@@ -261,14 +271,20 @@ $out->close;
 
 # Check that all the symbols have a template.
 {
-  my $in = new Autom4te::XFile ("< " . open_quote ("$tmp/config.hin"));
-  my $suggest_ac_define = 1;
-  while ($_ = $in->getline)
+  foreach my $template ("$tmp/config.hin", @config_templates)
     {
-      my ($symbol) = /^\#\s*\w+\s+(\w+)/
- or next;
-      delete $symbol{$symbol};
+      my $in = new Autom4te::XFile ("< " . open_quote ($template));
+
+      while ($_ = $in->getline)
+ {
+  my ($sym) = /^\#\s*\w+\s+(\w+)/
+    or next;
+  delete $symbol{$sym};
+ }
     }
+
+  my $suggest_ac_define = 1;
+
   foreach (sort keys %symbol)
     {
       msg 'syntax', "warning: missing template: $_";
@@ -277,7 +293,6 @@ $out->close;
   msg 'syntax',  "Use AC_DEFINE([$_], [], [Description])";
   $suggest_ac_define = 0;
  }
-
     }
   exit 1
     if keys %symbol;
diff --git a/tests/tools.at b/tests/tools.at
index 24173c9..d5a919c 100644
--- a/tests/tools.at
+++ b/tests/tools.at
@@ -860,6 +860,31 @@ config.h.in:0
 AT_CLEANUP
 

+# autoheader should take all config header inputs into account when
+# checking for missing templates.
+AT_SETUP([autoheader with multiple headers])
+
+AT_DATA([config-extra.h.in],
+[[/* Define this to whatever you want. */
+#undef HANNA
+]])
+AT_DATA([configure.ac],
+[[AC_INIT
+AC_CONFIG_HEADERS([config.h config-extra.h])
+AC_DEFINE([HANNA], ["Hanna"])
+AC_DEFINE([SEAN], ["Sean"], [Sean's name])
+AC_OUTPUT
+]])
+
+AT_CHECK_AUTOCONF
+AT_CHECK_AUTOHEADER
+AT_CHECK([grep HANNA configure], [0], [ignore], [ignore])
+AT_CHECK([grep HANNA config.h.in], [1], [ignore], [ignore])
+AT_CHECK([grep SEAN configure], [0], [ignore], [ignore])
+AT_CHECK([grep SEAN config.h.in], [0], [ignore], [ignore])
+
+AT_CLEANUP
+
 

 ## ------------ ##



_______________________________________________
Autoconf mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/autoconf
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] autoheader: check templates of all config headers

Paul Eggert
The basic idea for this sounds good; thanks.  It'd be nice if someone else who
uses Perl more than I could look over the details.

_______________________________________________
Autoconf mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/autoconf
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] autoheader: check templates of all config headers

Daniel Elstner
Hi Paul,

On Fri, 2015-10-09 at 21:48 -0700, Paul Eggert wrote:
> The basic idea for this sounds good; thanks.  It'd be nice if someone
> else who uses Perl more than I could look over the details.

tentative ping? ;)

Cheers,
--Daniel


_______________________________________________
Autoconf mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/autoconf
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] autoheader: check templates of all config headers

Warren Young-2
In reply to this post by Paul Eggert
On Oct 9, 2015, at 10:48 PM, Paul Eggert <[hidden email]> wrote:
>
> The basic idea for this sounds good; thanks.  It'd be nice if someone else who uses Perl more than I could look over the details.

Well, I know Perl, but I don’t know the autotools internals, so all I can do is nitpick style details.  If that’s all you want, then:

> sub templates_for_header ($)

Don’t use subroutine prototypes.  They don’t do what you want, so perlcritic rightly complains about them:

  http://goo.gl/bDCsJ4

I suggest running the changed file through perlcritic at least at its default level.  I try to meet --harsh level in my code with minimal “## no critic” overrides.

> return @templates if (@templates);

Just a nit, but I typically leave the parens off when using the postfix form of “if”, especially when the expression is simple, as in this case.

> my $in = new Autom4te::XFile ("< " . open_quote ($template));

The space in that string literal looks like you’re trying to avoid some of the problems with the 1-argument form of IO::File-new().  Unless $template can be a dash to open STDIN, there is no good reason to use the 1-argument form of IO::File->new().

perlcritic will complain about the 2-argument form of open(), which the 1-argument form of IO::File->new() calls, even on the gentlest perlcritic setting:

  http://goo.gl/xoQq3r

I realize that the previous version of the code also used the 1-argument form, but it looks like the Autom4te::XFile implementation that comes with Autoconf 2.69 was changed to allow use of the 2-argument form:

  https://lists.gnu.org/archive/html/automake-patches/2012-03/msg00111.html

> my ($sym) = /^\#\s*\w+\s+(\w+)/

One of the things perlcritic --harsh will insist on are ‘x’ modifiers on regexes, and this RE is a good reason why.  It’s complex enough to benefit from comments:

    my ($sym) = m{
        ^\#         # a comment starting at the beginning of the line
        \s*\w+      # followed by optional whitespace and a word
        \s+         # followed by mandatory whitespace
        (\w+)       # and the symbol we actually care about
    }x or next;

I’m guessing that this is matching a comment of some kind, and “word” on the second RE line should be replaced with a more specific term.  Again, I don’t really know what this code is trying to accomplish.
_______________________________________________
Autoconf mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/autoconf
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] autoheader: check templates of all config headers

Daniel Elstner
Hi,

sorry for taking so long to get back to this. But I see a release is
imminent, so I figured better late than never.

I've updated my patch and rebased it onto current master. I would be
cool if it could make the release.

On Tue, 2015-12-15 at 18:13 -0700, Warren Young wrote:
> On Oct 9, 2015, at 10:48 PM, Paul Eggert <[hidden email]> wrote:
> >
> > The basic idea for this sounds good; thanks.  It'd be nice if
> > someone else who uses Perl more than I could look over the details.
>
> Well, I know Perl, but I don’t know the autotools internals, so all I
> can do is nitpick style details.  If that’s all you want, then:

OK, I addressed some of these issues in my updated and rebased patch.

> > sub templates_for_header ($)
> Don’t use subroutine prototypes.

Got rid of it.

> > return @templates if (@templates);
> Just a nit, but I typically leave the parens off when using the
> postfix form of “if”, especially when the expression is simple, as in
> this case.

Done.

> > my $in = new Autom4te::XFile ("< " . open_quote ($template));
>
> The space in that string literal looks like you’re trying to avoid
> some of the problems with the 1-argument form of IO::File-
> new().  Unless $template can be a dash to open STDIN, there is no
> good reason to use the 1-argument form of IO::File->new().

Agreed, and it turns out that it has already been changed to the two-
argument form in current master. I followed suit in my updated patch.

> > my ($sym) = /^\#\s*\w+\s+(\w+)/
> One of the things perlcritic --harsh will insist on are ‘x’ modifiers
> on regexes, and this RE is a good reason why.

Left it as is for now as I just took it over from the original code.

Cheers,
--Daniel

_______________________________________________
Autoconf mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/autoconf

0001-autoheader-check-templates-of-all-config-headers.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] autoheader: check templates of all config headers

Eric Blake-3
On 12/21/2016 10:15 AM, Daniel Elstner wrote:
> Hi,
>
> sorry for taking so long to get back to this. But I see a release is
> imminent, so I figured better late than never.

Thank you so much for reposting!

My perl is not as strong as I would like, but your patch was at least
legible to me (perl makes it easier to do worse), and the idea makes
sense. The testsuite addition was a clincher.  I'm going ahead and
applying this, once it passes 'make check' (currently in progress on my
machine).

--
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


_______________________________________________
Autoconf mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/autoconf

signature.asc (617 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] autoheader: check templates of all config headers

Daniel Elstner
Hi Eric,

On Wed, 2016-12-21 at 10:31 -0600, Eric Blake wrote:
> Thank you so much for reposting!
>
> My perl is not as strong as I would like, but your patch was at least
> legible to me (perl makes it easier to do worse), and the idea makes
> sense. The testsuite addition was a clincher.  I'm going ahead and
> applying this, once it passes 'make check' (currently in progress on
> my machine).

Great, thanks! And I'm confident it will pass; it does for me at least.

Cheers,
--Daniel


_______________________________________________
Autoconf mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/autoconf
Loading...