From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxim Cournoyer Subject: bug#24450: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Sun, 16 Jun 2019 14:10:52 +0900 Message-ID: <878su22jz7.fsf@gmail.com> References: <87pnod7ot4.fsf@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:470:142:3::10]:57265) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hcNSV-000145-Sj for bug-guix@gnu.org; Sun, 16 Jun 2019 01:12:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hcNSU-0002BP-IQ for bug-guix@gnu.org; Sun, 16 Jun 2019 01:12:03 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:54233) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1hcNSU-0002BF-Eo for bug-guix@gnu.org; Sun, 16 Jun 2019 01:12:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1hcNSU-00024V-8C for bug-guix@gnu.org; Sun, 16 Jun 2019 01:12:02 -0400 Sender: "Debbugs-submit" Resent-Message-ID: In-Reply-To: (Ricardo Wurmus's message of "Tue, 28 May 2019 16:48:57 +0200") List-Id: Bug reports for GNU Guix List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-guix-bounces+gcggb-bug-guix=m.gmane.org@gnu.org Sender: "bug-Guix" To: Ricardo Wurmus Cc: 24450@debbugs.gnu.org Hello Ricardo! Ricardo Wurmus writes: >> From cfde6e09f8f8c692fe252d76ed27e8c50a9e5377 Mon Sep 17 00:00:00 2001 >> From: Maxim Cournoyer >> Date: Sat, 30 Mar 2019 23:13:26 -0400 >> Subject: [PATCH 8/9] import: pypi: Scan source archive to find requires.= txt >> file. > >> * guix/import/pypi.scm (use-modules): Use invoke from (guix build utils). >> (guess-requirements)[archive-root-directory]: Remove procedure. > > Oh, I guess I reviewed this procedure in vain :( > > Please modify the commits so that added procedures are not removed in > later commits. This is easier on the reviewer and makes for a clearer > commit history. Indeed; I'll be more careful about this is the future; sorry! I've squashed this commit along with the one enabling more archive types support, as this is where the modified (and later removed) procedure originated. >> (define (guess-requirements-from-source) >> ;; Return the package's requirements by guessing them from the sour= ce. >> - (let ((dirname (archive-root-directory source-url)) >> - (extension (file-extension source-url))) >> - (if (string? dirname) >> - (call-with-temporary-directory >> - (lambda (dir) >> - (let* ((pypi-name (string-take dirname (string-rindex dirn= ame #\-))) >> - (requires.txt (string-append dirname "/" pypi-name >> - ".egg-info" "/requires= .txt")) >> - (exit-code >> - (parameterize ((current-error-port (%make-void-por= t "rw+")) >> - (current-output-port (%make-void-po= rt "rw+"))) >> - (if (string=3D? "zip" extension) >> - (system* "unzip" archive "-d" dir requires.t= xt) >> - (system* "tar" "xf" archive "-C" dir require= s.txt))))) >> - (if (zero? exit-code) >> - (parse-requires.txt (string-append dir "/" requires.= txt)) >> - (begin >> - (warning >> - (G_ "Failed to extract file: ~a from source.~%") >> - requires.txt) >> - (list '() '())))))) >> + (if (compressed-file? source-url) >> + (call-with-temporary-directory >> + (lambda (dir) >> + (parameterize ((current-error-port (%make-void-port "rw+")) >> + (current-output-port (%make-void-port "rw+"))) >> + (if (string=3D? "zip" (file-extension source-url)) >> + (invoke "unzip" archive "-d" dir) >> + (invoke "tar" "xf" archive "-C" dir))) >> + (let ((requires.txt-files >> + (find-files dir (lambda (abs-file-name _) >> + (string-match "\\.egg-info/requires.txt$" >> + abs-file-name))))) >> + (if (> (length requires.txt-files) 0) > > Let=E2=80=99s work on the empty list directly. Here =E2=80=9Cmatch=E2=80= =9D would be better. Done, like this: --8<---------------cut here---------------start------------->8--- - (if (> (length requires.txt-files) 0) - (parse-requires.txt (first requires.txt-files)) - (begin (warning (G_ "Cannot guess requirements from sourc= e archive:\ + (match requires.txt-files + (() + (warning (G_ "Cannot guess requirements from source archiv= e:\ no requires.txt file found.~%")) - '()))))) + '()) + (else (parse-requires.txt (first requires.txt-files))))))) --8<---------------cut here---------------end--------------->8--- >> + (begin >> + (parse-requires.txt (first requires.txt-files))) > > No need for =E2=80=9Cbegin=E2=80=9D here. Fixed. >> + (begin (warning (G_ "Cannot guess requirements from so= urce archive:\ >> + no requires.txt file found.~%")) >> + (list '() '())))))) > > I know that this is from an earlier commit, but I don=E2=80=99t like the = look of > =E2=80=9C(list '() '())=E2=80=9D at all :) > >> + (begin >> + (warning (G_ "Unsupported archive format; \ >> +cannot determine package dependencies from source archive: ~a~%") >> + (basename source-url)) >> (list '() '())))) > > Same here. Certainly there=E2=80=99s a better return value. This might look ugly, but I can't think of a better return value, since using anything else would mean having to introduce extra logic in the callers, while it is now a correct value that needs no special case. Maxim