| Message ID | patch-1.1-911881ce19f-20210702T115617Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Documentation/Makefile: don't re-build on 'git version' changes | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Since 9a71722b4df (Doc: auto-detect changed build flags, 2019-03-17) > we've been eagerly re-building the documentation whenever the output > of "git version" (via the GIT-VERSION file) changed. This was never > the intention, and was a regression on what we intended in > 7b8a74f39cb (Documentation: Replace @@GIT_VERSION@@ in documentation, > 2007-03-25). I am not sure. Even if there were no changes in say 'Documentation/git-cat-file.txt' and the sources it depends on between 'master' and 'next', after doing this: $ git checkout next $ make prefix=$HOME/git-next/ install install-doc $ git checkout master $ make prefix=$HOME/git-master/ install install-doc $ $HOME/git-master/bin/git help cat-file | tail -n 1 I should see that the documentation should say it is from the 'master' branch in its footer, no? In other words, I think 7b8a74f39cb's reasoning (not the implementation), especially the last sentence of its log message, is flawed, where it said: Documentation: Replace @@GIT_VERSION@@ in documentation Include GIT-VERSION-FILE and replace @@GIT_VERSION@@ in the HTML and XML asciidoc output. The documentation doesn't depend on GIT-VERSION-FILE so it will not be automatically rebuild if nothing else changed. Thanks.
Ævar Arnfjörð Bjarmason wrote: > Since 9a71722b4df (Doc: auto-detect changed build flags, 2019-03-17) > we've been eagerly re-building the documentation whenever the output > of "git version" (via the GIT-VERSION file) changed. This was never > the intention, and was a regression on what we intended in > 7b8a74f39cb (Documentation: Replace @@GIT_VERSION@@ in documentation, > 2007-03-25). > > So let's add an ASCIIDOC_MANVERSION variable that we exclude from > ASCIIDOC_COMMON. The change in 9a71722b4df was only intending to catch > cases where we e.g. switched between asciidoc and asciidoctor, not to > undo the logic in 7b8a74f39cb and force a re-build every time our HEAD > changed in the repository. Once again, why do we care that the version is 2.32.0.98.gcfb60a24d6 and not 2.32.0.97.g949e814b27? Not just in the documentation, but everywhere. Maybe we can add a GIT_RELEASE variable that unlike GIT_VERSION it doesn't contain the precise commit. For example GIT_RELEASE = 2.33-dev.
On Fri, Jul 02 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Since 9a71722b4df (Doc: auto-detect changed build flags, 2019-03-17) >> we've been eagerly re-building the documentation whenever the output >> of "git version" (via the GIT-VERSION file) changed. This was never >> the intention, and was a regression on what we intended in >> 7b8a74f39cb (Documentation: Replace @@GIT_VERSION@@ in documentation, >> 2007-03-25). > > I am not sure. Even if there were no changes in say > 'Documentation/git-cat-file.txt' and the sources it depends on > between 'master' and 'next', after doing this: > > $ git checkout next > $ make prefix=$HOME/git-next/ install install-doc > $ git checkout master > $ make prefix=$HOME/git-master/ install install-doc > $ $HOME/git-master/bin/git help cat-file | tail -n 1 > > I should see that the documentation should say it is from the > 'master' branch in its footer, no? Yes in theory, in practice it's very annoying to have the very slow documentation build be re-built so aggressively. Since it wasn't a practical issue anyone worried about before 2019 I think it's worth reverting it. > In other words, I think 7b8a74f39cb's reasoning (not the > implementation), especially the last sentence of its log message, is > flawed, where it said: > > Documentation: Replace @@GIT_VERSION@@ in documentation > > Include GIT-VERSION-FILE and replace @@GIT_VERSION@@ in > the HTML and XML asciidoc output. The documentation > doesn't depend on GIT-VERSION-FILE so it will not be > automatically rebuild if nothing else changed. Arguably it's a feature. The point of the version in the documentation is to make it clear what version we're discussing. If I build something on the master SHA-1 and advance to next, and none of the documentation dependencies change, it's most useful to refer to the oldest last version we can cover. I think nobody's doing such a "chained" build when building the docs for a "real" release, and having mixed versions might be confusing, but for the "local build" case from a development checkout it's arguably more useful.
On Fri, Jul 02 2021, Felipe Contreras wrote: > Ævar Arnfjörð Bjarmason wrote: >> Since 9a71722b4df (Doc: auto-detect changed build flags, 2019-03-17) >> we've been eagerly re-building the documentation whenever the output >> of "git version" (via the GIT-VERSION file) changed. This was never >> the intention, and was a regression on what we intended in >> 7b8a74f39cb (Documentation: Replace @@GIT_VERSION@@ in documentation, >> 2007-03-25). >> >> So let's add an ASCIIDOC_MANVERSION variable that we exclude from >> ASCIIDOC_COMMON. The change in 9a71722b4df was only intending to catch >> cases where we e.g. switched between asciidoc and asciidoctor, not to >> undo the logic in 7b8a74f39cb and force a re-build every time our HEAD >> changed in the repository. > > Once again, why do we care that the version is 2.32.0.98.gcfb60a24d6 and > not 2.32.0.97.g949e814b27? > > Not just in the documentation, but everywhere. It's useful e.g. on my Debian system to see that the "next" Debian packaged is 2.31.0.291.g576ba9dcdaf in docs & "git version", arguably less so for documentation. But yeah, I think we should at least have some opt-out for sticking the exact version everywhere, given the mostly unless re-building it does during development. > Maybe we can add a GIT_RELEASE variable that unlike GIT_VERSION it > doesn't contain the precise commit. For example GIT_RELEASE = 2.33-dev. I just added this to my pre-make script: grep -q ^/version .git/info/exclude || echo /version >>.git/info/exclude echo $(grep -o -P '(?<=^DEF_VER=v).*' GIT-VERSION-GEN)-dev >version It makes use of GIT-VERSION-GEN picking up a tarball "version" file. So far it seems like Junio isn't interested in the patch, and in any case it doesn't fix the more general over-rebuilding due to version changes noted upthread when it comes to *.o and libgit. Doing this fixes both for me. Then when I build an installed version I don't use that trick.
Ævar Arnfjörð Bjarmason wrote: > > On Fri, Jul 02 2021, Felipe Contreras wrote: > > > Ævar Arnfjörð Bjarmason wrote: > >> Since 9a71722b4df (Doc: auto-detect changed build flags, 2019-03-17) > >> we've been eagerly re-building the documentation whenever the output > >> of "git version" (via the GIT-VERSION file) changed. This was never > >> the intention, and was a regression on what we intended in > >> 7b8a74f39cb (Documentation: Replace @@GIT_VERSION@@ in documentation, > >> 2007-03-25). > >> > >> So let's add an ASCIIDOC_MANVERSION variable that we exclude from > >> ASCIIDOC_COMMON. The change in 9a71722b4df was only intending to catch > >> cases where we e.g. switched between asciidoc and asciidoctor, not to > >> undo the logic in 7b8a74f39cb and force a re-build every time our HEAD > >> changed in the repository. > > > > Once again, why do we care that the version is 2.32.0.98.gcfb60a24d6 and > > not 2.32.0.97.g949e814b27? > > > > Not just in the documentation, but everywhere. > > It's useful e.g. on my Debian system to see that the "next" Debian > packaged is 2.31.0.291.g576ba9dcdaf in docs & "git version", arguably > less so for documentation. Obviously packagers would use real versions I meant this only for developers (e.g. DEVELOPER=1). And BTW, where is that Debian package coming from? I thought distributions didn't package git pre-releases. > > Maybe we can add a GIT_RELEASE variable that unlike GIT_VERSION it > > doesn't contain the precise commit. For example GIT_RELEASE = 2.33-dev. > > I just added this to my pre-make script: > > grep -q ^/version .git/info/exclude || echo /version >>.git/info/exclude > echo $(grep -o -P '(?<=^DEF_VER=v).*' GIT-VERSION-GEN)-dev >version > > It makes use of GIT-VERSION-GEN picking up a tarball "version" file. This would also do the trick (you need DEVELOPER on the environment though). --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -6,9 +6,10 @@ DEF_VER=v2.32.0 LF=' ' -# First see if there is a version file (included in release tarballs), -# then try git-describe, then default. -if test -f version +if test -n "$DEVELOPER" +then + VN="$DEF_VER"-dev +elif test -f version then VN=$(cat version) || VN="$DEF_VER" elif test -d ${GIT_DIR:-.git} -o -f .git &&
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > It's useful e.g. on my Debian system to see that the "next" Debian > packaged is 2.31.0.291.g576ba9dcdaf in docs & "git version", arguably > less so for documentation. If it is arguable, perhaps make an argument that is more convincing? What I dislike the most is that in the sample scenario where master and next has the same documentation material to build "git-cat-file.1", the installed result would be different depending on the order of building the documentation, with the change being discussed, i.e. $ git checkout master && make prefix=$HOME/git-master install-doc $ git checkout next && make prefix=$HOME/git-next install-doc would make "~/git-next/bin/git help cat-file" to claim the documentation is from the "master" version. Which is not all that bad, given that there wasn't anything that changed the documentation between 'master' and 'next'. But if you swap the installation order, "~/git-master/bin/git help cat-file" would say that the documentation is from a version much newer than 'master', which is not quite acceptable. I am OK with the approach you hinted to have an option to _hide_ the version string in the generated documentation (hence they lose their dependency on GIT-VERSION-FILE), while keeping the dependency of version.o on GIT-VERSION-FILE, so that something goes wrong in a built binary, the developer can still ask "git version" to identify where the binary came from. Thanks.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I think nobody's doing such a "chained" build when building the docs for > a "real" release, and having mixed versions might be confusing, but for > the "local build" case from a development checkout it's arguably more > useful. Well, when I prepare a cascade of maintenance releases for CVE, what do you think is done? Yes, I do have "make distclean" in the loop that iterates over the maint-$version branches, just to be extra careful, but I shouldn't have to.
Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I am OK with the approach you hinted to have an option to _hide_ the > version string in the generated documentation (hence they lose their > dependency on GIT-VERSION-FILE), while keeping the dependency of > version.o on GIT-VERSION-FILE, so that something goes wrong in a > built binary, the developer can still ask "git version" to identify > where the binary came from. But it would be even better if the build system knew what was the intent of the build. There's a difference when building a revision just to run `make test` on it, and building it to do `make install`. In the former there's no need for a real version in version.o, only in the latter. Maybe RELASE=1. Not a release from the point of view of a maintainer, but a release in the sense that the binaries will escape our in-tree build.
diff --git a/Documentation/Makefile b/Documentation/Makefile index f5605b7767f..6b3f0bb6c8b 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -137,11 +137,12 @@ ASCIIDOC_HTML = xhtml11 ASCIIDOC_DOCBOOK = docbook ASCIIDOC_CONF = -f asciidoc.conf ASCIIDOC_COMMON = $(ASCIIDOC) $(ASCIIDOC_EXTRA) $(ASCIIDOC_CONF) \ - -amanversion=$(GIT_VERSION) \ -amanmanual='Git Manual' -amansource='Git' +ASCIIDOC_MANVERSION = -amanversion=$(GIT_VERSION) +ASCIIDOC_ALL = $(ASCIIDOC_COMMON) $(ASCIIDOC_MANVERSION) ASCIIDOC_DEPS = asciidoc.conf GIT-ASCIIDOCFLAGS -TXT_TO_HTML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_HTML) -TXT_TO_XML = $(ASCIIDOC_COMMON) -b $(ASCIIDOC_DOCBOOK) +TXT_TO_HTML = $(ASCIIDOC_ALL) -b $(ASCIIDOC_HTML) +TXT_TO_XML = $(ASCIIDOC_ALL) -b $(ASCIIDOC_DOCBOOK) MANPAGE_XSL = manpage-normal.xsl XMLTO = xmlto XMLTO_EXTRA = @@ -333,6 +334,16 @@ mergetools-list.made: ../git-mergetool--lib.sh $(wildcard ../mergetools/*) show_tool_names can_merge "* " || :' >mergetools-merge.txt && \ date >$@ +# We use $(ASCIIDOC_COMMON) here, and not $(ASCIIDOC_ALL). We don't +# want to include $(ASCIIDOC_MANVERSION) and have the documentation +# re-built every time HEAD changes. +# +# This is a trade-off requiring a "clean" build of the documentation +# for release purposes, in the future we might include the version if +# there's a cheaper way to re-insert the "Source" version during +# re-builds. If we detect that that's the only thing we changed we +# could insert it with a cheap search/replacement against the existing +# files. TRACK_ASCIIDOCFLAGS = $(subst ','\'',$(ASCIIDOC_COMMON):$(ASCIIDOC_HTML):$(ASCIIDOC_DOCBOOK)) GIT-ASCIIDOCFLAGS: FORCE
Since 9a71722b4df (Doc: auto-detect changed build flags, 2019-03-17) we've been eagerly re-building the documentation whenever the output of "git version" (via the GIT-VERSION file) changed. This was never the intention, and was a regression on what we intended in 7b8a74f39cb (Documentation: Replace @@GIT_VERSION@@ in documentation, 2007-03-25). So let's add an ASCIIDOC_MANVERSION variable that we exclude from ASCIIDOC_COMMON. The change in 9a71722b4df was only intending to catch cases where we e.g. switched between asciidoc and asciidoctor, not to undo the logic in 7b8a74f39cb and force a re-build every time our HEAD changed in the repository. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- As a follow-up to https://lore.kernel.org/git/874kdn1j6i.fsf@evledraar.gmail.com/ I cut "make man" out of my "rebase -x" invocations, I could swear it didn't used to take so long. Turns out it didn't, and that its eagerness is a recent-ish regression. This is what we used to do before v2.22.0, so I'm not too worried about the edge case discussed in the comment here. I think an improvement on this might be to e.g. force all the flags with a "make dist" or one of the install targets. In practice I don't think there's many/any people who build releases that matter to anyone out of the checkout they've been using for their own development. Documentation/Makefile | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)