Discussion:
[PATCH] "make dist" did not depend on $(BUILT_SOURCES)
Jim Meyering
2017-11-28 19:21:11 UTC
Permalink
I was dismayed to discover this patch I wrote over three years ago.
Today I confirmed that the problem still remains by running these commands:

git clone git://git.sv.gnu.org/hello.git \
cd hello && ./bootstrap && ./configure && env make dist

It failed like this:

src/hello.c:27:10: fatal error: configmake.h: No such file or directory
#include "configmake.h"
^~~~~~~~~~~~~~

Here's the patch I expect to push to master:

From: Jim Meyering <***@fb.com>
Date: Thu, 20 Mar 2014 12:31:32 -0700
Subject: [PATCH] "make dist" did not depend on $(BUILT_SOURCES)

* lib/am/distdir.am (distdir-am): New intermediate target.
Interpose this target between $(distdir) and its dependency
on $(DISTFILES), so that we can ensure $(BUILT_SOURCES) are
all created before we begin creating $(DISTFILES).
* t/dist-vs-built-sources.sh: Test for this.
* t/list-of-tests.mk (handwritten_TESTS): Add it.
* NEWS (Bugs fixed): Mention it.
Assaf Gordon reported that "make dist" (after ./configure
from a pristine clone of GNU hello) would fail due to the
absence of configmake.h while compiling lib/localcharset.c.
https://lists.gnu.org/r/bug-hello/2014-03/msg00016.html
---
NEWS | 3 +++
lib/am/distdir.am | 7 ++++--
t/dist-vs-built-sources.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++++
t/list-of-tests.mk | 1 +
4 files changed, 65 insertions(+), 2 deletions(-)
create mode 100644 t/dist-vs-built-sources.sh

diff --git a/NEWS b/NEWS
index dd057b7b1..7d52aeb93 100644
--- a/NEWS
+++ b/NEWS
@@ -113,6 +113,9 @@ New in ?.?.?:
- Installed 'aclocal' m4 macros can now accept installation directories
containing '@' characters (automake bug#20903)

+ - "./configure && make dist" no longer fails when a distributed file depends
+ on one from BUILT_SOURCES.
+
- When combining AC_LIBOBJ or AC_FUNC_ALLOCA with the
"--disable-dependency-tracking" configure option in an out of source
build, the build sub-directory defined by AC_CONFIG_LIBOBJ_DIR is now
diff --git a/lib/am/distdir.am b/lib/am/distdir.am
index 653966f0e..4b6543591 100644
--- a/lib/am/distdir.am
+++ b/lib/am/distdir.am
@@ -72,10 +72,13 @@ endif %?SUBDIRS%

.PHONY: distdir
if %?SUBDIRS%
-AM_RECURSIVE_TARGETS += distdir
+AM_RECURSIVE_TARGETS += distdir distdir-am
endif %?SUBDIRS%

-distdir: $(DISTFILES)
+distdir: $(BUILT_SOURCES)
+ $(MAKE) $(AM_MAKEFLAGS) distdir-am
+
+distdir-am: $(DISTFILES)
##
## For Gnits users, this is pretty handy. Look at 15 lines
## in case some explanatory text is desirable.
diff --git a/t/dist-vs-built-sources.sh b/t/dist-vs-built-sources.sh
new file mode 100644
index 000000000..94f8b600f
--- /dev/null
+++ b/t/dist-vs-built-sources.sh
@@ -0,0 +1,56 @@
+#! /bin/sh
+# Copyright (C) 2017 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2, or (at your option)
+# any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <https://www.gnu.org/licenses/>.
+
+# Ensure that "make dist" no longer fails when a distributed file
+# depends on a file from the list of BUILT_SOURCES.
+
+. test-init.sh
+
+cat >> configure.ac << 'END'
+AC_PROG_CC
+AC_OUTPUT
+END
+
+cat > Makefile.am << 'END'
+BUILT_SOURCES = h.h
+h.h:
+ rm -f $@ $@-t
+ printf '%s\n' '#define F "F"' > $@-t
+ mv -f $@-t $@
+CLEANFILES = h.h
+
+EXTRA_DIST = gen
+gen: foo
+ ./foo > $@-t && mv $@-t $@
+
+bin_PROGRAMS = foo
+foo_SOURCES = foo.c
+END
+
+cat > foo.c << 'END'
+#include "h.h"
+int main (void) { printf ("%s\n", F); return 0; }
+END
+chmod a-w foo.c
+
+$ACLOCAL
+$AUTOCONF
+$AUTOMAKE
+
+./configure
+$MAKE dist
+
+:
diff --git a/t/list-of-tests.mk b/t/list-of-tests.mk
index 33e5adc49..fde769971 100644
--- a/t/list-of-tests.mk
+++ b/t/list-of-tests.mk
@@ -409,6 +409,7 @@ t/dist-missing-m4.sh \
t/dist-readonly.sh \
t/dist-repeated.sh \
t/dist-pr109765.sh \
+t/dist-vs-built-sources.sh \
t/distcleancheck.sh \
t/distcom2.sh \
t/distcom3.sh \
--
2.14.1.729.g59c0ea183
Mathieu Lirzin
2017-11-28 20:01:09 UTC
Permalink
Hello Jim,
Post by Jim Meyering
I was dismayed to discover this patch I wrote over three years ago.
git clone git://git.sv.gnu.org/hello.git \
cd hello && ./bootstrap && ./configure && env make dist
src/hello.c:27:10: fatal error: configmake.h: No such file or directory
#include "configmake.h"
^~~~~~~~~~~~~~
Date: Thu, 20 Mar 2014 12:31:32 -0700
Subject: [PATCH] "make dist" did not depend on $(BUILT_SOURCES)
* lib/am/distdir.am (distdir-am): New intermediate target.
Interpose this target between $(distdir) and its dependency
on $(DISTFILES), so that we can ensure $(BUILT_SOURCES) are
all created before we begin creating $(DISTFILES).
* t/dist-vs-built-sources.sh: Test for this.
* t/list-of-tests.mk (handwritten_TESTS): Add it.
* NEWS (Bugs fixed): Mention it.
Assaf Gordon reported that "make dist" (after ./configure
from a pristine clone of GNU hello) would fail due to the
absence of configmake.h while compiling lib/localcharset.c.
https://lists.gnu.org/r/bug-hello/2014-03/msg00016.html
---
NEWS | 3 +++
lib/am/distdir.am | 7 ++++--
t/dist-vs-built-sources.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++++
t/list-of-tests.mk | 1 +
4 files changed, 65 insertions(+), 2 deletions(-)
create mode 100644 t/dist-vs-built-sources.sh
OK to push.

Thanks.
--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37
Jim Meyering
2017-11-29 03:43:15 UTC
Permalink
Post by Mathieu Lirzin
Hello Jim,
...
...
Post by Mathieu Lirzin
OK to push.
Thanks. Pushed.
I have also removed the micro branch.
Nick Bowler
2017-11-28 20:45:30 UTC
Permalink
Hi Jim,
Post by Jim Meyering
Date: Thu, 20 Mar 2014 12:31:32 -0700
Subject: [PATCH] "make dist" did not depend on $(BUILT_SOURCES)
* lib/am/distdir.am (distdir-am): New intermediate target.
Interpose this target between $(distdir) and its dependency
on $(DISTFILES), so that we can ensure $(BUILT_SOURCES) are
all created before we begin creating $(DISTFILES).
[...]
Post by Jim Meyering
NEWS | 3 +++
lib/am/distdir.am | 7 ++++--
t/dist-vs-built-sources.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++++
t/list-of-tests.mk | 1 +
4 files changed, 65 insertions(+), 2 deletions(-)
create mode 100644 t/dist-vs-built-sources.sh
The Automake manual unequivocally states that BUILT_SOURCES files are
generated only when running 'make all', 'make check' or 'make install'.

So if they are going to be generated on 'make dist' as well, then the
manual needs a corresponding update.

Otherwise this looks like a useful improvement.

Cheers,
Nick
Jim Meyering
2017-11-29 02:13:16 UTC
Permalink
Post by Nick Bowler
Hi Jim,
Post by Jim Meyering
Date: Thu, 20 Mar 2014 12:31:32 -0700
Subject: [PATCH] "make dist" did not depend on $(BUILT_SOURCES)
* lib/am/distdir.am (distdir-am): New intermediate target.
Interpose this target between $(distdir) and its dependency
on $(DISTFILES), so that we can ensure $(BUILT_SOURCES) are
all created before we begin creating $(DISTFILES).
[...]
Post by Jim Meyering
NEWS | 3 +++
lib/am/distdir.am | 7 ++++--
t/dist-vs-built-sources.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++++
t/list-of-tests.mk | 1 +
4 files changed, 65 insertions(+), 2 deletions(-)
create mode 100644 t/dist-vs-built-sources.sh
The Automake manual unequivocally states that BUILT_SOURCES files are
generated only when running 'make all', 'make check' or 'make install'.
So if they are going to be generated on 'make dist' as well, then the
manual needs a corresponding update.
Hi Nick,
Thanks for the suggestion, but I do not think it is desired. "make
dist" is already defined as building everything that goes into the
distribution tarball, and that implies it must also build anything
(e.g., from BUILT_SOURCES) that happens to be required to do that.
Perhaps more importantly, this is an implementation detail that feels
like it should not be made part of the contract that the documentation
provides, in case some day automake tightens up "make dist" so it
builds only those BUILT_SOURCES files that are actually required to
build the tarball components.
Nick Bowler
2017-11-29 04:35:37 UTC
Permalink
Post by Jim Meyering
Post by Nick Bowler
The Automake manual unequivocally states that BUILT_SOURCES files are
generated only when running 'make all', 'make check' or 'make install'.
So if they are going to be generated on 'make dist' as well, then the
manual needs a corresponding update.
Hi Nick,
Thanks for the suggestion, but I do not think it is desired. "make
dist" is already defined as building everything that goes into the
distribution tarball, and that implies it must also build anything
(e.g., from BUILT_SOURCES) that happens to be required to do that.
I agree that it *should* but not that it *must*, because BUILT_SOURCES
explicitly (by design) bypasses the usual prerequisite mechanisms.

If you use normal prerequisites instead of BUILT_SOURCS everything
works just fine wrt. distribution, of course, and is the approach I
would personally recommend in all cases.
Post by Jim Meyering
Perhaps more importantly, this is an implementation detail that feels
like it should not be made part of the contract that the documentation
provides ...
But now with the change applied, as it stands the documentation is
simply wrong. For example, this passage from the manual (§9.4 Built
Sources):

"... BUILT_SOURCES is honored only by ‘make all’, ‘make check’ and
‘make install’."

is no longer true. This error can be corrected without explicitly
documenting the new behaviour, for example by making the list of
targets non-exhaustive in nature.

Perhaps something like:

... BUILT_SOURCES is honored only by certain targets, including ‘make
all’, ‘make check’ and ‘make install’.

Although not mentioning distribution at all means that someone reading
this section is left to figure out for themselves if and how these two
Automake features work together...
Post by Jim Meyering
... in case some day automake tightens up "make dist" so it builds
only those BUILT_SOURCES files that are actually required to build
the tarball components.
There is need to worry about this ever happening, because computing
such a subset of BUILT_SOURCES is impossible in general.

Cheers,
Nick
Jim Meyering
2017-11-29 04:42:49 UTC
Permalink
Post by Nick Bowler
Post by Jim Meyering
Post by Nick Bowler
The Automake manual unequivocally states that BUILT_SOURCES files are
generated only when running 'make all', 'make check' or 'make install'.
So if they are going to be generated on 'make dist' as well, then the
manual needs a corresponding update.
Hi Nick,
Thanks for the suggestion, but I do not think it is desired. "make
dist" is already defined as building everything that goes into the
distribution tarball, and that implies it must also build anything
(e.g., from BUILT_SOURCES) that happens to be required to do that.
I agree that it *should* but not that it *must*, because BUILT_SOURCES
explicitly (by design) bypasses the usual prerequisite mechanisms.
If you use normal prerequisites instead of BUILT_SOURCS everything
works just fine wrt. distribution, of course, and is the approach I
would personally recommend in all cases.
Post by Jim Meyering
Perhaps more importantly, this is an implementation detail that feels
like it should not be made part of the contract that the documentation
provides ...
But now with the change applied, as it stands the documentation is
simply wrong. For example, this passage from the manual (§9.4 Built
"... BUILT_SOURCES is honored only by ‘make all’, ‘make check’ and
‘make install’."
is no longer true. This error can be corrected without explicitly
documenting the new behaviour, for example by making the list of
targets non-exhaustive in nature.
... BUILT_SOURCES is honored only by certain targets, including ‘make
all’, ‘make check’ and ‘make install’.
Thanks for keeping us honest.
That sounds reasonable. Send a patch?

Loading...