diff mbox series

[bug#68935,1/3] svn-fetch: Require svn-command argument.

Message ID 3a3748ee3fd345ff26bd63979bf8cee2c32d87ab.1707144191.git.herman@rimm.ee
State New
Headers show
Series Add 'put' option to guix import. | expand

Commit Message

Herman Rimm Feb. 5, 2024, 3:07 p.m. UTC 
* guix/build/svn.scm (svn-fetch): Require svn-command argument.
* guix/svn-download.scm: Pass svn-command to each (svn-fetch).

Change-Id: Ibc7d3a79e45374588f71ca4d4ac2685c60ff3c00
---
 guix/build/svn.scm    |  5 ++---
 guix/svn-download.scm | 35 ++++++++++++++++++++++++++---------
 2 files changed, 28 insertions(+), 12 deletions(-)

Comments

Ludovic Courtès Feb. 7, 2024, 9:34 p.m. UTC | #1 
Hi Herman,

Herman Rimm <herman@rimm.ee> skribis:

> * guix/build/svn.scm (svn-fetch): Require svn-command argument.
> * guix/svn-download.scm: Pass svn-command to each (svn-fetch).
>
> Change-Id: Ibc7d3a79e45374588f71ca4d4ac2685c60ff3c00

Usually we write all the entities (procedures, variables, etc.)
modified in the commit log to help review.

> -(define* (svn-fetch url revision directory
> -                    #:key (svn-command "svn")
> -                    (recursive? #t)
> +(define* (svn-fetch url revision directory svn-command
> +                    #:key (recursive? #t)
>                      (user-name #f)
>                      (password #f))

What’s the rationale for this change?

It’s a relatively common pattern in Guix build-side code to assume that
commands can be found in $PATH while still providing a way to specify
the command’s absolute file name.

>  (define* (download-svn-to-store store ref
>                                  #:optional (name (basename (svn-reference-url ref)))
> -                                #:key (log (current-error-port)))
> +                                #:key (log (current-error-port))
> +                                (svn (subversion-package)))
>    "Download from REF, a <svn-reference> object to STORE.  Write progress
>  reports to LOG."
>    (call-with-temporary-directory
>     (lambda (temp)
>       (let ((result
>              (parameterize ((current-output-port log))
> -              (build:svn-fetch (svn-reference-url ref)
> +              (with-imported-modules
> +                  (source-module-closure '((guix build svn)
> +                                           (guix build utils)))
> +                #~(begin
> +                    (use-modules (guix build svn)
> +                                 (guix build utils))
> +                    (svn-fetch (svn-reference-url ref)
>                                 (svn-reference-revision ref)
>                                 (string-append temp "/svn")
> +                               #+(file-append svn "/bin/svn")
>                                 #:user-name (svn-reference-user-name ref)
> -                               #:password (svn-reference-password ref)))))
> +                               #:password (svn-reference-password ref)))))))
>         (and result
>              (add-to-store store name #t "sha256"
>                            (string-append temp "/svn")))))))

This doesn’t have the desired effect: ‘download-svn-to-store’ will now
always return a gexp instead of returning either #f or a store file
name.

This procedure and ‘download-multi-svn-to-store’ (used by the texlive
importer) really expect “svn” to be in $PATH.  It cannot really be
avoided AFAICS.

Thanks,
Ludo’.
diff mbox series

Patch

diff --git a/guix/build/svn.scm b/guix/build/svn.scm
index 875d3c50ca..e8f168abc6 100644
--- a/guix/build/svn.scm
+++ b/guix/build/svn.scm
@@ -31,9 +31,8 @@  (define-module (guix build svn)
 ;;;
 ;;; Code:
 
-(define* (svn-fetch url revision directory
-                    #:key (svn-command "svn")
-                    (recursive? #t)
+(define* (svn-fetch url revision directory svn-command
+                    #:key (recursive? #t)
                     (user-name #f)
                     (password #f))
   "Fetch REVISION from URL into DIRECTORY.  REVISION must be an integer, and a
diff --git a/guix/svn-download.scm b/guix/svn-download.scm
index c6688908de..c4eeed3563 100644
--- a/guix/svn-download.scm
+++ b/guix/svn-download.scm
@@ -2,6 +2,7 @@ 
 ;;; Copyright © 2014-2016, 2019, 2021-2023 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2014 Sree Harsha Totakura <sreeharsha@totakura.in>
 ;;; Copyright © 2017, 2019, 2021 Ricardo Wurmus <rekado@elephly.net>
+;;; Copyright © 2024 Herman Rimm <herman@rimm.ee>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -105,7 +106,7 @@  (define* (svn-fetch ref hash-algo hash
             (or (svn-fetch (getenv "svn url")
                            (string->number (getenv "svn revision"))
                            #$output
-                           #:svn-command #+(file-append svn "/bin/svn")
+                           #+(file-append svn "/bin/svn")
                            #:recursive? (match (getenv "svn recursive?")
                                           ("yes" #t)
                                           (_ #f))
@@ -195,7 +196,7 @@  (define* (svn-multi-fetch ref hash-algo hash
                               (if (string-suffix? "/" location)
                                   (string-append #$output "/" location)
                                   (string-append #$output "/" (dirname location)))
-                              #:svn-command #+(file-append svn "/bin/svn")
+                              #+(file-append svn "/bin/svn")
                               #:recursive? (match (getenv "svn recursive?")
                                              ("yes" #t)
                                              (_ #f))
@@ -245,25 +246,34 @@  (define* (svn-multi-fetch ref hash-algo hash
 
 (define* (download-svn-to-store store ref
                                 #:optional (name (basename (svn-reference-url ref)))
-                                #:key (log (current-error-port)))
+                                #:key (log (current-error-port))
+                                (svn (subversion-package)))
   "Download from REF, a <svn-reference> object to STORE.  Write progress
 reports to LOG."
   (call-with-temporary-directory
    (lambda (temp)
      (let ((result
             (parameterize ((current-output-port log))
-              (build:svn-fetch (svn-reference-url ref)
+              (with-imported-modules
+                  (source-module-closure '((guix build svn)
+                                           (guix build utils)))
+                #~(begin
+                    (use-modules (guix build svn)
+                                 (guix build utils))
+                    (svn-fetch (svn-reference-url ref)
                                (svn-reference-revision ref)
                                (string-append temp "/svn")
+                               #+(file-append svn "/bin/svn")
                                #:user-name (svn-reference-user-name ref)
-                               #:password (svn-reference-password ref)))))
+                               #:password (svn-reference-password ref)))))))
        (and result
             (add-to-store store name #t "sha256"
                           (string-append temp "/svn")))))))
 
 (define* (download-multi-svn-to-store store ref
                                       #:optional (name (basename (svn-multi-reference-url ref)))
-                                      #:key (log (current-error-port)))
+                                      #:key (log (current-error-port))
+                                      (svn (subversion-package)))
   "Download from REF, a <svn-multi-reference> object to STORE.  Write progress
 reports to LOG."
   (call-with-temporary-directory
@@ -272,16 +282,23 @@  (define* (download-multi-svn-to-store store ref
                    (let ((dir (string-append temp "/" (dirname location))))
                      (mkdir-p dir))
                    (parameterize ((current-output-port log))
-                     (build:svn-fetch (string-append (svn-multi-reference-url ref)
-                                                     "/" location)
+                     (with-imported-modules
+                         (source-module-closure '((guix build svn)
+                                                  (guix build utils)))
+                       #~(begin
+                           (use-modules (guix build svn)
+                                        (guix build utils))
+                           (svn-fetch (string-append (svn-multi-reference-url ref)
+                                                           "/" location)
                                       (svn-multi-reference-revision ref)
                                       (if (string-suffix? "/" location)
                                           (string-append temp "/" location)
                                           (string-append temp "/" (dirname location)))
+                                      #+(file-append svn "/bin/svn")
                                       #:recursive?
                                       (svn-multi-reference-recursive? ref)
                                       #:user-name (svn-multi-reference-user-name ref)
-                                      #:password (svn-multi-reference-password ref))))
+                                      #:password (svn-multi-reference-password ref))))))
                  (svn-multi-reference-locations ref))
           (add-to-store store name #t "sha256" temp)))))