GNU bug report logs - #73833
[PATCH] guix: import: composer: Improve composer-fetch.

Previous Next

Full log

Message #33 received at 73833 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Nicolas Graves <ngraves <at> ngraves.fr>
Cc: 73833 <at> debbugs.gnu.org
Subject: Re: [bug#73833] [PATCH v2 3/5] guix: import: composer: Improve
 importer.
Date: Wed, 06 Nov 2024 16:41:10 +0100

Nicolas Graves <ngraves <at> ngraves.fr> skribis:

> * guix/import/composer.scm
> (%composer-base-url): Move from here...
> (%packagist-base-url): ...to here.
> (requirements->prefixes): Add variable to read and take advantage of
> version info in composer package requirements and...
> (json->require): ...use it here. Rewrite of the function.
> (composer-source): Add a sanitizer for composer-source-url.
> (select-version): Add variable to select the most recent availble
> version that is above to a given min-version and...
> (composer-fetch): ...use it here. Improve the function.
> (make-php-sexp, composer->guix-package): Adapt to requirements being
> alists now.
> (php-package?): Handle the particular phpunit case.
> (dependency->input): Add min-version and max-version information. This
> is currently limited to the first dependency suggested by
> requirements.
> (import-release): Fix git urls case. This is better but still a bit
> buggy (refreshing can replace the version by a commit).
>
> * tests/composer.scm
> (%composer-base-url): Move from here...
> (%packagist-base-url): ...to here.

[...]

> -(define %composer-base-url
> +(define %packagist-base-url
>    (make-parameter "https://repo.packagist.org"))

It would be best to have this in a separate patch.

> +(define (requirements->prefixes str)
> +  (let* ((processed-str (string-replace-substring str " || " "|"))
> +         (prefix-strs (string-split processed-str #\|)))
> +    (filter-map (match-lambda
> +                  ;; SemVer: ^ indicates major+minor match, not a whole match.
> +                  ((? (cut string-prefix? "^" <>) prefix)
> +                   (let ((pfx (string-drop prefix 1)))
> +                     (if (eq? 2 (string-count prefix #\.))
> +                         (string-take pfx (string-rindex pfx #\.))
> +                         pfx)))
> +                  ((? (cut string-suffix? ".*" <>) prefix)
> +                   (string-drop-right prefix 2))
> +                  (_ #f))
> +         prefix-strs)))

Please add a docstring and following the coding style for identifiers.

>  (define-json-mapping <composer-source> make-composer-source composer-source?
>    json->composer-source
>    (type      composer-source-type)
> -  (url       composer-source-url)
> +  (url       composer-source-url "url"
> +             (lambda (uri)
> +               (if (string-suffix? ".git" uri)
> +                   (string-drop-right uri 4)
> +                   uri)))

Is it the right place to change things?  I would keep <composer-source>
identical to the information available at packagist.org, and
post-process the URL only where it’s needed.

> +(define* (select-version packages #:key (min-version #f))
> +  "Select the most recent available version in the PACKAGES list
> +that is above or equal to MIN-VERSION. MIN-VERSION can be incomplete
> +(e.g. version-major only)."
> +  (let* ((points (and min-version (string-count min-version #\.)))
> +         (min-prefix (and min-version
> +                          (match points
> +                            ((or 0 1) (fix-version min-version))
> +                            (_ #f)))))
> +    (cdr
> +     (fold
> +      (lambda (new cur-max)
> +        (match new
> +          (((? valid-version? version) . tail)
> +           (let ((valid-version (fix-version version)))
> +             (if (and (version>? valid-version (fix-version (car cur-max)))
> +                      (or (not min-prefix)
> +                          (version-prefix? min-prefix valid-version)))
> +                 (cons* version tail)
> +                 cur-max)))
> +          (_ cur-max)))

No car/cdr please, especially since it’s already using ‘match’.

> +  (let* ((version (fix-version (caadr dependency)))
> +         (points (and version (string-count version #\.)))
> +         (max "99"))
> +    (upstream-input
> +     (name (car dependency))
> +     (downstream-name (php-package-name (car dependency)))
> +     (type type)
> +     (min-version (match points
> +                    (0 (string-append version ".0.0"))
> +                    (1 (string-append version ".0"))
> +                    (2 version)
> +                    (_ 'any)))
> +     (max-version (match points
> +                    (0 (string-append version "." max "." max))
> +                    (1 (string-append version "." max))
> +                    (2 version)
> +                    (_ 'any))))))

This thing with ‘points’ is kinda weird: ‘version>?’ works fine even
when there’s a different number of points in the arguments.

>  (define* (import-release package #:key (version #f))
>    "Return an <upstream-source> for VERSION or the latest release of PACKAGE."
>    (let* ((php-name (guix-package->composer-name package))
> -         (composer-package (composer-fetch php-name #:version version)))
> +         (composer-package (composer-fetch php-name #:version version))
> +         (new-version new-version-tag
> +                      (latest-git-tag-version package #:version version)))
>      (if composer-package
>          (upstream-source
>           (package (composer-package-name composer-package))
>           (version (composer-package-version composer-package))
> -         (urls (list (composer-source-url
> -                      (composer-package-source composer-package))))
> +         (urls
> +          (let ((source (composer-package-source composer-package)))
> +            (if (string=? (composer-source-type source) "git")
> +                (git-reference
> +                 (url (composer-source-url source))
> +                 (commit (or new-version-tag
> +                             (composer-source-reference source))))
> +                (list (composer-source-url source)))))

This seems to be an unrelated change (supporting fetching source from
Git).  Could it be separated out?

Could you add tests for the new functionality?

It’s important to have good test coverage to help maintain importers in
the long run.

Thanks,
Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.