On Sat, May 29 2021, Ludovic Courtès wrote:

> 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

That’s great to see!

>> 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.

Ah, that makes sense, I am actually not that familiar with CHICKEN. :)

>> +  #:use-module (ice-9 string-fun)
>
> Uh, first time I see this one (!).  Maybe add #:select to clarify why
> it’s used for.

It’s needed for the ‘string-replace-substring’ procedure.  Here is the
relevant part of the Guile manual (in case you were curios)

     The following additional functions are available in the module
  ‘(ice-9 string-fun)’.  They can be used with:
  
       (use-modules (ice-9 string-fun))
  
   -- Scheme Procedure: string-replace-substring str substring replacement
       Return a new string where every instance of SUBSTRING in string STR
       has been replaced by REPLACEMENT.  For example:
  
            (string-replace-substring "a ring of strings" "ring" "rut")
            ⇒ "a rut of struts"

>> +(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/".

Indeed, that looks like a better page.

>> +(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.

Oh, I didn’t know about that, definitely looks nicer.

>> +(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?

Not sure if it should be an exception or not, if you run ‘guix import egg’
on a package that doesn’t exist, it will already throw an error

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix import egg lasdkfj
Package not found in eggs repository: lasdkfj
guix import: error: failed to download meta-data for package 'lasdkfj'
--8<---------------cut here---------------end--------------->8---

>> +(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’.

Good idea.

>> +            ;; 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))
>
> ?

That would also work, and then we don’t have to import (ice-9
string-fun). :)

>> +        (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’?

Sure!

> Otherwise LGTM.  Could you send an updated patch?
>
> Thank you!
>
> Ludo’.

Thanks for the review!  v2 will be coming. :)