Hello Ricardo! Ricardo Wurmus <ricardo.wur...@mdc-berlin.de> writes:
>> From cfde6e09f8f8c692fe252d76ed27e8c50a9e5377 Mon Sep 17 00:00:00 2001 >> From: Maxim Cournoyer <maxim.courno...@gmail.com> >> 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 source. >> - (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 dirname >> #\-))) >> - (requires.txt (string-append dirname "/" pypi-name >> - ".egg-info" >> "/requires.txt")) >> - (exit-code >> - (parameterize ((current-error-port (%make-void-port >> "rw+")) >> - (current-output-port (%make-void-port >> "rw+"))) >> - (if (string=? "zip" extension) >> - (system* "unzip" archive "-d" dir requires.txt) >> - (system* "tar" "xf" archive "-C" dir >> requires.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=? "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’s work on the empty list directly. Here “match” 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 source archive:\ + (match requires.txt-files + (() + (warning (G_ "Cannot guess requirements from source archive:\ 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 “begin” here. Fixed. >> + (begin (warning (G_ "Cannot guess requirements from source >> archive:\ >> + no requires.txt file found.~%")) >> + (list '() '())))))) > > I know that this is from an earlier commit, but I don’t like the look of > “(list '() '())” at all :) > >> + (begin >> + (warning (G_ "Unsupported archive format; \ >> +cannot determine package dependencies from source archive: ~a~%") >> + (basename source-url)) >> (list '() '())))) > > Same here. Certainly there’s 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
