[cmake] Untangle the CMake versioning

This untangles the CMake versioning issues (I hope) as discussed in #4626.
Note most of the advice found on the Internet about how to inject git
versions into CMake is just wrong.

The behavior we want is to unconditionally run the script
build_tools/git_version_gen.sh at build time (i.e. when you invoke ninja or
make, and not when you invoke cmake, which is build system generation time).
This script is careful to only update the FISH-BUILD-VERSION-FILE if the
contents have changed, to avoid spurious rebuilding dependencies of
FISH-BUILD-VERSION-FILE. Assuming the git version hasn't changed, the script
will run, but not update FISH-BUILD-VERSION-FILE, and therefore
fish_version.o will not have to be rebuilt.

This might normally rebuild more than is necessary even if the timestamp is
not updated, because ninja computes the dependency chain ahead of time. But
Ninja also supports the 'restat' option for just this case, and CMake is rad
and exposes this via BYPRODUCTS. So mark FISH-BUILD-VERSION-FILE as a
byproduct and make the script always update a dummy file
(fish-build-version-witness.txt). Note this is the use case for which
BYPRODUCTS is designed.

We also have fish_version.cpp #include "FISH-BUILD-VERSION-FILE", and do a
semi-silly thing and make FISH-BUILD-VERSION-FILE valid C++ (so there's just
one version file). This means we have to filter out the quotes in other
cases..

.gitignore

@@ -78,6 +78,7 @@ messages.pot
 /lexicon_filter
 /toc.txt
 /version
+fish-build-version-witness.txt
 
 # File names that can appear below the project root that represent artifacts
 # from building and testing.
@@ -101,4 +102,5 @@ xcuserdata/
 *.moved-aside
 *.xccheckout
 *.xcscmblueprin
+.vscode
 

CMakeLists.txt

@@ -55,17 +55,6 @@ SET(FISH_SRCS
 # Header files are just globbed.
 FILE(GLOB FISH_HEADERS src/*.h)
 
-# Set up the version target.
-# This creates the file FISH-BUILD-VERSION-FILE which is only modified if necessary.
-ADD_CUSTOM_COMMAND(OUTPUT "FISH-BUILD-VERSION-FILE"
-                   DEPENDS CHECK-FISH-BUILD-VERSION-FILE)
-
-ADD_CUSTOM_TARGET("CHECK-FISH-BUILD-VERSION-FILE"
-                  COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/build_tools/git_version_gen.sh)
-
-SET(FBVF "FISH-BUILD-VERSION-FILE")
-
-
 # Set up config.h
 INCLUDE(cmake/ConfigureChecks.cmake)
 INCLUDE(cmake/gettext.cmake)
@@ -76,7 +65,6 @@ INCLUDE_DIRECTORIES(${CMAKE_CURRENT_BINARY_DIR})
 # Set up standard directories.
 INCLUDE(GNUInstallDirs)
 ADD_DEFINITIONS(-D_UNICODE=1
-                -DFISH_BUILD_VERSION="${FISH_BUILD_VERSION}"
                 -DLOCALEDIR="${CMAKE_INSTALL_FULL_LOCALEDIR}"
                 -DPREFIX=L"${CMAKE_INSTALL_PREFIX}"
                 -DDATADIR=L"${CMAKE_INSTALL_FULL_DATADIR}"
@@ -84,6 +72,16 @@ ADD_DEFINITIONS(-D_UNICODE=1
                 -DBINDIR=L"${CMAKE_INSTALL_FULL_BINDIR}"
                 -DDOCDIR=L"${CMAKE_INSTALL_FULL_DOCDIR}")
 
+# Set up the machinery around FISH-BUILD-VERSION-FILE
+# This defines the FBVF variable.
+INCLUDE(Version)
+
+# Teach fish_version.o to rebuild when FBVF changes.
+# The standard C++ include detection machinery misses this.
+SET_SOURCE_FILES_PROPERTIES(src/fish_version.cpp
+                            PROPERTIES OBJECT_DEPENDS
+                            ${CMAKE_CURRENT_BINARY_DIR}/${FBVF})
+
 # Set up PCRE2
 INCLUDE(cmake/PCRE2.cmake)
 

build_tools/git_version_gen.sh

@@ -1,14 +1,11 @@
-#!/bin/sh
+#!/bin/bash
 # Originally from the git sources (GIT-VERSION-GEN)
 # Presumably (C) Junio C Hamano <junkio@cox.net>
 # Reused under GPL v2.0
 # Modified for fish by David Adam <zanchey@ucc.gu.uwa.edu.au>
 
-# Obtain directory containing this script in POSIX-compatible manner
-# See https://stackoverflow.com/a/43919044/17027 (public domain)
-a="/$0"; a="${a%/*}"; a="${a:-.}"; a="${a#/}/"; BASEDIR=$(cd "$a"; pwd)
-# Find the fish git directory as two levels up from this directory.
-GIT_DIR=$(dirname "$a")
+# Find the fish git directory as two levels up from script directory.
+GIT_DIR="$( cd "$( dirname $( dirname "${BASH_SOURCE[0]}" ) )" && pwd )"
 
 FBVF=FISH-BUILD-VERSION-FILE
 DEF_VER=unknown
@@ -24,11 +21,21 @@ fi
 
 if test -r $FBVF
 then
-	VC=$(sed -e 's/^FISH_BUILD_VERSION=//' <$FBVF)
+	VC=$(grep -v '^#' $FBVF | tr -d '"' | sed -e 's/^FISH_BUILD_VERSION=//')
 else
 	VC=unset
 fi
+
+# Output the FBVF
+# Note that we are super-double sneaky: we produce a file that is valid bash
+# and valid C++, so it may be included from either. Also it may have leading
+# hashes filtered out with grep to get a version of the form (for example):
+#   FISH_BUILD_VERSION=2.7.1-620-g94c9f5c2
 test "$VN" = "$VC" || {
 	echo >&2 "FISH_BUILD_VERSION=$VN"
-	echo "FISH_BUILD_VERSION=$VN" >$FBVF
+	echo "FISH_BUILD_VERSION=\"$VN\"" >$FBVF
 }
+
+# Output the fish-build-version-witness.txt
+# See https://cmake.org/cmake/help/v3.4/policy/CMP0058.html
+date +%s > fish-build-version-witness.txt

cmake/Docs.cmake

@@ -70,7 +70,7 @@ ADD_CUSTOM_COMMAND(OUTPUT doc.h
 # Note we would like to add doc_src/index.hdr.in as a dependency but CMake replaces this with
 # doc_src/index.hdr; CMake bug?
 ADD_CUSTOM_COMMAND(OUTPUT toc.txt
-                   COMMAND env `cat ${FBVF}` ${CMAKE_CURRENT_SOURCE_DIR}/build_tools/build_toc_txt.sh
+                   COMMAND env `cat ${FBVF} | tr -d '\"'` ${CMAKE_CURRENT_SOURCE_DIR}/build_tools/build_toc_txt.sh
                            doc_src/index.hdr.in ${HDR_FILES_NO_INDEX}
                            > ${CMAKE_CURRENT_BINARY_DIR}/toc.txt
                    DEPENDS ${FBVF} ${HDR_FILES_NO_INDEX})
@@ -95,7 +95,7 @@ ADD_CUSTOM_TARGET(doc
                   DEPENDS ${FBVF} Doxyfile.user ${DOC_SRC_FILES} doc.h ${HDR_FILES} lexicon_filter)
 
 ADD_CUSTOM_COMMAND(OUTPUT share/man/
-                   COMMAND env `cat ${FBVF}`
+                   COMMAND env `cat ${FBVF} | tr -d '\"' `
                            INPUT_FILTER=lexicon_filter ${CMAKE_CURRENT_SOURCE_DIR}/build_tools/build_documentation.sh ${CMAKE_CURRENT_SOURCE_DIR}/Doxyfile.help doc_src ./share
                    DEPENDS ${FBVF} ${HELP_SRC} ${CMAKE_CURRENT_BINARY_DIR}/lexicon_filter)
 

cmake/Install.cmake

@@ -111,7 +111,7 @@ FISH_CREATE_DIRS(${rel_datadir}/pkgconfig ${extra_completionsdir}
 # $v $(INSTALL) -m 644 fish.pc $(DESTDIR)$(datadir)/pkgconfig
 CONFIGURE_FILE(fish.pc.in fish.pc.noversion)
 ADD_CUSTOM_COMMAND(OUTPUT fish.pc
-  COMMAND awk -v `cat ${FBVF}` '/^Version:/ {$$0=$$0 FISH_BUILD_VERSION} 1' fish.pc.noversion  > fish.pc
+  COMMAND awk -v `cat ${FBVF} | tr -d '\"'` '/^Version:/ {$$0=$$0 FISH_BUILD_VERSION} 1' fish.pc.noversion  > fish.pc
   WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
   DEPENDS ${FBVF} ${CMAKE_CURRENT_BINARY_DIR}/fish.pc.noversion)
 

cmake/Version.cmake

@@ -0,0 +1,51 @@
+# This file adds commands to manage the FISH-BUILD-VERSION-FILE (hereafter
+# FBVF). This file exists in the build directory and is used to populate the
+# documentation and also the version string in fish_version.o (printed with
+# `echo $version` and also fish --version). The essential idea is that we are
+# going to invoke git_version_gen.sh, which will update the
+# FISH-BUILD-VERSION-FILE only if it needs to change; this is what makes
+# incremental rebuilds fast.
+#
+# This code is delicate, with the chief subtlety revolving around Ninja. A
+# natural and naive approach would tell the generated build system that FBVF is
+# a dependency of fish_version.o, and that git_version_gen.sh updates it. Make
+# will then invoke the script, check the timestamp on fish_version.o and FBVF,
+# see that FBVF is earlier, and then not rebuild fish_version.o. Ninja,
+# however, decides what to build up-front and will unconditionally rebuild
+# fish_version.o.
+#
+# To avoid this with Ninja, we want to hook into its 'restat' option which we
+# can do through the BYPRODUCTS feature of CMake. See
+# https://cmake.org/cmake/help/latest/policy/CMP0058.html
+#
+# Unfortunately BYPRODUCTS behaves strangely with the Makefile generator: it
+# marks FBVF as generated and then CMake itself will `touch` it on every build,
+# meaning that using BYPRODUCTS will cause fish_version.o to be rebuilt
+# unconditionally with the Makefile generator. Thus we want to use the
+# natural-and-naive approach for Makefiles.
+
+# **IMPORTANT** If you touch these build rules, please test both Ninja and
+# Makefile generators with both a clean and dirty git tree. Verify that both
+# generated build systems rebuild fish when the git tree goes from dirty to
+# clean (and vice versa), and verify they do NOT rebuild it when the git tree
+# stays the same (incremental builds must be fast).
+
+# Just a handy abbreviation.
+SET(FBVF FISH-BUILD-VERSION-FILE)
+
+# TODO: find a cleaner way to do this.
+IF (${CMAKE_GENERATOR} STREQUAL Ninja)
+  SET(FBVF-OUTPUT fish-build-version-witness.txt)
+  SET(CFBVF-BYPRODUCTS ${FBVF})
+ELSE(${CMAKE_GENERATOR} STREQUAL Ninja)
+  SET(FBVF-OUTPUT ${FBVF})
+  SET(CFBVF-BYPRODUCTS)
+ENDIF(${CMAKE_GENERATOR} STREQUAL Ninja)
+
+# Set up the version targets
+ADD_CUSTOM_TARGET(CHECK-FISH-BUILD-VERSION-FILE
+                  COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/build_tools/git_version_gen.sh
+                  BYPRODUCTS ${CFBVF-BYPRODUCTS})
+
+ADD_CUSTOM_COMMAND(OUTPUT ${FBVF-OUTPUT}
+                    DEPENDS CHECK-FISH-BUILD-VERSION-FILE)

configure.ac

@@ -12,7 +12,7 @@ m4_syscmd([build_tools/git_version_gen.sh 2>/dev/null])
 
 AC_PREREQ([2.60])
 AC_INIT(fish,
-        m4_esyscmd([cut -f 3 -d ' ' FISH-BUILD-VERSION-FILE | tr -d '\n']),
+        m4_esyscmd([cut -f 2 -d '=' FISH-BUILD-VERSION-FILE | tr -d '"\n']),
         fish-users@lists.sourceforge.net)
 ac_clean_files=a.out.dSYM
 

src/fish_version.cpp

@@ -5,7 +5,12 @@
 #include "fish_version.h"
 
 #ifndef FISH_BUILD_VERSION
-#include "fish-build-version.h"
+// The contents of FISH-BUILD-VERSION-FILE looks like:
+// FISH_BUILD_VERSION="2.7.1-62-gc0480092-dirty"
+// Arrange for it to become a variable.
+static const char *
+#include "FISH-BUILD-VERSION-FILE"
+    ;
 #endif
 
 /// Return fish shell version.