Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> writes:
Toggle quote (2 lines)
> Next up: Seven of Nine, tertiary adjunct of unimatrix zero one:
Ehe! I had to look up the reference; I'm not much of a Star Trek fan
obviously :-P.
Toggle quote (14 lines)
>> From 37e499d5d5d5f690aa0a065c730e13f6a31dd30d Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
>> Date: Thu, 28 Mar 2019 23:12:26 -0400
>> Subject: [PATCH 7/9] import: pypi: Include optional test inputs as
>> native-inputs.
>>
>> * guix/import/pypi.scm (maybe-inputs): Add INPUT-TYPE argument, and use it.
>> (test-section?): New predicate.
>> (parse-requires.txt): Collect the optional test inputs, and return them as the
>> second element of the returned list.
>
> AFAICT parse-requires.txt now returns a list of pairs, but used to
> return a plain list before. Is this correct?
Right, a list of two lists to be technically correct.
Toggle quote (21 lines)
>> (define (parse-requires.txt requires.txt)
>> - "Given REQUIRES.TXT, a Setuptools requires.txt file, return a list of
>> -requirement names."
>> - ;; This is a very incomplete parser, which job is to select the non-optional
>> - ;; dependencies and strip them out of any version information.
>> + "Given REQUIRES.TXT, a Setuptools requires.txt file, return a pair of requirements.
>> +
>> +The first element of the pair contains the required dependencies while the
>> +second the optional test dependencies. Note that currently, optional,
>> +non-test dependencies are omitted since these can be difficult or expensive to
>> +satisfy."
>> +
>> + ;; This is a very incomplete parser, which job is to read in the requirement
>> + ;; specification lines, and strip them out of any version information.
>> ;; Alternatively, we could implement a PEG parser with the (ice-9 peg)
>> ;; library and the requirements grammar defined by PEP-0508
>> ;; (https://www.python.org/dev/peps/pep-0508/).
>
> Does it really return a pair? Or a list of pairs? Or is it a
> two-element list of lists?
The latter! I've fixed the docstring accordingly.
Toggle quote (25 lines)
>> (call-with-input-file requires.txt
>> (lambda (port)
>> - (let loop ((result '()))
>> + (let loop ((required-deps '())
>> + (test-deps '())
>> + (inside-test-section? #f)
>> + (optional? #f))
>> (let ((line (read-line port)))
>> - ;; Stop when a section is encountered, as sections contains optional
>> - ;; (extra) requirements. Non-optional requirements must appear
>> - ;; before any section is defined.
>> - (if (or (eof-object? line) (section-header? line))
>> + (if (eof-object? line)
>> ;; Duplicates can occur, since the same requirement can be
>> ;; listed multiple times with different conditional markers, e.g.
>> ;; pytest >= 3 ; python_version >= "3.3"
>> ;; pytest < 3 ; python_version < "3.3"
>> - (reverse (delete-duplicates result))
>> + (map (compose reverse delete-duplicates)
>> + (list required-deps test-deps))
>
> Looks like a list of lists to me. “delete-duplicates” now won’t delete
> a name that is in both “required-deps” as well as in “test-deps”. Is
> this acceptable?
It is acceptable, as this corner case cannot exist given the current
code (a requirement can exist in either required-deps or test-deps, but
never in both). It also doesn't make sense that a run time requirement
would also be listed as a test requirement, so that corner case is not
likely to exist in the future either.
Toggle quote (3 lines)
> Personally, I’m not a fan of using data structures for returning
> multiple values, because we can simply return multiple values.
I thought the Guile supported multiple values return value would be
great here as well, but I've found that for this specific case here, a
list of lists worked better, since the two lists contain requirements to
be processed the same, which "map" can readily do (i.e. less ceremony is
required).
Toggle quote (5 lines)
> Or we could have more than just strings. The meaning of these strings
> is provided by the bin into which they are thrown — either
> “required-deps” or “test-deps”. It could be an option to collect tagged
> values instead and have the caller deal with filtering.
Sounds neat, but I'd rather punt on this one for now.
Toggle quote (11 lines)
>> (define (parse-wheel-metadata metadata)
>> - "Given METADATA, a Wheel metadata file, return a list of requirement names."
>> + "Given METADATA, a Wheel metadata file, return a pair of requirements.
>> +
>> +The first element of the pair contains the required dependencies while the second the optional
>> +test dependencies. Note that currently, optional, non-test dependencies are
>> +omitted since these can be difficult or expensive to satisfy."
>> ;; METADATA is a RFC-2822-like, header based file.
>
> This sounds like this is going to duplicate the previous procedures.
Instead of duplicating the docstring, I'm now referring to that of
PARSE-REQUIRES.TXT for PARSE-WHEEL-METADATA. The two procedures share
the same interface, but implement a different parser, which consist of
mostly a loop + conditional branches. IMHO, it's not worth, or even,
desirable to try to merge those two parsers into one; I prefer to keep
the logic of two distinct parsers separate.
Toggle quote (16 lines)
>> (define (requires-dist-header? line)
>> ;; Return #t if the given LINE is a Requires-Dist header.
>> - (regexp-match? (string-match "^Requires-Dist: " line)))
>> + (string-match "^Requires-Dist: " line))
>>
>> (define (requires-dist-value line)
>> (string-drop line (string-length "Requires-Dist: ")))
>>
>> (define (extra? line)
>> ;; Return #t if the given LINE is an "extra" requirement.
>> - (regexp-match? (string-match "extra == " line)))
>> + (string-match "extra == '(.*)'" line))
>
> These hunks should be part of the previous patch where they were
> introduced. (See my comments there about “regexp-match?”.)
Toggle quote (8 lines)
>> + (define (test-requirement? line)
>> + (let ((extra-label (match:substring (extra? line) 1)))
>> + (and extra-label (test-section? extra-label))))
>
> You can use “and=>” instead of binding a name:
>
> (and=> (match:substring (extra? line) 1) test-section?)
Neat! I still don't have the reflex to use "and=>", thanks for reminding
me about it!
Toggle quote (31 lines)
>> (call-with-input-file metadata
>> (lambda (port)
>> - (let loop ((requirements '()))
>> + (let loop ((required-deps '())
>> + (test-deps '()))
>> (let ((line (read-line port)))
>> - ;; Stop at the first 'Provides-Extra' section: the non-optional
>> - ;; requirements appear before the optional ones.
>> (if (eof-object? line)
>> - (reverse (delete-duplicates requirements))
>> + (map (compose reverse delete-duplicates)
>> + (list required-deps test-deps))
>> (cond
>> ((and (requires-dist-header? line) (not (extra? line)))
>> (loop (cons (specification->requirement-name
>> (requires-dist-value line))
>> - requirements)))
>> + required-deps)
>> + test-deps))
>> + ((and (requires-dist-header? line) (test-requirement? line))
>> + (loop required-deps
>> + (cons (specification->requirement-name (requires-dist-value line))
>> + test-deps)))
>> (else
>> - (loop requirements)))))))))
>> + (loop required-deps test-deps))))))))) ;skip line
>
> Could you refactor this so that the other parser can be reused? If not,
> the same comments about “if” and “cond” and the use of pairs/lists
> instead of “values” applies here.
Done w.r.t. using only "cond" rather than "if" + "cond". As explained
above, the docstring's body now refer to the documentation of
PARSE-REQUIRES.TXT; otherwise I prefer to keep the parsers separate.
Toggle quote (30 lines)
>> (define (guess-requirements source-url wheel-url archive)
>> - "Given SOURCE-URL, WHEEL-URL and a ARCHIVE of the package, return a list
>> + "Given SOURCE-URL, WHEEL-URL and an ARCHIVE of the package, return a list
>> of the required packages specified in the requirements.txt file. ARCHIVE will
>> be extracted in a temporary directory."
>>
>> @@ -244,7 +289,10 @@ cannot determine package dependencies") (file-extension url))
>> (metadata (string-append dirname "/METADATA")))
>> (call-with-temporary-directory
>> (lambda (dir)
>> - (if (zero? (system* "unzip" "-q" wheel-archive "-d" dir metadata))
>> + (if (zero?
>> + (parameterize ((current-error-port (%make-void-port "rw+"))
>> + (current-output-port (%make-void-port "rw+")))
>> + (system* "unzip" wheel-archive "-d" dir metadata)))
>> (parse-wheel-metadata (string-append dir "/" metadata))
>> (begin
>> (warning
>> @@ -283,32 +331,41 @@ cannot determine package dependencies") (file-extension url))
>> (warning
>> (G_ "Failed to extract file: ~a from source.~%")
>> requires.txt)
>> - '())))))
>> - '())))
>> + (list '() '()))))))
>> + (list '() '()))))
>
> I would like to see cosmetic changes like these three hunks in separate
> commits.
Done for the first two hunks; the last one isn't cosmetic; it changes
the default return from an empty list to a list of two empty lists.
Toggle quote (46 lines)
>> (define (compute-inputs source-url wheel-url archive)
>> - "Given the SOURCE-URL of an already downloaded ARCHIVE, return a list of
>> -name/variable pairs describing the required inputs of this package. Also
>> + "Given the SOURCE-URL and WHEEL-URL of an already downloaded ARCHIVE, return
>> +a pair of lists, each consisting of a list of name/variable pairs, for the
>> +propagated inputs and the native inputs, respectively. Also
>> return the unaltered list of upstream dependency names."
>> - (let ((dependencies
>> - (remove (cut string=? "argparse" <>)
>> - (guess-requirements source-url wheel-url archive))))
>> - (values (sort
>> - (map (lambda (input)
>> - (let ((guix-name (python->package-name input)))
>> - (list guix-name (list 'unquote (string->symbol guix-name)))))
>> - dependencies)
>> - (lambda args
>> - (match args
>> - (((a _ ...) (b _ ...))
>> - (string-ci<? a b)))))
>> - dependencies)))
>> +
>> + (define (strip-argparse deps)
>> + (remove (cut string=? "argparse" <>) deps))
>> +
>> + (define (requirement->package-name/sort deps)
>> + (sort
>> + (map (lambda (input)
>> + (let ((guix-name (python->package-name input)))
>> + (list guix-name (list 'unquote (string->symbol guix-name)))))
>> + deps)
>> + (lambda args
>> + (match args
>> + (((a _ ...) (b _ ...))
>> + (string-ci<? a b))))))
>> +
>> + (define process-requirements
>> + (compose requirement->package-name/sort strip-argparse))
>> +
>> + (let ((dependencies (guess-requirements source-url wheel-url archive)))
>> + (values (map process-requirements dependencies)
>> + (concatenate dependencies))))
>
> Giving names to these processing steps is fine and improves clarity, but
> I’m not so comfortable about returning ad-hoc pairs *and* multiple
> values (like above).
Using "values" is required by the API of the recursive importer.
Toggle quote (28 lines)
>> + (match guix-dependencies
>> + ((required-inputs test-inputs)
>> + (values
>> + `(package
>> + (name ,(python->package-name name))
>> + (version ,version)
>> + (source (origin
>> + (method url-fetch)
>> + ;; Sometimes 'pypi-uri' doesn't quite work due to mixed
>> + ;; cases in NAME, for instance, as is the case with
>> + ;; "uwsgi". In that case, fall back to a full URL.
>> + (uri (pypi-uri ,(string-downcase name) version))
>> + (sha256
>> + (base32
>> + ,(guix-hash-url temp)))))
>> + (build-system python-build-system)
>> + ,@(maybe-inputs required-inputs 'propagated-inputs)
>> + ,@(maybe-inputs test-inputs 'native-inputs)
>> + (home-page ,home-page)
>> + (synopsis ,synopsis)
>> + (description ,description)
>> + (license ,(license->symbol license)))
>> + ;; Flatten the nested lists and return the upstream
>> + ;; dependencies.
>> + upstream-dependencies))))))))
>
> I don’t see anything being flattened here?
Good catch! Seems a remnant of something that is now gone :-).
Thanks for the review.
Maxim
