When we add a file to the distribution, automake does not know about so it is not removed automatically by distclean. We should have a line in the makefile: DISTCLEANFILES = ChangeLog When running distcheck to verify the tarballs, it performs a VPATH build in a temporary _build directory. It then proceeds to clean it and verify it is empty. To do that, it invokes the distcleancheck, a command that is only available from a VPATH build. This command would normally print out an error because of ChangeLog still being present. The distcheck target then removes the whole VPATH build. It doesn't print an error at the moment because the CHANGELOG_CMD macro has overwritten the distcleancheck code to ignore the ChangeLog file. In the context of distcheck, it does not matter because the whole temp build is removed right after. There are 3 issues with overriding distcleancheck_listfiles: 1) It is not scalable. If we generate other files that need to be cleaned, the CHANGELOG_CMD would need to know about it which makes no sense. 2) Only the error is suppressed, the file is not removed 3) When running autoreconf -Wall, a warning is emitted about the fact that distcleancheck code has been overwritten. Given that there is a clean solution which does not have any of the 3 issues above, we should update the makefile appropriately. A side note about the generated tar files ----------------------------------------- When creating a VPATH build (so we can see what is going on during a distcheck), you will notice an error about the tar files left in the directory. This is an inconsistency in Automake. The distcheck target does explicitly remove the tar files before running distcleancheck so we don't see an error message when running distcheck. This is not an issue for us.
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
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.