diff mbox

[1/2] import: json: Silence json-fetch output.

Message ID 20161205050317.13222-1-bavier@member.fsf.org
State New
Headers show

Commit Message

Eric Bavier Dec. 5, 2016, 5:03 a.m. UTC
* guix/import/json.scm (json-fetch): Use http-fetch instead of url-fetch
to avoid writing to stdout and a temporary file for each invocation.
* guix/import/gem.scm (rubygems-fetch): Do not redirect json-fetch
output to /dev/null.
* guix/import/pypi.scm (pypi-fetch): Likewise.
---
 guix/import/gem.scm  | 10 ++--------
 guix/import/json.scm | 14 +++++++-------
 guix/import/pypi.scm | 10 ++--------
 3 files changed, 11 insertions(+), 23 deletions(-)

Comments

Ludovic Courtès Dec. 7, 2016, 10:59 a.m. UTC | #1
Eric Bavier <bavier@member.fsf.org> skribis:

> * guix/import/json.scm (json-fetch): Use http-fetch instead of url-fetch
> to avoid writing to stdout and a temporary file for each invocation.
> * guix/import/gem.scm (rubygems-fetch): Do not redirect json-fetch
> output to /dev/null.
> * guix/import/pypi.scm (pypi-fetch): Likewise.

[...]

>  (define (json-fetch url)
>    "Return an alist representation of the JSON resource URL, or #f on failure."
> -  (call-with-temporary-output-file
> -   (lambda (temp port)
> -     (and (url-fetch url temp)
> -          (hash-table->alist
> -           (call-with-input-file temp json->scm))))))
> +  (and=> (false-if-exception (http-fetch url))
> +         (lambda (port)
> +           (let ((result (hash-table->alist (json->scm port))))
> +             (close-port port)
> +             result))))

It’d be better to not catch exceptions raised by ‘http-fetch’ here.
Instead they’d be caught at the top level and a detailed error message
would be displayed, which is always better than silently ignoring
issues.

However we’d need to check if there are uses where this is a problem.
For example, there might be updaters or importers that assume that #f
means that the package doesn’t exist or something like that.

WDYT?

> diff --git a/guix/import/pypi.scm b/guix/import/pypi.scm
> index 68153d5..9794ff9 100644
> --- a/guix/import/pypi.scm
> +++ b/guix/import/pypi.scm
> @@ -51,14 +51,8 @@
>  (define (pypi-fetch name)
>    "Return an alist representation of the PyPI metadata for the package NAME,
>  or #f on failure."
> -  ;; XXX: We want to silence the download progress report, which is especially
> -  ;; annoying for 'guix refresh', but we have to use a file port.
> -  (call-with-output-file "/dev/null"
> -    (lambda (null)
> -      (with-error-to-port null
> -        (lambda ()
> -          (json-fetch (string-append "https://pypi.python.org/pypi/"
> -                                     name "/json")))))))
> +  (json-fetch (string-append "https://pypi.python.org/pypi/"
> +                             name "/json")))

The rest LGTM, thanks!

Ludo’.
Eric Bavier Dec. 8, 2016, 5:57 a.m. UTC | #2
On Wed, 07 Dec 2016 11:59:10 +0100
ludo@gnu.org (Ludovic Courtès) wrote:

> Eric Bavier <bavier@member.fsf.org> skribis:
> 
> > * guix/import/json.scm (json-fetch): Use http-fetch instead of url-fetch
> > to avoid writing to stdout and a temporary file for each invocation.
> > * guix/import/gem.scm (rubygems-fetch): Do not redirect json-fetch
> > output to /dev/null.
> > * guix/import/pypi.scm (pypi-fetch): Likewise.  
> 
> [...]
> 
> >  (define (json-fetch url)
> >    "Return an alist representation of the JSON resource URL, or #f on failure."
> > -  (call-with-temporary-output-file
> > -   (lambda (temp port)
> > -     (and (url-fetch url temp)
> > -          (hash-table->alist
> > -           (call-with-input-file temp json->scm))))))
> > +  (and=> (false-if-exception (http-fetch url))
> > +         (lambda (port)
> > +           (let ((result (hash-table->alist (json->scm port))))
> > +             (close-port port)
> > +             result))))  
> 
> It’d be better to not catch exceptions raised by ‘http-fetch’ here.
> Instead they’d be caught at the top level and a detailed error message
> would be displayed, which is always better than silently ignoring
> issues.
> 
> However we’d need to check if there are uses where this is a problem.
> For example, there might be updaters or importers that assume that #f
> means that the package doesn’t exist or something like that.
> 
> WDYT?

The importers that use json-fetch all do something like '(and=> meta
->package)', so a #f result from json-fetch is passed up to (@ (guix
scripts import) guix-import) and interpreted as a "import failed".

Using the false-if-exception* syntax which prints the exception message,
from guix/build/download.scm might be nicer.  This is the syntax
that url-fetch uses.  That syntax would need to be exported from
somewhere.  WDYT?

`~Eric
Ludovic Courtès Dec. 8, 2016, 9:52 a.m. UTC | #3
Eric Bavier <ericbavier@centurylink.net> skribis:

> On Wed, 07 Dec 2016 11:59:10 +0100
> ludo@gnu.org (Ludovic Courtès) wrote:
>
>> Eric Bavier <bavier@member.fsf.org> skribis:
>> 
>> > * guix/import/json.scm (json-fetch): Use http-fetch instead of url-fetch
>> > to avoid writing to stdout and a temporary file for each invocation.
>> > * guix/import/gem.scm (rubygems-fetch): Do not redirect json-fetch
>> > output to /dev/null.
>> > * guix/import/pypi.scm (pypi-fetch): Likewise.  
>> 
>> [...]
>> 
>> >  (define (json-fetch url)
>> >    "Return an alist representation of the JSON resource URL, or #f on failure."
>> > -  (call-with-temporary-output-file
>> > -   (lambda (temp port)
>> > -     (and (url-fetch url temp)
>> > -          (hash-table->alist
>> > -           (call-with-input-file temp json->scm))))))
>> > +  (and=> (false-if-exception (http-fetch url))
>> > +         (lambda (port)
>> > +           (let ((result (hash-table->alist (json->scm port))))
>> > +             (close-port port)
>> > +             result))))  
>> 
>> It’d be better to not catch exceptions raised by ‘http-fetch’ here.
>> Instead they’d be caught at the top level and a detailed error message
>> would be displayed, which is always better than silently ignoring
>> issues.
>> 
>> However we’d need to check if there are uses where this is a problem.
>> For example, there might be updaters or importers that assume that #f
>> means that the package doesn’t exist or something like that.
>> 
>> WDYT?
>
> The importers that use json-fetch all do something like '(and=> meta
> ->package)', so a #f result from json-fetch is passed up to (@ (guix

OK.

So if we instead “let the exception through”, ‘guix import’ would
something like “failed to download from http://…: 404 (Not Found)”,
which is worse than “package not found in repo” in this case.

A middle ground would be this:

  (guard (c ((http-get-error? c)
             (if (= 404 (http-get-error-code c))
                 #f ;this is “expected”, just means the package isn’t known
                 (raise c)))) ;using (srfi srfi-34) here
    (let* ((port (json->scm port)))
      …))

That way, 404 would still be treated as an expected error meaning
“package does not exist in upstream repo”, but more problematic errors
(DNS lookup errors, 403, etc.) would go through.

How does that sound?

Thanks,
Ludo’.
David Craven Dec. 14, 2016, 3:16 p.m. UTC | #4
I think this commit broke the pypi tests.
Ludovic Courtès Dec. 15, 2016, 5:37 p.m. UTC | #5
David Craven <david@craven.ch> skribis:

> I think this commit broke the pypi tests.

Indeed, because it’s a different procedure that needs to be mocked now.

Eric, could you have a look?

Thanks for the heads-up!

Ludo’.
diff mbox

Patch

diff --git a/guix/import/gem.scm b/guix/import/gem.scm
index 3d0c190..3ad7fac 100644
--- a/guix/import/gem.scm
+++ b/guix/import/gem.scm
@@ -38,14 +38,8 @@ 
 (define (rubygems-fetch name)
   "Return an alist representation of the RubyGems metadata for the package NAME,
 or #f on failure."
-  ;; XXX: We want to silence the download progress report, which is especially
-  ;; annoying for 'guix refresh', but we have to use a file port.
-  (call-with-output-file "/dev/null"
-    (lambda (null)
-      (with-error-to-port null
-        (lambda ()
-          (json-fetch
-           (string-append "https://rubygems.org/api/v1/gems/" name ".json")))))))
+  (json-fetch
+   (string-append "https://rubygems.org/api/v1/gems/" name ".json")))
 
 (define (ruby-package-name name)
   "Given the NAME of a package on RubyGems, return a Guix-compliant name for
diff --git a/guix/import/json.scm b/guix/import/json.scm
index c3092a5..f0d75fd 100644
--- a/guix/import/json.scm
+++ b/guix/import/json.scm
@@ -1,6 +1,6 @@ 
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2014 David Thompson <davet@gnu.org>
-;;; Copyright © 2015 Eric Bavier <bavier@member.fsf.org>
+;;; Copyright © 2015, 2016 Eric Bavier <bavier@member.fsf.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -19,14 +19,14 @@ 
 
 (define-module (guix import json)
   #:use-module (json)
-  #:use-module (guix utils)
+  #:use-module (guix http-client)
   #:use-module (guix import utils)
   #:export (json-fetch))
 
 (define (json-fetch url)
   "Return an alist representation of the JSON resource URL, or #f on failure."
-  (call-with-temporary-output-file
-   (lambda (temp port)
-     (and (url-fetch url temp)
-          (hash-table->alist
-           (call-with-input-file temp json->scm))))))
+  (and=> (false-if-exception (http-fetch url))
+         (lambda (port)
+           (let ((result (hash-table->alist (json->scm port))))
+             (close-port port)
+             result))))
diff --git a/guix/import/pypi.scm b/guix/import/pypi.scm
index 68153d5..9794ff9 100644
--- a/guix/import/pypi.scm
+++ b/guix/import/pypi.scm
@@ -51,14 +51,8 @@ 
 (define (pypi-fetch name)
   "Return an alist representation of the PyPI metadata for the package NAME,
 or #f on failure."
-  ;; XXX: We want to silence the download progress report, which is especially
-  ;; annoying for 'guix refresh', but we have to use a file port.
-  (call-with-output-file "/dev/null"
-    (lambda (null)
-      (with-error-to-port null
-        (lambda ()
-          (json-fetch (string-append "https://pypi.python.org/pypi/"
-                                     name "/json")))))))
+  (json-fetch (string-append "https://pypi.python.org/pypi/"
+                             name "/json")))
 
 ;; For packages found on PyPI that lack a source distribution.
 (define-condition-type &missing-source-error &error