[bug#58623,0/3] import/cran: Parameterize for guix-cran.
Commit Message
Hi,
the attached patches are required for guix-cran
(https://github.com/guix-science/guix-cran). import/cran already has the
ability to add a prefix to licenses, but it was not exposed. Additionally
I need to parameterize fetch/download functions, so I can cache the
tarballs/DESCRIPTION files.
Cheers,
Lars
Lars-Dominik Braun (3):
import/cran: Allow custom license prefix.
import/cran: Allow overriding description fetch function.
import/cran: Allow overriding tarball download.
guix/import/cran.scm | 30 +++++++++++++++++++++---------
guix/scripts/import/cran.scm | 18 ++++++++++++++++--
2 files changed, 37 insertions(+), 11 deletions(-)
Comments
Hi Lars,
On mer., 19 oct. 2022 at 11:28, Lars-Dominik Braun <lars@6xq.net> wrote:
> the attached patches are required for guix-cran
> (https://github.com/guix-science/guix-cran). import/cran already has the
> ability to add a prefix to licenses, but it was not exposed. Additionally
> I need to parameterize fetch/download functions, so I can cache the
> tarballs/DESCRIPTION files.
This is really cool! Thanks for working on that.
> Subject: [PATCH 1/3] import/cran: Allow custom license prefix.
> X-Debbugs-Cc: zimon.toutoune@gmail.com
> X-Debbugs-Cc: dev@jpoiret.xyz
> X-Debbugs-Cc: mail@cbaines.net
> X-Debbugs-Cc: rekado@elephly.net
> X-Debbugs-Cc: othacehe@gnu.org
> X-Debbugs-Cc: ludo@gnu.org
Because the patch had been sent as attachment, I have not received this
X-Debbugs-CC, IIRC. Well, I have added these CC.
> * guix/import/cran.scm (%license-prefix): New parameter.
> (string->license): Use it.
> * guix/scripts/import/cran.scm (%options): Add new parameter -p/--license-prefix.
> (show-help): Document it.
> (parse-options): Pass it as a parameter to importer.
LGTM. Maybe a line in the manual could help.
> Subject: [PATCH 2/3] import/cran: Allow overriding description fetch function.
>
> * guix/import/cran.scm (%fetch-description): New parameter.
> (cran->guix-package): Use it.
> (upstream-name): Use it.
[...]
> Subject: [PATCH 3/3] import/cran: Allow overriding tarball download.
>
> * guix/import/cran.scm (%download-source): New parameter.
> (description->package): Use it.
> ---
> guix/import/cran.scm | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
Well, I miss what it changes – I have nothing special to comment, so
LGTM. :-)
Cheers,
simon
Hi!
Lars-Dominik Braun <lars@6xq.net> skribis:
> the attached patches are required for guix-cran
> (https://github.com/guix-science/guix-cran). import/cran already has the
> ability to add a prefix to licenses, but it was not exposed. Additionally
> I need to parameterize fetch/download functions, so I can cache the
> tarballs/DESCRIPTION files.
Nice! Minor comments:
> +(define %license-prefix
> + (make-parameter identity))
Overall, unless it’s impractical, I’d suggest using explicit keyword
parameters instead of SRFI-39 parameters (like this one). That makes
things clearer. (That’s essentially lexical scope vs. dynamic scope.)
Also, when introducing a public global variable, please add a short
comment below the ‘define’ line explaining what it does.
> +++ b/guix/scripts/import/cran.scm
> @@ -53,6 +53,9 @@ (define (show-help)
> (display (G_ "
> -s, --style=STYLE choose output style, either specification or variable"))
> (display (G_ "
> + -p, --license-prefix=PREFIX
> + add custom prefix to licenses, useful for prefixed import of (guix licenses)"))
I agree with zimoun that this should be documented in the manual. I’d
also remove everything after the comma.
> +(define %fetch-description
> + (make-parameter fetch-description))
Same comment as above.
> +(define %download-source
> + (make-parameter download))
Ditto.
Thanks!
Ludo’.
Thank you for the patches! I made minor changes to the commit messages
(to replace “parameter” with “argument”), used multiple values with
SRFI-71 instead of cdr+cadr, adjusted expectations in a test, and fixed
a small bug in a follow-up commit.
@@ -55,6 +55,7 @@ (define-module (guix import cran)
#:use-module (guix packages)
#:use-module (gnu packages)
#:export (%input-style
+ %license-prefix
cran->guix-package
bioconductor->guix-package
@@ -82,6 +83,9 @@ (define-module (guix import cran)
(define %input-style
(make-parameter 'variable)) ; or 'specification
+(define %license-prefix
+ (make-parameter identity))
+
(define (string->licenses license-string)
(let ((licenses
(map string-trim-both
@@ -89,9 +93,9 @@ (define (string->licenses license-string)
(char-set-complement (char-set #\|))))))
(string->license licenses)))
-(define string->license
- (let ((prefix identity))
- (match-lambda
+(define (string->license license-string)
+ (let ((prefix (%license-prefix)))
+ (match license-string
("AGPL-3" (prefix 'agpl3))
("AGPL (>= 3)" (prefix 'agpl3+))
("Artistic-2.0" (prefix 'artistic2.0))
@@ -53,6 +53,9 @@ (define (show-help)
(display (G_ "
-s, --style=STYLE choose output style, either specification or variable"))
(display (G_ "
+ -p, --license-prefix=PREFIX
+ add custom prefix to licenses, useful for prefixed import of (guix licenses)"))
+ (display (G_ "
-V, --version display version information and exit"))
(newline)
(show-bug-report-information))
@@ -74,6 +77,10 @@ (define %options
(lambda (opt name arg result)
(alist-cons 'style (string->symbol arg)
(alist-delete 'style result))))
+ (option '(#\p "license-prefix") #t #f
+ (lambda (opt name arg result)
+ (alist-cons 'license-prefix arg
+ (alist-delete 'license-prefix result))))
(option '(#\r "recursive") #f #f
(lambda (opt name arg result)
(alist-cons 'recursive #t result)))
@@ -95,8 +102,15 @@ (define (parse-options)
(('argument . value)
value)
(_ #f))
- (reverse opts))))
- (parameterize ((%input-style (assoc-ref opts 'style)))
+ (reverse opts)))
+ (prefix (assoc-ref opts 'license-prefix))
+ (prefix-proc (if (string? prefix)
+ (lambda (symbol)
+ (string->symbol
+ (string-append prefix (symbol->string symbol))))
+ identity)))
+ (parameterize ((%input-style (assoc-ref opts 'style))
+ (%license-prefix prefix-proc))
(match args
((spec)
(let ((name version (package-name->name+version spec)))