diff mbox series

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

Message ID 626e4718c45c95a7278460f132bd38e08835e9f4.1633533541.git.h.goebel@crazy-compilers.com
State Accepted
Headers show
Series [bug#42180,v2,01/23] guix: Add extracting-download. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue

Commit Message

Hartmut Goebel Oct. 6, 2021, 3:20 p.m. UTC
* guix/extracting-download.scm: New file
* Makefile.am (MODULES): Add it.
---
 Makefile.am                  |   1 +
 guix/extracting-download.scm | 179 +++++++++++++++++++++++++++++++++++
 2 files changed, 180 insertions(+)
 create mode 100644 guix/extracting-download.scm

Comments

Ludovic Courtès Oct. 7, 2021, 9:55 p.m. UTC | #1
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’.
Tobias Geerinckx-Rice Oct. 7, 2021, 10:25 p.m. UTC | #2
Ludovic Courtès 写道:
> 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.

Yes, I think we should revert the series for now.  Though let's 
give maintainers [CC'd] a day (or so) to chime in.

Kind regards,

T G-R
Tobias Geerinckx-Rice Oct. 7, 2021, 10:34 p.m. UTC | #3
Tobias Geerinckx-Rice 写道:
> Yes, I think we should revert the series for now.  Though let's 
> give
> maintainers [CC'd] a day (or so) to chime in.

…that is, unless you want to go ahead & do so already, Hartmut :-) 
No need to wait if you agree.

Kind regards,

T G-R
M Oct. 8, 2021, 5:49 a.m. UTC | #4
Ludovic Courtès schreef op do 07-10-2021 om 23:55 [+0200]:
> Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:
> 
> > * guix/extracting-download.scm: New file
> > * Makefile.am (MODULES): Add it.

One potential problem, is that there doesn't seem to be code to fallback
to SWH.  Does SWH work on tarballs inside tarballs?

Greetings,
Maxime.
Ludovic Courtès Oct. 8, 2021, 7:05 a.m. UTC | #5
Hi!

Maxime Devos <maximedevos@telenet.be> skribis:

> Ludovic Courtès schreef op do 07-10-2021 om 23:55 [+0200]:
>> Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:
>> 
>> > * guix/extracting-download.scm: New file
>> > * Makefile.am (MODULES): Add it.
>
> One potential problem, is that there doesn't seem to be code to fallback
> to SWH.  Does SWH work on tarballs inside tarballs?

Good point.  SWH may archive tarballs inside tarballs as-is, but we’ll
have to check on concrete examples.

Ludo’.
Hartmut Goebel Oct. 8, 2021, 9:10 a.m. UTC | #6
Hi,

I'm sorry for the trouble. I'll revert the series - if not yet done by 
someone else.
Hartmut Goebel Oct. 8, 2021, 9:39 a.m. UTC | #7
Am 08.10.21 um 11:10 schrieb Hartmut Goebel:
> I'll revert the series

I'd prefer if someone else would revert the series. I'm unsure about how 
you want this to be done and I didn't spot an appropriate example for 
reverting a series in the commit history.

Thanks - and sorry again for the trouble!
Tobias Geerinckx-Rice Oct. 8, 2021, 9 p.m. UTC | #8
Hartmut,

Thanks for the quick reply!

Hartmut Goebel 写道:
> Am 08.10.21 um 11:10 schrieb Hartmut Goebel:
>> I'll revert the series
>
> I'd prefer if someone else would revert the series. I'm unsure 
> about
> how you want this to be done and I didn't spot an appropriate 
> example
> for reverting a series in the commit history.

I saw no value in 23 noise commits, so here's what I did:

  $ git log f63c79bf7674df012517f8e9148f94c611e35f32\
    ..f86f7e24b39928247729020df0134e2e1c4cde62 --format=oneline |
    while read c _; do git revert --no-commit $c; done

Then just used the standard git reversion message, but with a 
commit range[0].

> Thanks - and sorry again for the trouble!

Absolutely no worries.

Kind regards,

T G-R

[0]: 
https://git.savannah.gnu.org/cgit/guix.git/commit/?id=a1679b74c9aa20bb51bc4add82ebb7ba78926b9c
diff mbox series

Patch

diff --git a/Makefile.am b/Makefile.am
index b66789fa0b..f2b6c8e8da 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -96,6 +96,7 @@  MODULES =					\
   guix/discovery.scm				\
   guix/android-repo-download.scm		\
   guix/bzr-download.scm            		\
+  guix/extracting-download.scm			\
   guix/git-download.scm				\
   guix/hg-download.scm				\
   guix/swh.scm					\
diff --git a/guix/extracting-download.scm b/guix/extracting-download.scm
new file mode 100644
index 0000000000..4b7dcc7e83
--- /dev/null
+++ 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 file is part of GNU Guix.
+;;;
+;;; GNU Guix is free software; you can redistribute it and/or modify it
+;;; under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation; either version 3 of the License, or (at
+;;; your option) any later version.
+;;;
+;;; GNU Guix is distributed in the hope that it will be useful, but
+;;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
+
+(define-module (guix extracting-download)
+  #:use-module (ice-9 match)
+  #:use-module (ice-9 popen)
+  #:use-module ((guix build download) #:prefix build:)
+  #:use-module ((guix build utils) #:hide (delete))
+  #:use-module (guix gexp)
+  #:use-module (guix modules)
+  #:use-module (guix monads)
+  #:use-module (guix packages) ;; for %current-system
+  #:use-module (guix store)
+  #:use-module (guix utils)
+  #:use-module (srfi srfi-26)
+  #:export (http-fetch/extract
+            download-to-store/extract))
+
+;;;
+;;; Produce fixed-output derivations with data extracted from n archive
+;;; fetched over HTTP or FTP.
+;;;
+;;; This is meant to be used for package repositories where the actual source
+;;; archive is packed into another archive, eventually carrying meta-data.
+;;; Using this derivation saves both storing the outer archive and extracting
+;;; the actual one at build time.  The hash is calculated on the actual
+;;; archive to ease validating the stored file.
+;;;
+
+(define* (http-fetch/extract url filename-to-extract hash-algo hash
+                    #:optional name
+                    #:key (system (%current-system)) (guile (default-guile)))
+  "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)
+        #~(begin
+            (use-modules (guix build download)
+                         (guix build utils)
+                         (guix utils)
+                         (web uri)
+                         (ice-9 match)
+                         (ice-9 popen))
+            ;; The code below expects tar to be in $PATH.
+            (set-path-environment-variable "PATH" '("bin")
+                                           (match '#+inputs
+                                             (((names dirs outputs ...) ...)
+                                              dirs)))
+
+            (setvbuf (current-output-port) 'line)
+            (setvbuf (current-error-port) 'line)
+
+            (call-with-temporary-directory
+             (lambda (directory)
+               ;; TODO: Support different archive types, based on content-type
+               ;; or archive name extention.
+               (let* ((file-to-extract (getenv "extract filename"))
+                      (port (http-fetch (string->uri (getenv "download url"))
+                                        #:verify-certificate? #f))
+                      (tar (open-pipe* OPEN_WRITE "tar" "-C" directory
+                                       "-xf" "-" file-to-extract)))
+                 (dump-port port tar)
+                 (close-port port)
+                 (let ((status (close-pipe tar)))
+                   (unless (zero? status)
+                     (error "tar extraction failure" status)))
+                 (copy-file (string-append directory "/"
+                                           (getenv "extract filename"))
+                            #$output))))))))
+
+  (mlet %store-monad ((guile (package->derivation guile system)))
+    (gexp->derivation (or name file-name) build
+
+                      ;; Use environment variables and a fixed script name so
+                      ;; there's only one script in store for all the
+                      ;; downloads.
+                      #:script-name "extract-download"
+                      #:env-vars
+                      `(("download url" . ,url)
+                        ("extract filename" . ,filename-to-extract))
+                      #:leaked-env-vars '("http_proxy" "https_proxy"
+                                          "LC_ALL" "LC_MESSAGES" "LANG"
+                                          "COLUMNS")
+                      #:system system
+                      #:local-build? #t           ; don't offload download
+                      #:hash-algo hash-algo
+                      #:hash hash
+                      #:guile-for-build guile)))
+
+
+(define* (download-to-store/extract store url filename-to-extract
+                                    #:optional (name (basename url))
+                                    #:key (log (current-error-port))
+                                    (verify-certificate? #t))
+  "Download an archive from URL, and extracts FILE_TO_EXTRACT from the archive
+to STORE, either under NAME or URL's basename if omitted.  Write progress
+reports to LOG.  VERIFY-CERTIFICATE? determines whether or not to validate
+HTTPS server certificates."
+  (call-with-temporary-output-file
+   (lambda (temp port)
+     (let ((result
+            (parameterize ((current-output-port log))
+              (build:url-fetch url temp
+                               ;;#:mirrors %mirrors
+                               #:verify-certificate?
+                               verify-certificate?))))
+       (close port)
+       (and result
+            (call-with-temporary-output-file
+             (lambda (contents port)
+               (let ((tar (open-pipe* OPEN_READ
+                                      "tar"  ;"--auto-compress"
+                                      "-xf" temp "--to-stdout" filename-to-extract)))
+                 (dump-port tar port)
+                 (close-port port)
+                 (let ((status (close-pipe tar)))
+                   (unless (zero? status)
+                     (error "tar extraction failure" status)))
+                 (add-to-store store name #f "sha256" contents)))))))))