From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: [PATCH] import: pypi: Detect inputs. Date: Sun, 07 Jun 2015 22:03:34 +0200 Message-ID: <87d217uvzd.fsf@gnu.org> References: <874mp6iplj.fsf@fsf.org> <1433458614-4189-1-git-send-email-tipecaml@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:37463) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z1gnJ-0007tL-4D for guix-devel@gnu.org; Sun, 07 Jun 2015 16:03:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z1gnC-0006by-C0 for guix-devel@gnu.org; Sun, 07 Jun 2015 16:03:45 -0400 In-Reply-To: <1433458614-4189-1-git-send-email-tipecaml@gmail.com> (Cyril Roelandt's message of "Fri, 5 Jun 2015 00:56:54 +0200") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Cyril Roelandt Cc: guix-devel@gnu.org Cyril Roelandt skribis: > * guix/import/pypi.scm (compute-inputs, guess-requirements): New procedur= es. Nice! Please mention the =E2=80=98guix-hash-url=E2=80=99, =E2=80=98make-pypi-sexp= =E2=80=99, =E2=80=98python->package-name=E2=80=99, etc. changes and the tests. > +(define (maybe-inputs python->package-name inputs) > + (match inputs > + (() > + '()) > + ((inputs ...) > + `((,python->package-name (,'quasiquote ,inputs)))))) Please add a docstring. The first argument is misnamed: There=E2=80=99s on= ly on call site and its value is 'inputs, so I think you should remove this argument. > +(define (guess-requirements source-url tarball) > + "Given SOURCE-URL and a TARBALL of the package, return a list of the r= equired > +packages specified in the requirements.txt file." Perhaps this should mention that TARBALL is extracted in the current directory. > + (define (tarball-directory url) > + "Given the URL of the package's tarball, return the name of the dire= ctory > +that will be created upon decompressing it." Use comments instead of docstrings for internal procedures. Also mention the #f return value. > + (let ((basename (substring url (+ 1 (string-rindex url #\/))))) > + (cond The opening paren should be under the =E2=80=98e=E2=80=99 of =E2=80=98let= =E2=80=99. > + ((string-suffix? ".tar.gz" basename) > + (string-drop-right basename 7)) > + ((string-suffix? ".tar.bz2" basename) > + (string-drop-right basename 8)) > + (else #f)))) Would be nice to emit a warning like =E2=80=9Cunsupported archive format; c= annot determine package dependencies=E2=80=9D in the #f case. > + (if (zero? (system* "tar" "xf" tarball req-file)) > + (dynamic-wind > + (const #t) > + (lambda () > + (read-requirements req-file)) > + (lambda () > + (delete-file req-file) > + (rmdir dirname))) > + '())) When =E2=80=98tar=E2=80=99 exits with non-zero, it would be nice to report = the failure and exit value using =E2=80=98warning=E2=80=99 from (guix ui). > + ,@(maybe-inputs 'inputs > + (compute-inputs source-url temp)) Second line should be aligned with 'inputs. > + (match url > + ("https://pypi.python.org/pypi/foo/json" > + (with-output-to-file file-name > + (lambda () > + (display test-json)))) > + ("https://example.com/foo-1.0.0.tar.gz" > + (begin > + (mkdir "foo-1.0.0") > + (with-output-to-file "foo-1.0.0/requirements.txt" > + (lambda () > + (display test-requirements))) > + (system* "tar" "czvf" file-name "foo-1.0.0/") > + (system* "rm" "-rf" "foo-1.0.0") Use =E2=80=98delete-file-recursively=E2=80=99 here. Could you send an updated patch? Thank you! Ludo=E2=80=99.