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

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

Add a dependency on import/Makefile and config.h

Otherwise, since nothing else depends on that, the Makefile won't be
regenerated when necessary, which is problematic when importing new
gnulib modules.

gnulib/ChangeLog:

2019-11-12  Christian Biesinger  <cbiesinger@google.com>

	* Makefile.in: Depend on import/Makefile and config.h.

Change-Id: I6a2c4d41cf4f0e21d5c813197bad63ed5c08e408
---
M gnulib/Makefile.in
1 file changed, 2 insertions(+), 2 deletions(-)
  

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.
  

Christian Biesinger has posted comments on this change.

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


Patch Set 1:

> 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.
  

Christian Biesinger has posted comments on this change.

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


Patch Set 1:

> Patch Set 1:
> 
> > 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
  

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?
  

Christian Biesinger has posted comments on this change.

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


Patch Set 1:

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

Yeah, good point. That's actually really easy; I will upload a new version of this patch to do that in a second.
  

Simon Marchi has posted comments on this change.

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


Patch Set 2:

The file gnulib/aclocal-m4-deps.mk is not used anymore.  I believe that the automake-generated Makefile.in offers the feature that we did by hand using that file, so I think it's fine to just get rid of it.

I'm adding Tom as a reviewer, as he has some experience with Automake, I believe, so he might be able to spot some problems that I did not.
  

Christian Biesinger has posted comments on this change.

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


Patch Set 3:

> Patch Set 2:
> 
> The file gnulib/aclocal-m4-deps.mk is not used anymore.  I believe that the automake-generated Makefile.in offers the feature that we did by hand using that file, so I think it's fine to just get rid of it.

Thanks, I was wondering about that. Now deleted.

> I'm adding Tom as a reviewer, as he has some experience with Automake, I believe, so he might be able to spot some problems that I did not.

I did notice his name in "man automake" :)
  

Simon Marchi has posted comments on this change.

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


Patch Set 3:

(1 comment)

| --- /dev/null
| +++ gnulib/Makefile.am
PS3:

As silly as it sounds, this file probably needs a license header.
  

Simon Marchi has posted comments on this change.

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


Patch Set 4:

(1 comment)

| --- /dev/null
| +++ gnulib/Makefile.am
PS3:

Don't hesitate to mark comments as resolved once you have addressed
them.
  

Tom Tromey has posted comments on this change.

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


Patch Set 4:

(2 comments)

Thanks for doing this.  It looks good to me.  I think it needs a couple
of minor tweaks.

| --- gnulib/update-gnulib.sh
| +++ gnulib/update-gnulib.sh
| @@ -168,22 +168,17 @@ apply_patches ()
|          exit 1
|      fi
|  }
|  
|  apply_patches "patches/0001-Fix-PR-gdb-23558-Use-system-s-getcwd-when-cross-comp.patch"
|  apply_patches "patches/0002-mkostemp-mkostemps-Fix-compilation-error-in-C-mode-o.patch"
|  apply_patches "patches/0003-Fix-glob-c-Coverity-issues.patch"
|  
|  # Regenerate all necessary files...
|  aclocal -Iimport/m4 -I../config &&

PS4, Line 177:

I think if you need -I options to aclocal, you should also invoke
AC_CONFIG_MACRO_DIRS in configure.ac.  See
https://www.gnu.org/software/automake/manual/automake.html#Local-
Macros

|  autoconf &&
|  autoheader &&
| -automake
| +automake --foreign

PS4, Line 180:

Rather than write --foreign here, add `foreign` to the call to
`AM_INIT_AUTOMAKE` in `configure.ac`.

|  if [ $? -ne 0 ]; then
|     echo "Error: Failed to regenerate Makefiles and configure scripts."
|     exit 1
|  fi
| -
| -# Update aclocal-m4-deps.mk
| -ACLOCAL_M4_DEPS_FILE=aclocal-m4-deps.mk
| -cat > ${ACLOCAL_M4_DEPS_FILE}.tmp <<EOF
| -# THIS FILE IS GENERATED.  -*- buffer-read-only: t -*- vi :set ro:
  

Christian Biesinger has posted comments on this change.

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


Patch Set 5:

(2 comments)

| --- gnulib/update-gnulib.sh
| +++ gnulib/update-gnulib.sh
| @@ -168,22 +168,17 @@ apply_patches ()
|          exit 1
|      fi
|  }
|  
|  apply_patches "patches/0001-Fix-PR-gdb-23558-Use-system-s-getcwd-when-cross-comp.patch"
|  apply_patches "patches/0002-mkostemp-mkostemps-Fix-compilation-error-in-C-mode-o.patch"
|  apply_patches "patches/0003-Fix-glob-c-Coverity-issues.patch"
|  
|  # Regenerate all necessary files...
|  aclocal -Iimport/m4 -I../config &&

PS4, Line 177:

Thanks, done.

|  autoconf &&
|  autoheader &&
| -automake
| +automake --foreign

PS4, Line 180:

Ah, thanks, done.

|  if [ $? -ne 0 ]; then
|     echo "Error: Failed to regenerate Makefiles and configure scripts."
|     exit 1
|  fi
| -
| -# Update aclocal-m4-deps.mk
| -ACLOCAL_M4_DEPS_FILE=aclocal-m4-deps.mk
| -cat > ${ACLOCAL_M4_DEPS_FILE}.tmp <<EOF
| -# THIS FILE IS GENERATED.  -*- buffer-read-only: t -*- vi :set ro:
  

Tom Tromey has posted comments on this change.

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


Patch Set 5: Code-Review+2

Thank you for doing this.  This is ok.