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: Mon, 10 Jun 2019 22:30:45 +0900 Message-ID: <87blz55zzu.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]:46338) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1haKP8-0005jF-KB for bug-guix@gnu.org; Mon, 10 Jun 2019 09:32:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1haKP6-0006Rx-Kc for bug-guix@gnu.org; Mon, 10 Jun 2019 09:32:06 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:42625) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1haKP4-0006PJ-Ge for bug-guix@gnu.org; Mon, 10 Jun 2019 09:32:04 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1haKP4-0001Xv-7n for bug-guix@gnu.org; Mon, 10 Jun 2019 09:32:02 -0400 Sender: "Debbugs-submit" Resent-Message-ID: In-Reply-To: (Ricardo Wurmus's message of "Tue, 28 May 2019 12:23:53 +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 again! Ricardo Wurmus writes: > On to the next: > >> From 73e27235cac1275ba7671fd2364325cf5788cb3c Mon Sep 17 00:00:00 2001 >> From: Maxim Cournoyer >> Date: Thu, 28 Mar 2019 00:26:02 -0400 >> Subject: [PATCH 5/9] import: pypi: Support more types of archives. >> >> This change enables the PyPI importer to look for requirements in a sour= ce >> archive of a different type than "tar.gz" or "tar.bz2". > > Okay. > >> * guix/import/pypi.scm: (guess-requirements)[tarball-directory]: Rename = to... >> [archive-root-directory]: this. Use COMPRESSED-FILED? to determine if an >> archive is supported or not. > > Nitpick: please use =E2=80=9C...this.=E2=80=9D and leave two spaces betwe= en sentences. Done. > Typo: it should be COMPRESSED-FILE? Fixed. >> [guess-requirements-from-source]: Adapt to use the new method, and use u= nzip >> to extract ZIP archives. > > s/method/procedure/ Done. > Please also mention that =E2=80=9Ccompute-inputs=E2=80=9D has been adjust= ed. Done. >> - (define (tarball-directory url) >> - ;; Given the URL of the package's tarball, return the name of the d= irectory >> + (define (archive-root-directory url) >> + ;; Given the URL of the package's archive, return the name of the d= irectory >> ;; that will be created upon decompressing it. If the filetype is n= ot >> ;; supported, return #f. >> - ;; TODO: Support more archive formats. >> - (let ((basename (substring url (+ 1 (string-rindex url #\/))))) >> - (cond >> - ((string-suffix? ".tar.gz" basename) >> - (string-drop-right basename 7)) >> - ((string-suffix? ".tar.bz2" basename) >> - (string-drop-right basename 8)) >> - (else >> + (if (compressed-file? url) >> + (let ((root-directory (file-sans-extension (basename url)))) >> + (if (string=3D? "tar" (file-extension root-directory)) >> + (file-sans-extension root-directory) >> + root-directory)) >> (begin >> - (warning (G_ "Unsupported archive format: \ >> -cannot determine package dependencies")) >> - #f))))) >> + (warning (G_ "Unsupported archive format (~a): \ >> +cannot determine package dependencies") (file-extension url)) >> + #f))) > > I think the double application of file-sans-extension and the > intermediate variable name =E2=80=9Croot-directory=E2=80=9D for something= that is a file > is a little confusing, but I don=E2=80=99t have a better proposal (other = than to > replace file-extension and file-sans-extension with a match expression). Done, w.r.t. using "match": --8<---------------cut here---------------start------------->8--- @@ -198,10 +198,12 @@ be extracted in a temporary directory." ;; that will be created upon decompressing it. If the filetype is not ;; supported, return #f. (if (compressed-file? url) - (let ((root-directory (file-sans-extension (basename url)))) - (if (string=3D? "tar" (file-extension root-directory)) - (file-sans-extension root-directory) - root-directory)) + (match (file-sans-extension (basename url)) + (root-directory + (match (file-extension root-directory) + ("tar" + (file-sans-extension root-directory)) + (_ root-directory)))) (begin (warning (G_ "Unsupported archive format (~a): \ cannot determine package dependencies") (file-extension url)) --8<---------------cut here---------------end--------------->8--- >> (define (read-wheel-metadata wheel-archive) >> ;; Given WHEEL-ARCHIVE, a ZIP Python wheel archive, return the pack= age's >> @@ -246,16 +243,20 @@ cannot determine package dependencies")) > >> (define (guess-requirements-from-source) >> ;; Return the package's requirements by guessing them from the sour= ce. >> - (let ((dirname (tarball-directory source-url))) >> + (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 (%mak= e-void-port "rw+")) >> - (current-output-port (%ma= ke-void-port "rw+"))) >> - (system* "tar" "xf" tarball "-C" dir r= equires.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))))) > > I guess this is why I=E2=80=99m not too happy with this: we=E2=80=99re ch= ecking in > multiple places if the format is supported but then forget about this > again until the next time we need to do something to the file. I don't see much of a problem with the current design since there are two questions being answered: 1) What should be the directory name of the extracted package (retrieved from the base name of the archive). 2) What extractor should be used (zip vs tar). These two questions are orthogonal, and that the same primitive get used to answer both is an implementation, or rather, an optimization detail. > I wonder if we could do better and answer the question just once. The questions are different :-). We could optimize, but that would be at the price of expressiveness (squash the two questions into one solving space). What do you think? Maxim