Subject: Re: bug#74413: [PATCH] Allow to store and read repository information of VCS builds

From: Björn Bidar <[email-masked]>
Date: Mon, 18 Nov 2024 16:21:28 +0200

Eli Zaretskii <[email-masked]> writes:

>> Date: Mon, 18 Nov 2024 10:18:11 +0200
>> From:  Björn Bidar via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <[email-masked]>
>> 
>> Emacs has the feature to read the repository version and
>> branch if git is installed during the build and afterwards if also
>> the sources including the VCS repository is present.
>> 
>> For the Android builds the feature was added to store and read the
>> information mentioned above in a special version file.
>
> Po Lu, why is that needed in the Android build, and how is it used
> there?
>
>> This patch reuses that mechanism so it can be reused on other platforms
>> to use for the same reasons its available for Android and to also be
>> able to use the information on CI workers without git installed and/or a
>> full source checkout.
>
> Doesn't that go against the tendency to have _less_ detailed/private
> information in the build?  We've lately removed some relatively useful
> infos from what we report in commands that use the build information.

The information added is only the branch and the repository similarly as
used by the Android builds. There's no private information there unless
the exact change reference Emacs was built on is private.

> More generally, could you present the motivation and the rationale for
> making this information available in production builds?

The information wouldn't be only available to production builds but also
testing/developer builds that are builtin in a CI environment to
e.g. provide test builds for developers to use or to instruct user to
use to try to reproduce a bug.

Even for production builds it could be useful for convenience to track
down the exact reference/branch a build came from from, that's
side effect only thou.

>> The things I'm not sure about for this patch are:
>> - Is the generating of the version file in the right place in
>> Makefile.in
>
> It should be in the build tree, yes.

I was more talking if the section for the file should be in a separate
recipe or if etc-emacsver fits this purpose, I think the usecase is
quite close so it does sound ok to me.

>> - Is the data directory the right place to store the file
>
> Not sure, but I don't see why not.

OK I was just mostly wondering about the macOS builds who don't ship the
etc dir but since the information is be present also during dumping if
so desired it shouldn't be a big issue anyway.

>> - Should the creation of the version file be shared between the Android
>>   builds and the other platforms
>
> Let's first discuss whether this is at all needed and a good idea,
> okay?

Sure np, I was mostly speaking out load.

>
>> -	${INSTALL_PROGRAM} $(INSTALL_STRIP) src/emacs${EXEEXT} "$(DESTDIR)${bindir}/$(EMACSFULL)"
>> +	${INSTALL_PROGRAM} $(INSTALL_STRIP) src/emacs${EXEEXT} "$(DESTDIR)${bindir}/$(EMACS)"
>
> Why this change (and other similar ones)?  What does EMACSFULL have to
> do with the repository version data?
>
>> @@ -826,6 +837,7 @@ install-man:
>>  	umask 022; ${MKDIR_P} "$(DESTDIR)${man1dir}"
>>  	thisdir=`pwd -P`; \
>>  	cd ${mansrcdir}; \
>> +	cp ctags.1 gnuctags.1; \
>
> This hunk also looks unrelated.

I'm sorry these changes came in accidentally. I attached a fixed patch below:

>From d522173a61a84237d098690ee5289d2c11307306 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Bidar?= <[email-masked]>
Date: Mon, 18 Nov 2024 16:18:40 +0200
Subject: [PATCH] Allow to store and read repository information of VCS builds

Store repository information in version file if Emacs is build from
source while VCS is present. The version file can also be stored in the Emacs
sources prior built to indicate if Emacs was built from VCS sources.

An example use case could be if Emacs runs on a system where
the version control system isn't available, e.g. similarly
as it is intended for Android builds.
Another one is to be able to set the VCS information for workers
in a CI environment where sources are generated separately from VCS
but it or the VCS repository isn't present on the worker.

Reuse the same mechanism that exist for Android builds if the version
file is present.

* Makefile.in (etc-emacsver):
Generate etc/version file with revision and branch if git is installed
and Emacs sources are VCS sources.

* lisp/version.el (emacs-repository-get-branch)
(emacs-repository-get-version, emacs-repository-branch-static)
(emacs-repository-version-static):
Implement static versions that can use the information generated during
build if present.
---
 .gitignore      |  1 +
 Makefile.in     | 13 ++++++++++++-
 lisp/version.el | 44 ++++++++++++++++++++++++++++++++------------
 3 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/.gitignore b/.gitignore
index c1f31514d06..d3b737d590c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -314,6 +314,7 @@ doc/misc/modus-themes.texi
 doc/misc/org.texi
 etc/DOC
 etc/refcards/emacsver.tex
+etc/version
 gnustmp*
 /info/
 
diff --git a/Makefile.in b/Makefile.in
index 30a762ed03b..2817742af86 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -452,7 +452,18 @@ etc-emacsver:
 	sed "s/[@]majorversion@/$${majorversion}/" \
 	  ${srcdir}/etc/refcards/emacsver.tex.in > emacsver.tex.$$$$ && \
 	  ${srcdir}/build-aux/move-if-change emacsver.tex.$$$$ \
-	  ${srcdir}/etc/refcards/emacsver.tex
+	  ${srcdir}/etc/refcards/emacsver.tex; \
+	  if [ -e $(srcdir)/.git ]  && \
+	  which git > /dev/null ; then \
+	  { (cd $(srcdir) \
+	     && git rev-parse HEAD || echo "Unknown") \
+	     && (git rev-parse --abbrev-ref HEAD \
+	         || echo "Unknown") } 2> /dev/null > \
+	     ${top_builddir}/etc/version.$$$$; \
+	     ${srcdir}/build-aux/move-if-change \
+		${top_builddir}/etc/version.$$$$ \
+		${top_builddir}/etc/version; \
+          else : ;fi
 
 # The shared gamedir name as a C string literal, or a null ptr if not in use.
 PATH_GAME = $(if $(use_gamedir),"$(gamedir)",((char const *) 0))
diff --git a/lisp/version.el b/lisp/version.el
index db2afd55694..3b2c91c03dc 100644
--- a/lisp/version.el
+++ b/lisp/version.el
@@ -171,15 +171,21 @@ emacs-repository-version-git
 		  (looking-at "[[:xdigit:]]\\{40\\}"))
 	   (match-string 0)))))
 
-(defun emacs-repository-version-android ()
+(defun emacs-repository-version-static (dir)
   "Return the Emacs repository revision Emacs was built from.
 Value is nil if Emacs was not built from a repository checkout.
-Use information from the `/assets/version' special file."
+Use information from the `DIR/version' special file."
   (with-temp-buffer
-    (insert-file-contents "/assets/version")
+    (insert-file-contents (expand-file-name "version" dir))
     (let ((string (buffer-substring 1 (line-end-position))))
       (and (not (equal string "Unknown")) string))))
 
+(defun emacs-repository-version-android ()
+  "Return the Emacs repository revision Emacs was built from.
+Value is nil if Emacs was not built from a repository checkout.
+Use information from the `/assets/version' special file."
+  (emacs-repository-version-static "/assets"))
+
 (defun emacs-repository-get-version (&optional dir _external)
   "Try to return as a string the repository revision of the Emacs sources.
 The format of the returned string is dependent on the VCS in use.
@@ -194,9 +200,13 @@ emacs-repository-get-version
 
 Optional argument DIR is a directory to use instead of `source-directory'.
 Optional argument EXTERNAL is ignored."
-  (cond ((and (featurep 'android)
-              (eq system-type 'android))
-         (emacs-repository-version-android))
+  (cond ((and (or (and (featurep 'android)
+                       (eq system-type 'android)
+                       (setq dir "/assets"))
+                  (and (not dir)
+                       (file-exists-p (expand-file-name  "version" data-directory))
+                       (setq dir data-directory)))
+              (emacs-repository-version-static dir)))
         (t (emacs-repository-version-git
             (or dir source-directory)))))
 
@@ -209,8 +219,14 @@ emacs-repository-branch-android
   "Return the Emacs repository branch Emacs was built from.
 Value is nil if Emacs was not built from a repository checkout.
 Use information from the `/assets/version' special file."
+  (emacs-repository-branch-static "/assets"))
+
+(defun emacs-repository-branch-static (dir)
+  "Return the Emacs repository branch Emacs was built from.
+Value is nil if Emacs was not built from a repository checkout.
+Use information from the `DIR/version' special file."
   (with-temp-buffer
-    (insert-file-contents "/assets/version")
+    (insert-file-contents (expand-file-name "version" dir))
     (end-of-line)
     (forward-char)
     (let ((string (buffer-substring (point) (line-end-position))))
@@ -232,8 +248,8 @@ emacs-repository-get-branch
   "Try to return as a string the repository branch of the Emacs sources.
 The format of the returned string is dependent on the VCS in use.
 
-If Emacs is built for Android, use the version information
-embedded in the Emacs installation package.
+If Emacs is built for Android or contains version file,
+use the version information embedded in the Emacs installation package.
 
 Value is nil if the sources do not seem to be under version
 control, or if we could not determine the branch.  Note that
@@ -241,9 +257,13 @@ emacs-repository-get-branch
 correspond to the running Emacs.
 
 Optional argument DIR is a directory to use instead of `source-directory'."
-  (cond ((and (featurep 'android)
-              (eq system-type 'android))
-         (emacs-repository-branch-android))
+  (cond ((and (or (and (featurep 'android)
+                       (eq system-type 'android)
+                       (setq dir "/assets"))
+                  (and (not dir)
+                       (file-exists-p (expand-file-name  "version" data-directory))
+                       (setq dir data-directory)))
+              (emacs-repository-branch-static dir)))
         (t (emacs-repository-branch-git
             (or dir source-directory)))))
 
-- 
2.45.2