[bug#42180] bug#51061: [PATCH v2 01/23] guix: Add extracting-download.

["Ludovic Courtès" <ludo@gnu.org>]

Hi Hartmut,

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> * guix/extracting-download.scm: New file
> * Makefile.am (MODULES): Add it.

I see you already pushed this change, but AFAICS it hasn’t seen any real
review—not great.  We don’t commit the whole project to supporting new
APIs at this level without first having collectively looked into them.

I’ll make some quick comments for now.  You might consider reverting to
leave people enough time to comment without pressure.

First, could you explain the rationale and use cases?

I can imagine reasons to do it this way, but also reasons to not do it
this way.

[...]

> +++ b/guix/extracting-download.scm
> @@ -0,0 +1,179 @@
> +;;; GNU Guix --- Functional package management for GNU
> +;;; Copyright © 2014, 2015, 2016, 2017, 2018, 2019 Ludovic Courtès <ludo@gnu.org>
> +;;; Copyright © 2017 Mathieu Lirzin <mthl@gnu.org>
> +;;; Copyright © 2017 Christopher Baines <mail@cbaines.net>
> +;;; Copyright © 2020 Jakub Kądziołka <kuba@kadziolka.net>
> +;;; Copyright © 2020 Hartmut Goebel <h.goebel@crazy-compilers.com>

This many people?  :-)

> +(define* (http-fetch/extract url filename-to-extract hash-algo hash
> +                    #:optional name
> +                    #:key (system (%current-system)) (guile (default-guile)))

Why ‘http-fetch’ when we have a generic ‘url-fetch’?

We’d rather like to see ‘url-fetch/extract’, and it should be expressed
in ~10 lines around (@ (guix download) url-fetch).

> +  "Return a fixed-output derivation that fetches an archive at URL, and
> +extracts FILE_TO_EXTRACT from the archive.  The FILE_TO_EXTRACT is expected to
> +have hash HASH of type HASH-ALGO (a symbol).  By default, the file name is the
> +base name of URL; optionally, NAME can specify a different file name."
> +  (define file-name
> +    (match url
> +      ((head _ ...)
> +       (basename head))
> +      (_
> +       (basename url))))
> +
> +  (define guile-zlib
> +    (module-ref (resolve-interface '(gnu packages guile)) 'guile-zlib))
> +
> +  (define guile-json
> +    (module-ref (resolve-interface '(gnu packages guile)) 'guile-json-4))
> +
> +  (define gnutls
> +    (module-ref (resolve-interface '(gnu packages tls)) 'gnutls))
> +
> +  (define inputs
> +    `(("tar" ,(module-ref (resolve-interface '(gnu packages base))
> +                          'tar))))
> +
> +  (define config.scm
> +    (scheme-file "config.scm"
> +                 #~(begin
> +                     (define-module (guix config)
> +                       #:export (%system))
> +
> +                     (define %system
> +                       #$(%current-system)))))
> +
> +  (define modules
> +    (cons `((guix config) => ,config.scm)
> +          (delete '(guix config)
> +                  (source-module-closure '((guix build download)
> +                                           (guix build utils)
> +                                           (guix utils)
> +                                           (web uri))))))
> +
> +  (define build
> +    (with-imported-modules modules
> +      (with-extensions (list guile-json gnutls ;for (guix swh)
> +                             guile-zlib)

This is really problematic: this code imports a ton of modules from the
host side.  (guix utils) is typically never imported on the build side
because it pulls in everything.  (web uri) must not be imported because
it’s part of Guile (I think there’s a warning for this).  All the
boilerplate above is because we’re importing the world.

> +(define* (download-to-store/extract store url filename-to-extract
> +                                    #:optional (name (basename url))
> +                                    #:key (log (current-error-port))
> +                                    (verify-certificate? #t))

What about this one?  What’s the intended use case?

Last, we’ve put a lot of effort over the years in properly documenting
things, like:

  https://guix.gnu.org/manual/en/html_node/origin-Reference.html#index-url_002dfetch

This should be held to the same standards.

Thanks,
Ludo’.