Re: [PATCH 2/2] import: Add stackage importer and updater.

[Ludovic Courtès]

Hello Federico,

Federico Beffa <address@hidden> skribis:

> From 50891dc929a21327405c6a3b58638126456deeed Mon Sep 17 00:00:00 2001
> From: Federico Beffa <address@hidden>
> Date: Mon, 6 Feb 2017 18:19:26 +0100
> Subject: [PATCH 2/2] import: Add stackage importer and updater.
>
> * guix/import/stackage.scm: New file.
> * guix/scripts/import/stackage.scm: New file.
> * Makefile.am (MODULES): Add new files.
> * guix/scripts/import.scm (importers): Add "stackage".
> * guix/scripts/refresh.scm (%updaters): Add %stackage-updater.
> * doc/guix.texi (Invoking 'guix import'): Document the importer.
>   (Invoking 'guix refresh'): Add stackage to option --type valid values.

Neat!

[...]

> address@hidden stackage
> address@hidden stackage
> +The @code{stackage} importer is a wrapper around the @code{hackage} one.
> +It takes a package name, looks up the package version included in an LTS

For the first occurrence of “LTS”:
s/LTS/long-term support (LTS)/

> +;;;
> +;;; Hackage importer low-level help functions
> +;;;
> +
> +(define guix-package->hackage-name
> +  (@@ (guix import hackage) guix-package->hackage-name))
> +
> +(define hackage-fetch
> +  (@@ (guix import hackage) hackage-fetch))
> +
> +(define hackage-source-url
> +  (@@ (guix import hackage) hackage-source-url))
> +
> +(define hackage-cabal-url
> +  (@@ (guix import hackage) hackage-cabal-url))

I’d suggest exporting these procedures from (guix import hackage) rather
than using ‘@@’, which should really be a last resort.

> +(define %stackage-updater
> +  (upstream-updater
> +   (name 'stackage)
> +   (description "Updater for Stackage LTS packages")
> +   (pred (@@ (guix import hackage) hackage-package?))

Likewise here, or use (upstream-updater-predicate hackage-updater).

> +    (if name-version
> +        (hackage->guix-package name-version
> +                               #:include-test-dependencies?
> +                               include-test-dependencies?)
> +        (begin
> +          (format (current-error-port)
> +                  "guix import stackage: package not found: ~a~%" 
> package-name)
> +          #f))))

The ‘if’ is unnecessary here: (guix scripts import) produces an error
message when an importer returns something other than a ‘package’ sexp.

> --- a/guix/scripts/refresh.scm
> +++ b/guix/scripts/refresh.scm
> @@ -40,6 +40,7 @@
>    #:use-module (guix import elpa)
>    #:use-module (guix import cran)
>    #:use-module (guix import hackage)
> +  #:use-module (guix import stackage)
>    #:use-module (guix gnupg)
>    #:use-module (gnu packages)
>    #:use-module ((gnu packages commencement) #:select (%final-inputs))
> @@ -205,6 +206,7 @@ unavailable optional dependencies such as Guile-JSON."
>                   %elpa-updater
>                   %cran-updater
>                   %bioconductor-updater
> +                 %stackage-updater

Because the dependency on Guile-JSON is still optional, you should
remove the #:use-module above and this line and write:

  ((guix import stackage) => %stackage-updater)

I haven’t tested it, but this LGTM.

Thank you!

Ludo’.