Subject: bug#30147: closed (Re: [bug#30147] Simplify “scripts/guix”)

From: [email-masked] (GNU bug Tracking System)
Date: Tue, 23 Jan 2018 09:15:02 +0000

Your bug report

#30147: Simplify “scripts/guix”

which was filed against the guix-patches package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
30147: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=30147
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: [bug#30147] Simplify “scripts/guix”
From: [email-masked] (Ludovic Courtès)
To: Mathieu Lirzin <[email-masked]>
Cc: [email-masked]
Date: Tue, 23 Jan 2018 10:13:57 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

Hello,

Mathieu Lirzin <[email-masked]> skribis:

> From 342444897673d5f9d9a475986e76ca2e912f6674 Mon Sep 17 00:00:00 2001
> From: Mathieu Lirzin <[email-masked]>
> Date: Wed, 17 Jan 2018 17:14:24 +0100
> Subject: [PATCH 1/2] =?UTF-8?q?build:=20Expand=20=E2=80=98scripts/guix?=
>  =?UTF-8?q?=E2=80=99=20at=20Make=20time.?=
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> This moves the complexity of Autotools variable expansion outside of the
> application code.
>
> * scripts/guix.in (config-lookup): Delete.
> (maybe-augment-load-paths!, run-guix-main): Use fully expanded variables
> instead of calling ‘config-lookup’.
> * configure.ac: Don't use AC_CONFIG_FILES for ‘scripts/guix’.
> * Makefile.am (scripts/guix): New rule.
> (do_subst): New variable.
> (CLEANFILES, EXTRA_DIST): Adapt.

That’s a good idea.  I applied it with the changes below, mostly to
account for Eric’s suggestions.

diff --git a/Makefile.am b/Makefile.am
index 5e36dbf44..9bafdab49 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -29,17 +29,17 @@
 bin_SCRIPTS = scripts/guix
 
 # Handle substitution of fully-expanded Autoconf variables.
-do_subst = sed \
-  -e 's,[@]GUILE[@],$(GUILE),g' \
-  -e 's,[@]guilemoduledir[@],$(guilemoduledir),g' \
-  -e 's,[@]guileobjectdir[@],$(guileobjectdir),g' \
+do_subst = $(SED)					\
+  -e 's,[@]GUILE[@],$(GUILE),g'				\
+  -e 's,[@]guilemoduledir[@],$(guilemoduledir),g'	\
+  -e 's,[@]guileobjectdir[@],$(guileobjectdir),g'	\
   -e 's,[@]localedir[@],$(localedir),g'
 
 scripts/guix: scripts/guix.in Makefile
-	$(AM_V_GEN)rm -f $@ $@-t \
-	  && $(MKDIR_P) $(@D) \
-	  && $(do_subst) <$(srcdir)/$@.in >$@-t \
-	  && chmod a+x,a-w $@-t && mv -f $@-t $@
+	$(AM_V_at)rm -f $@ $@-t
+	$(AM_V_at)$(MKDIR_P) "$(@D)"
+	$(AM_V_GEN)$(do_subst) < "$(srcdir)/$@.in" > "$@-t"
+	$(AM_V_at)chmod a+x,a-w "$@-t" && mv -f "$@-t" "$@"
 
 nodist_noinst_SCRIPTS =				\
   pre-inst-env					\
diff --git a/configure.ac b/configure.ac
index c50dcaf2e..f69f79648 100644
--- a/configure.ac
+++ b/configure.ac
@@ -124,6 +124,8 @@ dnl Make sure we don't suffer from the bug in 'equal?' wrt. syntax objects
 dnl found in 2.2.1.  See <https://bugs.gnu.org/29903>.
 GUIX_ASSERT_SYNTAX_OBJECT_EQUAL
 
+AC_PROG_SED
+
 dnl Decompressors, for use by the substituter and other modules.
 AC_PATH_PROG([GZIP], [gzip])
 AC_PATH_PROG([BZIP2], [bzip2])

> From b6f8331455da1ffc4896b06cd2ee98e09b05be43 Mon Sep 17 00:00:00 2001
> From: Mathieu Lirzin <[email-masked]>
> Date: Wed, 17 Jan 2018 19:55:49 +0100
> Subject: [PATCH 2/2] guix: Refactor script.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> * scripts/guix.in: Use ‘and-let*’ and remove empty surrounding ‘let’.
> (run-guix-main, maybe-augment-load-paths!): Inline them.

This is entirely subjective but I prefer the current style (in fact I
never use SRFI-2), so I’d rather skip this patch.  WDYT?  :-)

Thank you!

Ludo’.

Subject: Simplify “scripts/guix”
From: Mathieu Lirzin <[email-masked]>
To: [email-masked]
Date: Wed, 17 Jan 2018 20:25:14 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

Hi,

Here are some improvements for “scripts/guix”.

>From 342444897673d5f9d9a475986e76ca2e912f6674 Mon Sep 17 00:00:00 2001
From: Mathieu Lirzin <[email-masked]>
Date: Wed, 17 Jan 2018 17:14:24 +0100
Subject: [PATCH 1/2] =?UTF-8?q?build:=20Expand=20=E2=80=98scripts/guix?=
 =?UTF-8?q?=E2=80=99=20at=20Make=20time.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This moves the complexity of Autotools variable expansion outside of the
application code.

* scripts/guix.in (config-lookup): Delete.
(maybe-augment-load-paths!, run-guix-main): Use fully expanded variables
instead of calling ‘config-lookup’.
* configure.ac: Don't use AC_CONFIG_FILES for ‘scripts/guix’.
* Makefile.am (scripts/guix): New rule.
(do_subst): New variable.
(CLEANFILES, EXTRA_DIST): Adapt.
---
 Makefile.am     | 20 +++++++++++++++++---
 configure.ac    |  1 -
 scripts/guix.in | 31 ++++++-------------------------
 3 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index aebd3b1eb..5e36dbf44 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2,7 +2,7 @@
 # Copyright © 2012, 2013, 2014, 2015, 2016, 2017 Ludovic Courtès <[email-masked]>
 # Copyright © 2013 Andreas Enge <[email-masked]>
 # Copyright © 2015, 2017 Alex Kost <[email-masked]>
-# Copyright © 2016 Mathieu Lirzin <[email-masked]>
+# Copyright © 2016, 2018 Mathieu Lirzin <[email-masked]>
 # Copyright © 2016, 2017 Mark H Weaver <[email-masked]>
 # Copyright © 2017 Mathieu Othacehe <[email-masked]>
 # Copyright © 2017 Leo Famulari <[email-masked]>
@@ -26,8 +26,20 @@
 # You should have received a copy of the GNU General Public License
 # along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
-bin_SCRIPTS =					\
-  scripts/guix
+bin_SCRIPTS = scripts/guix
+
+# Handle substitution of fully-expanded Autoconf variables.
+do_subst = sed \
+  -e 's,[@]GUILE[@],$(GUILE),g' \
+  -e 's,[@]guilemoduledir[@],$(guilemoduledir),g' \
+  -e 's,[@]guileobjectdir[@],$(guileobjectdir),g' \
+  -e 's,[@]localedir[@],$(localedir),g'
+
+scripts/guix: scripts/guix.in Makefile
+	$(AM_V_GEN)rm -f $@ $@-t \
+	  && $(MKDIR_P) $(@D) \
+	  && $(do_subst) <$(srcdir)/$@.in >$@-t \
+	  && chmod a+x,a-w $@-t && mv -f $@-t $@
 
 nodist_noinst_SCRIPTS =				\
   pre-inst-env					\
@@ -437,6 +449,7 @@ EXTRA_DIST =						\
   TODO							\
   CODE-OF-CONDUCT					\
   .dir-locals.el					\
+  bin/guix.in						\
   build-aux/build-self.scm				\
   build-aux/compile-all.scm				\
   build-aux/hydra/evaluate.scm				\
@@ -473,6 +486,7 @@ endif !BUILD_DAEMON_OFFLOAD

 CLEANFILES =					\
+  $(bin_SCRIPTS)				\
   $(GOBJECTS)					\
   $(SCM_TESTS:tests/%.scm=%.log)
 
diff --git a/configure.ac b/configure.ac
index 1e3912248..c50dcaf2e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -272,7 +272,6 @@ AC_CONFIG_FILES([Makefile
                  po/packages/Makefile.in
 		 guix/config.scm])
 
-AC_CONFIG_FILES([scripts/guix], [chmod +x scripts/guix])
 AC_CONFIG_FILES([test-env:build-aux/test-env.in], [chmod +x test-env])
 AC_CONFIG_FILES([pre-inst-env:build-aux/pre-inst-env.in],
   [chmod +x pre-inst-env])
diff --git a/scripts/guix.in b/scripts/guix.in
index e20c27424..af50a782b 100644
--- a/scripts/guix.in
+++ b/scripts/guix.in
@@ -3,6 +3,7 @@
 !#
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2013 Mark H Weaver <[email-masked]>
+;;; Copyright © 2018 Mathieu Lirzin <[email-masked]>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -22,35 +23,15 @@
 ;; IMPORTANT: We must avoid loading any modules from Guix here,
 ;; because we need to adjust the guile load paths first.
 ;; It's okay to import modules from core Guile though.
-(use-modules (ice-9 regex)
-             (srfi srfi-26))
+(use-modules (srfi srfi-26))
 
 (let ()
   (define-syntax-rule (push! elt v) (set! v (cons elt v)))
 
-  (define config-lookup
-    (let ((config '(("prefix"         . "@prefix@")
-                    ("exec_prefix"    . "@exec_prefix@")
-                    ("datarootdir"    . "@datarootdir@")
-                    ("guilemoduledir" . "@guilemoduledir@")
-                    ("guileobjectdir" . "@guileobjectdir@")
-                    ("localedir"      . "@localedir@")))
-          (var-ref-regexp (make-regexp "\\$\\{([a-z_]+)\\}")))
-      (define (expand-var-ref match)
-        (lookup (match:substring match 1)))
-      (define (expand str)
-        (regexp-substitute/global #f var-ref-regexp str
-                                  'pre expand-var-ref 'post))
-      (define (lookup name)
-        (expand (assoc-ref config name)))
-      lookup))
-
   (define (maybe-augment-load-paths!)
     (unless (getenv "GUIX_UNINSTALLED")
-      (let ((module-dir (config-lookup "guilemoduledir"))
-            (object-dir (config-lookup "guileobjectdir")))
-        (push! module-dir %load-path)
-        (push! object-dir %load-compiled-path))
+      (push! "@guilemoduledir@" %load-path)
+      (push! "@guileobjectdir@" %load-compiled-path)
       (let ((updates-dir (and=> (or (getenv "XDG_CONFIG_HOME")
                                     (and=> (getenv "HOME")
                                            (cut string-append <> "/.config")))
@@ -64,8 +45,8 @@
   (define (run-guix-main)
     (let ((guix-main (module-ref (resolve-interface '(guix ui))
                                  'guix-main)))
-      (bindtextdomain "guix" (config-lookup "localedir"))
-      (bindtextdomain "guix-packages" (config-lookup "localedir"))
+      (bindtextdomain "guix" "@localedir@")
+      (bindtextdomain "guix-packages" "@localedir@")
       (apply guix-main (command-line))))
 
   (maybe-augment-load-paths!)
-- 
2.15.1

>From b6f8331455da1ffc4896b06cd2ee98e09b05be43 Mon Sep 17 00:00:00 2001
From: Mathieu Lirzin <[email-masked]>
Date: Wed, 17 Jan 2018 19:55:49 +0100
Subject: [PATCH 2/2] guix: Refactor script.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* scripts/guix.in: Use ‘and-let*’ and remove empty surrounding ‘let’.
(run-guix-main, maybe-augment-load-paths!): Inline them.
---
 scripts/guix.in | 45 +++++++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/scripts/guix.in b/scripts/guix.in
index af50a782b..f971398e8 100644
--- a/scripts/guix.in
+++ b/scripts/guix.in
@@ -23,36 +23,33 @@
 ;; IMPORTANT: We must avoid loading any modules from Guix here,
 ;; because we need to adjust the guile load paths first.
 ;; It's okay to import modules from core Guile though.
-(use-modules (srfi srfi-26))
 
-(let ()
-  (define-syntax-rule (push! elt v) (set! v (cons elt v)))
+(use-modules (srfi srfi-2)
+             (srfi srfi-26))
 
-  (define (maybe-augment-load-paths!)
-    (unless (getenv "GUIX_UNINSTALLED")
-      (push! "@guilemoduledir@" %load-path)
-      (push! "@guileobjectdir@" %load-compiled-path)
-      (let ((updates-dir (and=> (or (getenv "XDG_CONFIG_HOME")
-                                    (and=> (getenv "HOME")
-                                           (cut string-append <> "/.config")))
-                                (cut string-append <> "/guix/latest"))))
-        (when (and updates-dir (file-exists? updates-dir))
-          ;; XXX: Currently 'guix pull' puts both .scm and .go files in
-          ;; UPDATES-DIR.
-          (push! updates-dir %load-path)
-          (push! updates-dir %load-compiled-path)))))
+(define-syntax-rule (push! elt v) (set! v (cons elt v)))
 
-  (define (run-guix-main)
-    (let ((guix-main (module-ref (resolve-interface '(guix ui))
-                                 'guix-main)))
-      (bindtextdomain "guix" "@localedir@")
-      (bindtextdomain "guix-packages" "@localedir@")
-      (apply guix-main (command-line))))
+(unless (getenv "GUIX_UNINSTALLED")
+  ;; Add installed modules to load-path.
+  (push! "@guilemoduledir@" %load-path)
+  (push! "@guileobjectdir@" %load-compiled-path)
 
-  (maybe-augment-load-paths!)
+  ;; Add modules fetched by 'guix pull' to load-path.
+  (and-let* ((conf-dir (or (getenv "XDG_CONFIG_HOME")
+                           (and=> (getenv "HOME")
+                                  (cut string-append <> "/.config"))))
+             (updates-dir (string-append conf-dir "/guix/latest"))
+             ((file-exists? updates-dir)))
+    ;; XXX: Currently 'guix pull' puts both .scm and .go files in UPDATES-DIR.
+    (push! updates-dir %load-path)
+    (push! updates-dir %load-compiled-path)))
 
+;; Run Guix.
+(let ((guix-main (module-ref (resolve-interface '(guix ui)) 'guix-main)))
+  (bindtextdomain "guix" "@localedir@")
+  (bindtextdomain "guix-packages" "@localedir@")
   ;; XXX: It would be more convenient to change it to:
   ;;   (exit (run-guix-main))
   ;; but since the 'guix' command is not updated by 'guix pull', we cannot
   ;; really do it now.
-  (run-guix-main))
+  (apply guix-main (command-line)))
-- 
2.15.1

Thanks.

-- 
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37