Re: [PATCH 1/2] import: json: Silence json-fetch output.
On Thu, 15 Dec 2016 18:37:34 +0100 l...@gnu.org (Ludovic Courtès) wrote: > David Craven 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! Ricardo seems to have fixed this fallout before I had a chance to. Thanks Ricardo! Commits 662a1aa6b049d29977cfc376d4a185a3e8be4a07, e69c1a544606d6870eef959c3cda17fe6bdce875, and 506abddb99e02f824bff7ed7d7f7b37c4dafe0a7. `~Eric
Re: [PATCH 1/2] import: json: Silence json-fetch output.
David Craven 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’.
Re: [PATCH 1/2] import: json: Silence json-fetch output.
I think this commit broke the pypi tests.
Re: [PATCH 1/2] import: json: Silence json-fetch output.
Eric Bavier skribis: > On Wed, 07 Dec 2016 11:59:10 +0100 > l...@gnu.org (Ludovic Courtès) wrote: > >> Eric Bavier 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’.
Re: [PATCH 1/2] import: json: Silence json-fetch output.
On Wed, 07 Dec 2016 11:59:10 +0100 l...@gnu.org (Ludovic Courtès) wrote: > Eric Bavier 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
Re: [PATCH 1/2] import: json: Silence json-fetch output.
Eric Bavier 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’.
[PATCH 1/2] import: json: Silence json-fetch output.
* 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(-) 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 -;;; Copyright © 2015 Eric Bavier +;;; Copyright © 2015, 2016 Eric Bavier ;;; ;;; 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 -- 2.10.2
