[bug#48697] [PATCH] import: Add CHICKEN egg importer.

["Ludovic Courtès" <ludo@gnu.org>]

Hello!

Xinglu Chen <public@yoctocell.xyz> skribis:

> * guix/import/egg.scm: New file.
> * guix/scripts/import/egg.scm: New file.
> * tests/egg.scm: New file.
> * Makefile.am (MODULES, SCM_TESTS): Register them.
> * guix/scripts/import.scm (importers): Add egg importer.
> * doc/guix.texi (Invoking guix import, Invoking guix refresh): Document it.

Woohoo, very nice!

> This patch adds recursive importer for CHICKEN eggs, the generated
> packages aren’t entirely complete, though.  It gets information from The
> PACKAGE.egg, which is just a Scheme file that contains a list of lists
> that specify the metadata for an egg.  However, it doesn’t specify a
> description, so I have just set the ‘description’ field to #f for now.
> The licensing policy for eggs is also a bit vague[1], there is no strict
> naming format for licenses, and a lot of eggs just specify ‘BSD’ rather
> than ‘BSD-N-Clause’.

On IRC, Mario Goulart of CHICKEN fame mentioned that they are working on
it, linking to this discussion:

  https://lists.nongnu.org/archive/html/chicken-hackers/2020-10/msg00003.html

So hopefully it’ll improve soon.  In the meantime, what this patch does
(leaving a question mark when licensing is vague) LGTM.

> The PACKAGE.egg file can also specify system dependencies, but there is
> no consistent format for this, sometimes strings are used, other times
> symbols are used, and sometimes the version of the package is also
> included.  The user will have to double check the names and make sure
> they are correct.  I am also unsure about whether the system
> dependencies should be ‘propagated-inputs’ or just ‘inputs’.

Probably just ‘inputs’, no?  Why would they need to be propagated?  For
Guile (and Python, etc.) they have to be propagated because you need
.scm and .go files to be in the search path; but with CHICKEN, I believe
you end up with .so files, so there’s probably no need for propagation?
Not sure actually.

> +  #:use-module (ice-9 string-fun)

Uh, first time I see this one (!).  Maybe add #:select to clarify why
it’s used for.

> +(define %eggs-home-page
> +  (make-parameter "https://api.call-cc.org/5/doc"))

On IRC, Mario suggested that a better value may be
"https://wiki.call-cc.org/egg/".

> +(define (get-eggs-repository)
> +  "Update or fetch the latest version of the eggs repository and return the path
> +to the repository."
> +  (let*-values (((url) "git://code.call-cc.org/eggs-5-latest")
> +                ((directory commit _)
> +                 (update-cached-checkout url)))
> +    directory))

I’d call it ‘eggs-repository’ (without ‘get-’).

Also, I recommend using srfi-71 instead of srfi-11 in new code.

> +(define (find-latest-version name)
> +  "Get the latest version of the egg NAME."
> +  (let ((directory (scandir (egg-directory name))))
> +    (if directory
> +        (last directory)
> +        (begin
> +          (format #t (G_ "Package not found in eggs repository: ~a~%") name)
> +          #f))))

This should be rendered with ‘warning’ from (guix diagnostics).

Or maybe it should be raised as a ‘formatted-message’ exception?

> +(define (get-metadata port)
> +  "Parse the egg metadata from PORT."
> +  (let ((content (read port)))
> +    (close-port port)
> +    content))
> +
> +(define (egg-metadata name)
> +  "Return the package metadata file for the egg NAME."
> +  (let ((version (find-latest-version name)))
> +    (if version
> +        (get-metadata (open-file
> +                       (string-append (egg-directory name) "/"
> +                                      version "/" name ".egg")
> +                       "r"))
> +        #f)))

Rather:

  (call-with-input-file (string-append …)
    read)

… and you can remove ‘get-metadata’.

> +(define (guix-name->egg-name name)
> +  "Return the CHICKEN egg name corresponding to the Guix package NAME."
> +  (if (string-prefix? package-name-prefix name)
> +      (substring name (string-length package-name-prefix))
> +      name))

Use ‘string-drop’ instead of ‘substring’; I find it slightly clearer.

> +(define (guix-package->egg-name package)
> +  "Return the CHICKEN egg name of the Guix CHICKEN PACKAGE."
> +  (let ((upstream-name (assoc-ref
> +                        (package-properties package)
> +                        'upstream-name))
> +        (name (package-name package)))
> +    (if upstream-name
> +        upstream-name
> +        (guix-name->egg-name name))))

This can be simplified a bit:

  (define (guix-package->egg-name package)
    (or (assq-ref (package-properties package) 'upstream-name)
                  (guix-name->egg-name (package-name package))))

> +(define (egg-package? package)
> +  "Check if PACKAGE is an CHICKEN egg package."
> +  (and (eq? (build-system-name (package-build-system package)) 'chicken)
> +       (string-prefix? package-name-prefix (package-name package))))

I suggest not relying on build system names; they’re just a debugging
aid.  Thus, replace the first ‘eq?’ with:

  (eq? (package-build-system package) chicken-build-system)

> +        (define (safe-append lst1 lst2)
> +          (match (list lst1 lst2)
> +            ((#f #f) #f)
> +            ((lst1 #f) lst1)
> +            ((#f lst2) lst2)
> +            (_ (append lst1 lst2))))

This looks like a weird interface.  I’d simply ensure you always
manipulate lists: empty lists or non-empty lists.

> +        (define egg-home-page
> +          (string-append (%eggs-home-page) "/" name))
> +
> +        (define egg-synopsis
> +          (let ((synopsis (assoc-ref egg-content 'synopsis)))
> +            (if (list? synopsis)
> +                (first synopsis)
> +                #f)))

Rather: (match (assoc-ref egg-content 'synopsis)
          ((synopsis) synopsis)
          (_ #f))

> +        (define egg-license
> +          (let ((license (assoc-ref egg-content 'license)))
> +            (if (list? license)
> +                ;; Multiple licenses are separated by `/'.
> +                (string->license (string-split (first license) #\/))
> +                #f)))

Just make it ‘egg-licenses’ (plural) since the ‘license’ field of
<package> can be a list, and then:

  (match (assoc-ref egg-content 'license)
    ((license)
     (map string->license (string-split license #\/)))
    (#f
     '()))

> +        (define (prettify-system-dependency name)
> +          (let ((name* (if (symbol? name)
> +                           (symbol->string name)
> +                           name)))
> +            ;; System dependecies sometimes have spaces and/or upper case

Typo (“dependencies”).

> +            ;; letters in them.
> +            ;;
> +            ;; There will probably still be some weird edge cases.
> +            (string-replace-substring (string-downcase name*) " " "")))

How about:

  (string-map (lambda (chr)
                (case chr
                  ((#\space) #\-)
                  (else chr)))
              (string-downcase name))

?

> +        (define* (egg-parse-dependency name #:key (system? #f))
> +          (let* ((name* (if (list? name)
> +                            (first name) ; (name version)
> +                            name))
> +                 (name (if system?
> +                           (prettify-system-dependency name*)
> +                           (maybe-symbol->string name*))))

Use ‘match’.

> +        (define egg-native-inputs
> +          (let* ((test-dependencies (assoc-ref egg-content
> +                                               'test-dependencies))
> +                 (build-dependencies (assoc-ref egg-content
> +                                                'build-dependencies))
> +                 (test+build-dependencies (safe-append
> +                                           test-dependencies
> +                                           build-dependencies)))
> +            (if (list? test+build-dependencies)
> +                (map egg-parse-dependency
> +                     test+build-dependencies)
> +                '())))

Use ‘match’.  Arrange so ‘test-dependencies’ and ‘build-dependencies’
are always lists so you don’t need ‘safe-append’.

Last, could you add files that contain translatable strings to
‘po/guix/POTFILES.in’?

Otherwise LGTM.  Could you send an updated patch?

Thank you!

Ludo’.