Bug 24278

I just looked at the commit that introduced this line and the commit message says
(commit 55e8d740881ef622376440819119641e67aeb285):

    fix distcheck target

    Arrange that distcleancheck ignores ChangeLog left over by distclean.

    Don't mention ChangeLog under *CLEANFILES, can't be rebuilt from release
    tarball; ChangeLog is automatically distributed, no need to mention it.

The last point is the important one - since we generate the changelog from git, we can't re-generate it once removed. So the question - is it a reasonable assumption that people run make distclean from a tarball?

The way around this would be to generate a Changelog.in from git and move that into the real Changelog so we can't lose it in the tarball. This seems to be a messy though, doesn't it?

(In reply to comment #1)
> I just looked at the commit that introduced this line and the commit message
> says
> (commit 55e8d740881ef622376440819119641e67aeb285):
> 
>     fix distcheck target
> 
>     Arrange that distcleancheck ignores ChangeLog left over by distclean.
> 
>     Don't mention ChangeLog under *CLEANFILES, can't be rebuilt from release
>     tarball; ChangeLog is automatically distributed, no need to mention it.
> 
> The last point is the important one - since we generate the changelog from git,
> we can't re-generate it once removed. So the question - is it a reasonable
> assumption that people run make distclean from a tarball?
> 
Ah, it just hit me. This is why all Makefile.am have MAINTAINERCLEANFILES = ChangeLog. This target is designed for generated files that should not be removed from tarballs by "ordinary user" because they may not have the tools, namely git, to rebuild it. As long as AM_MAINTAINER_MODE is set in the configure.ac. 

Are there people who "make dist" from a tar ball? Good question.

> The way around this would be to generate a Changelog.in from git and move that
> into the real Changelog so we can't lose it in the tarball. This seems to be a
> messy though, doesn't it?
> 

It's in the spirit of Autoconf. *.pc.in --> *.pc. Server also generate headers using this pattern.

I have a few things to try.

I found the problem. It was nowhere near where I was looking.

First, a preamble. ChangeLog is one of the 43 files that automake automatically distribute. There is no need to add it to EXTRA_DIST, for example. The dist-hook isn't required either. Thee are some behaviours already associated with this file. Take the NEWS file for example, it just exists and gets distributed.

In the makefile, we blindly create ChangeLog every time we run the dist target. It now behaves more like object code. It now has an additional personality. This is more evident in a VPATH build where objects are kept in a separate directory. You end up with 2 ChangeLog files. The original one that simply existed at the time of the tarball creation, and the newly generated one (zero byte, no git) that is sitting in the object dir. 

It's easy to prove the point. Just remove the ".PHONEY ChangeLog" from the Makefile. It gets created once when X.Org builds for the first time. From now on it behaves like the NEWS file. It gets included in the tarball and never gets created again. There is nothing magic here, the makefile is just a way to get the file initially created as if it were git checked-out. 

I'll perform some more testing. We don't need any of the workarounds we have now. We only need:
ChangeLog:
	$(CHANGELOG_CMD)

No EXTRA_DIST = ChangeLog
No *CLEANFILES = ChangeLog
No .PHONEY ChangeLog
No dist-hook: ChangeLog
No AC_SUBST([distcleancheck_listfiles], ['find . -type f ! -name ChangeLog -print'])

A side note about Debian distro. They run autoreconf -vfi most likely because of the patches they must include. This is equivalent to our autogen.sh. I tested that as well for ChangeLog and it's ok.

I have found a better solution. ChangeLog should not be created by the Makefile, it should be created alongside other generated files like Makefile at configure time. Autoconf has a feature just for that. We need one line in configure.ac:

AC_CONFIG_COMMANDS([ChangeLog], [eval $CHANGELOG_CMD], [CHANGELOG_CMD=$CHANGELOG_CMD])

We could use the same CHANGELOG_CMD, but we still have the requirement that it must be created only once (during tarball generation). The macro is modified to generate only if it does not already exist.

AC_DEFUN([XORG_CHANGELOG], [
CHANGELOG_CMD="\"if test ! -f ChangeLog; then \
(git --git-dir=$srcdir/.git log > .changelog.tmp && mv .changelog.tmp ChangeLog) \
|| (rm -f .changelog.tmp; touch ChangeLog; \
echo 'git directory not found: installing possibly empty changelog.' >&2);fi\""
AC_SUBST([CHANGELOG_CMD])
]) # XORG_CHANGELOG

Because the command is stored in a shell variable, it needs an extra protected quoting \". Because it is executed in a shell rather than in make, it needs "eval" (mainly due to redirect >). Based on what I read in Autoconf and in the configure script, this is portable code. 

Also, at config time, very few variables are available. $srcdir is available and is designed for that purpose. On a normal build $srcdir expands to "." but on a VPATH build (used in distcheck) it expands to "..".

This will take the ChangeLog completely off the Makefile. It will get distributed because of the default rules (just like NEWS or README).

The output of ./configure looks like this:
configure: creating ./config.status
config.status: creating Makefile
config.status: creating config.h
config.status: config.h is unchanged
config.status: executing ChangeLog commands
config.status: executing depfiles commands

If I change the git command in config.status to simulate an error, we get:
config.status: executing ChangeLog commands
./config.status: line 1085: kgit: command not found
git directory not found: installing possibly empty changelog.

I guess a refinement could be to generate it if it is missing or it has zero byte. The tarball builder may have had a glitch with git which is now fixed. So he needs to overwrite those zero byte ChangeLog files.

In Comment #3, I thought I had found the problem. It's even simpler. I does not matter who or how ChangeLog is created, but "where". There is a hint in the macro: $(top_srcdir). 

CHANGELOG_CMD="(GIT_DIR=\$(top_srcdir)/.git git log > .changelog.tmp && \
mv .changelog.tmp ChangeLog) || (rm -f .changelog.tmp; touch ChangeLog; \
echo 'git directory not found: installing possibly empty changelog.' >&2)"

A distcheck is performed in a VPATH build which is one level below the srcdir. To find the git repo, srcdir is pointing one level up. However the files are created in cwd, which is where the generated objects are put.

Proceeding all filenames in the macro with \$(top_srcdir) fixes all problems. The method of creating the file from configure.ac is a separate idea and isn't a solution to the current problem.

In it's basic form, the solution is to fix the macro with $(top_srcdir) and remove the distcleancheck_listfiles workaround. The recursive tarball building scenario now works and there is now way of deleting ChangeLog (short of deleting manually). 

That sounds sane and is afaict what the CHANGELOG_CMD intends to do anyway -
even if it doesn't actually do so. Care to prepare a patch?

Created attachment 30232 [details] [review]
[PATCH] ChangeLog: generated file not cleaned from the dist directory

I'll open a separate bug for cleaning-up Makefile.am. Thanks for reviewing.

I don't think the comment should change the minimum version - that's
supposed to indicate the lowest version which has a macro, not the
last time it was changed.   Otherwise, the patch seems reasonable to me.

Created attachment 30233 [details] [review]
[PATCH] ChangeLog: generated file not cleaned from the dist directory

Revert change to version comment. Thanks

Typo: "writting" should be "writing".

Provided the hunk bumping the version number is removed, ACK from me.
I think we should bump in a separate commit after checking that there isn't
anything else we need to push into 1.3.1.

Created attachment 30289 [details] [review]
[PATCH] ChangeLog: generated file not cleaned from the dist directory

Fix typo and revert bumping mod level.Also remove a one line comment .

Fixed in commit f8695cf7b892028bf7c502e85f26f0a756edd316