[review] Add a dependency on import/Makefile and config.h

["Simon Marchi (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/622
......................................................................


Patch Set 1:

> > > Can you explain quickly the logic of this?  I am not completely aware of what generates what in this system, so if you could explain at high level what happens between importing a new gnulib module, and the Makefile/config.h getting re-generated, it would help.
> > 
> > Sure. So if a new module is imported, it will likely change what #defines get defined, which means config.h needs to be regenerated. Also, import/Makefile.in sets various variables similar to those defines such as REPLACE_STRERROR_R, and so it needs to be regenerated as well (those variables are used to generate headers like string.h)
> > 
> > But nothing currently ensures that those two get regenerated. Hence these new dependencies. all-lib was already depending on import/Makefile, presumably for this purpose, but does not seem to be used.
> 
> Sorry, to clarify, these are the steps that cause issues:
> - Have an existing GDB build <-- important
> - Run update-gnulib
> - Run make in your build directory <-- this is now using the old config.h and import/Makefile

Ok, so I'm able to reproduce the problem.  However, I'm not convinced yet your solution is the best way (although it works).  I was really wondering why import/Makefile wasn't getting magically re-generated, since it's managed by automake, and it led me to the solution explained below.

It's a little weird, because this is an automake project, where the top-level Makefile isn't managed by automake.  But we re-invent a bunch of things that automake does.

I "isolated" the problem to this simpler case that usually works in a normal automake project, but does not work here:

 $ pwd
 /home/simark/build/binutils-gdb/gnulib/import
 $ touch ../config.status /home/simark/src/binutils-gdb/gnulib/import/Makefile.in
 $ make Makefile
 make[1]: Entering directory '/home/simark/build/binutils-gdb/gnulib'
 # Regenerate the Makefile.
 CONFIG_FILES="Makefile" \
   CONFIG_COMMANDS= \
   CONFIG_HEADERS= \
   /bin/sh config.status
 config.status: creating Makefile
 make[1]: Leaving directory '/home/simark/build/binutils-gdb/gnulib'
 make: 'Makefile' is up to date.

When both config.status and import/Makefile.in are newer than import/Makefile, make goes into the top-level directory and seemingly re-generates only the top-level Makefile.  That looks weird and incorrect.  import/Makefile is still not re-generated.

The rule in import/Makefile to re-generate itself is:

  Makefile: $(srcdir)/Makefile.in $(top_builddir)/config.status
          @case '$?' in \
            *config.status*) \
              cd $(top_builddir) && $(MAKE) $(AM_MAKEFLAGS) am--refresh;; \
            *) \
              echo ' cd $(top_builddir) && $(SHELL) ./config.status $(subdir)/$@ $(am__depfiles_maybe)'; \
              cd $(top_builddir) && $(SHELL) ./config.status $(subdir)/$@ $(am__depfiles_maybe);; \
          esac;

Essentially, when import/Makefile is older than config.status, I guess it's the sign that all config files are outdated, so it calls the am--refresh target of the top-level Makefile.

This rule does nothing, but because of some magic I don't understand, the top-level Makefile is also checked against it dependencies.  Because it is older than config.status, it is rebuilt using

 Makefile: Makefile.in config.status
    # Regenerate the Makefile.
    CONFIG_FILES="Makefile" \
      CONFIG_COMMANDS= \
      CONFIG_HEADERS= \
      $(SHELL) config.status

And this only rebuilds the top-level Makefile, which is the behavior we observe.

I looked at a top-level Makefile generated by automake, and this is what I found:

  Makefile: $(srcdir)/Makefile.in $(top_builddir)/config.status
          @case '$?' in \
            *config.status*) \
              echo ' $(SHELL) ./config.status'; \
              $(SHELL) ./config.status;; \
            *) \
              echo ' cd $(top_builddir) && $(SHELL) ./config.status $@ $(am__maybe_remake_depfiles)'; \
              cd $(top_builddir) && $(SHELL) ./config.status $@ $(am__maybe_remake_depfiles);; \
          esac;

When the top-level Makefile is re-generated and older than config.status, it re-generates all the config files, not just itself.

So if we copy automake's behavior, it should work (at least my local testing here seemed to work).  When you run update-gnulib.sh, then type "make" in the top-level (we generally don't type make in import/, so this is a more realistic scenario), it will re-generate configure, which will cause config.status to get re-generated, which will cause the top-level Makefile to get re-generated.  And if that rules re-generates all config files (including import/Makefile and config.h), we are fine.

Writing all this made me think: could we just write that top-level Makefile using automake and stop trying to re-invent it?


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I6a2c4d41cf4f0e21d5c813197bad63ed5c08e408
Gerrit-Change-Number: 622
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Thu, 14 Nov 2019 05:36:24 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment