On Tue, Aug 10 2021, Alice BRENON wrote:

> - rephrase the documentation.
> - remove unnecessary #:ensure #f key in call to (cache-directory).
> - clarify what happens when string->uri fails and yields a warning by
>   passing a 'bad-repo symbol and commenting at its elimination site.
> - make a separate function of get-uri now that it's become as large as
>   its caller repo-type.
> - mention the possibility to call --repo several times in help message.
> - add points at the end of sentences in commit message and in this
>   e-mail.
> From 49e8236f81462501e89aa40d4e1b77bcc3fbb0ad Mon Sep 17 00:00:00 2001
> From: Alice BRENON <alice.brenon@ens-lyon.fr>
> Date: Sat, 7 Aug 2021 19:50:10 +0200
> Subject: [PATCH] guix: opam: More flexibility in the importer.
>
> * guix/scripts/import/opam.scm: pass all instances of --repo as a list
>   to the importer.
> * guix/import/opam.scm (opam-fetch): stop expecting "expanded"
>   repositories and call get-opam-repository instead to keep values
>   "symbolic" as long as possible and factorize.
>   (get-opam-repository): use the same repository source as CLI opam does
>   (i.e. HTTP-served index.tar.gz instead of git repositories).
>   (find-latest-version): be more flexible on the repositories structure
>   instead of expecting packages/PACKAGE-NAME/PACKAGE-NAME.VERSION/.
> * tests/opam.scm: update the call to opam->guix-package since repo is
>   now expected to be a list and remove the mocked get-opam-repository
>   deprecated by the support for local folders by the actual
>   implementation.
> * doc/guix.texi: document the new semantics and valid arguments for the
>   --repo option.
> ---
>  doc/guix.texi                |  25 ++++--
>  guix/import/opam.scm         | 158 +++++++++++++++++++++--------------
>  guix/scripts/import/opam.scm |   8 +-
>  tests/opam.scm               |  68 ++++++++-------
>  4 files changed, 155 insertions(+), 104 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 4eb5324b51..4a911e4c0f 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -94,6 +94,7 @@ Copyright @copyright{} 2021 Xinglu Chen@*
>  Copyright @copyright{} 2021 Raghav Gururajan@*
>  Copyright @copyright{} 2021 Domagoj Stolfa@*
>  Copyright @copyright{} 2021 Hui Lu@*
> +Copyright @copyright{} 2021 Alice Brenon@*
>  
>  Permission is granted to copy, distribute and/or modify this document
>  under the terms of the GNU Free Documentation License, Version 1.3 or
> @@ -11612,14 +11613,26 @@ Traverse the dependency graph of the given upstream package recursively
>  and generate package expressions for all those packages that are not yet
>  in Guix.
>  @item --repo
> -Select the given repository (a repository name).  Possible values include:
> +
> +By default, packages are searched in the official OPAM repository.  This
> +option, which can be used more than once, lets you add other
> +repositories where to look for packages.

“lets you add other repositories where to look for package” sounds a bit
weird, maybe

  lets you add other repositories which will be used to lookup packages.

?

>  @itemize
> -@item @code{opam}, the default opam repository,
> -@item @code{coq} or @code{coq-released}, the stable repository for coq packages,
> -@item @code{coq-core-dev}, the repository that contains development versions of coq,
> -@item @code{coq-extra-dev}, the repository that contains development versions
> -      of coq packages.
> +@item the name of a known repository - can be one of @code{opam},
> +      @code{coq} (equivalent to @code{coq-released}),
> +	  @code{coq-core-dev}, @code{coq-extra-dev} or @code{grew}.
> +@item the URL of a repository as expected by the @code{opam repository
> +      add} command (for instance, the URL equivalent of the above
> +	  @code{opam} name would be @uref{https://opam.ocaml.org}).
> +@item the path to a local copy of a repository (a directory containing a
> +      @file{packages/} sub-directory).
>  @end itemize
> +
> +Please note that repositories added with this option do not replace the
> +default @code{opam} repository, so calling this importer with the option
> +@code{--repo=opam} is redundant.

What happens if I specify an additional repository, and a package exists
in that repository _and_ the default opam repository?  From which
repository will the package be imported from?

> diff --git a/guix/import/opam.scm b/guix/import/opam.scm
> index a35b01d277..0e6cae72c4 100644
> --- a/guix/import/opam.scm
> +++ b/guix/import/opam.scm
> @@ -2,6 +2,7 @@
>  ;;; Copyright © 2018 Julien Lepiller <julien@lepiller.eu>
>  ;;; Copyright © 2020 Martin Becze <mjbecze@riseup.net>
>  ;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
> +;;; Copyright © 2021 Alice Brenon <alice.brenon@ens-lyon.fr>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -22,21 +23,24 @@
>    #:use-module (ice-9 ftw)
>    #:use-module (ice-9 match)
>    #:use-module (ice-9 peg)
> +  #:use-module ((ice-9 popen) #:select (open-pipe*))
>    #:use-module (ice-9 receive)
> -  #:use-module ((ice-9 rdelim) #:select (read-line))
>    #:use-module (ice-9 textual-ports)
>    #:use-module (ice-9 vlist)
>    #:use-module (srfi srfi-1)
>    #:use-module (srfi srfi-2)
> -  #:use-module (web uri)
> +  #:use-module ((srfi srfi-26) #:select (cut))
> +  #:use-module ((web uri) #:select (string->uri uri->string))
> +  #:use-module ((guix build utils) #:select (dump-port find-files mkdir-p))
>    #:use-module (guix build-system)
>    #:use-module (guix build-system ocaml)
>    #:use-module (guix http-client)
> -  #:use-module (guix git)
>    #:use-module (guix ui)
>    #:use-module (guix packages)
>    #:use-module (guix upstream)
> -  #:use-module (guix utils)
> +  #:use-module ((guix utils) #:select (cache-directory
> +                                       version>?
> +                                       call-with-temporary-output-file))
>    #:use-module (guix import utils)
>    #:use-module ((guix licenses) #:prefix license:)
>    #:export (opam->guix-package
> @@ -121,51 +125,83 @@
>  (define-peg-pattern condition-string all (and QUOTE (* STRCHR) QUOTE))
>  (define-peg-pattern condition-var all (+ (or (range #\a #\z) "-" ":")))
>  
> -(define* (get-opam-repository #:optional repo)
> +(define (opam-cache-directory path)
> +  (string-append (cache-directory) "/opam/" path))
> +
> +(define known-repositories
> +  '((opam . "https://opam.ocaml.org")
> +    (coq . "https://coq.inria.fr/opam/released")
> +    (coq-released . "https://coq.inria.fr/opam/released")
> +    (coq-core-dev . "https://coq.inria.fr/opam/core-dev")
> +    (coq-extra-dev . "https://coq.inria.fr/opam/extra-dev")
> +    (grew . "http://opam.grew.fr")))
> +
> +(define (get-uri repo-root)
> +  (let ((archive-file (string-append repo-root "/index.tar.gz")))
> +    (or (string->uri archive-file)
> +        (begin
> +          (warning (G_ "'~a' is not a valid URI~%") archive-file)
> +          'bad-repo))))
> +
> +(define (repo-type repo)
> +  (match (assoc-ref known-repositories (string->symbol repo))
> +    (#f (if (file-exists? repo)
> +            `(local ,repo)
> +            `(remote ,(get-uri repo))))
> +    (url `(remote ,(get-uri url)))))
> +
> +(define (update-repository input)
> +  "Make sure the cache for opam repository INPUT is up-to-date"
> +  (let* ((output (opam-cache-directory (basename (port-filename input))))
> +         (cached-date (if (file-exists? output)
> +                          (stat:mtime (stat output))
> +                          (begin (mkdir-p output) 0))))
> +    (when (> (stat:mtime (stat input)) cached-date)
> +      (call-with-port
> +       (open-pipe* OPEN_WRITE "tar" "xz" "-C" output "-f" "-")
> +       (cut dump-port input <>)))
> +    output))
> +
> +(define* (get-opam-repository #:optional (repo "opam"))
>    "Update or fetch the latest version of the opam repository and return the
>  path to the repository."
> -  (let ((url (cond
> -               ((or (not repo) (equal? repo 'opam))
> -                "https://github.com/ocaml/opam-repository")
> -               ((string-prefix? "coq-" (symbol->string repo))
> -                "https://github.com/coq/opam-coq-archive")
> -               ((equal? repo 'coq) "https://github.com/coq/opam-coq-archive")
> -               (else (throw 'unknown-repository repo)))))
> -    (receive (location commit _)
> -      (update-cached-checkout url)
> -      (cond
> -        ((or (not repo) (equal? repo 'opam))
> -         location)
> -        ((equal? repo 'coq)
> -         (string-append location "/released"))
> -        ((string-prefix? "coq-" (symbol->string repo))
> -         (string-append location "/" (substring (symbol->string repo) 4)))
> -        (else location)))))
> +  (match (repo-type repo)
> +    (('local p) p)
> +    (('remote 'bad-repo) #f) ; to weed it out during filter-map in opam-fetch
> +    (('remote r) (call-with-port (http-fetch/cached r) update-repository))))
>  
>  ;; Prevent Guile 3 from inlining this procedure so we can mock it in tests.
>  (set! get-opam-repository get-opam-repository)
>  
> -(define (latest-version versions)
> -  "Find the most recent version from a list of versions."
> -  (fold (lambda (a b) (if (version>? a b) a b)) (car versions) versions))
> +(define (get-version-and-file path)
> +  "Analyse a candidate path and return an list containing information for proper
> +  version comparison as well as the source path for metadata."
> +  (and-let* ((metadata-file (string-append path "/opam"))
> +             (filename (basename path))
> +             (version (string-join (cdr (string-split filename #\.)) ".")))
> +    (and (file-exists? metadata-file)
> +         (eq? 'regular (stat:type (stat metadata-file)))
> +         (if (string-prefix? "v" version)
> +             `(V ,(substring version 1) ,metadata-file)
> +             `(digits ,version ,metadata-file)))))

What happens if some other prefix is used, e.g., “release-” or “V-”?

Also, why not just return the version number and the metadata file; we
don’t really care about the prefix do we?

> +(define (keep-max-version a b)
> +  "Version comparison on the lists returned by the previous function taking the
> +  janestreet re-versioning into account (v-prefixed come first)."
> +  (match (cons a b)
> +    ((('V va _) . ('V vb _)) (if (version>? va vb) a b))
> +    ((('V _ _) . _) a)
> +    ((_ . ('V _ _)) b)
> +    ((('digits va _) . ('digits vb _)) (if (version>? va vb) a b))))
>  
>  (define (find-latest-version package repository)
>    "Get the latest version of a package as described in the given repository."
> -  (let* ((dir (string-append repository "/packages/" package))
> -         (versions (scandir dir (lambda (name) (not (string-prefix? "." name))))))
> -    (if versions
> -      (let ((versions (map
> -                        (lambda (dir)
> -                          (string-join (cdr (string-split dir #\.)) "."))
> -                        versions)))
> -        ;; Workaround for janestreet re-versionning
> -        (let ((v-versions (filter (lambda (version) (string-prefix? "v" version)) versions)))
> -          (if (null? v-versions)
> -            (latest-version versions)
> -            (string-append "v" (latest-version (map (lambda (version) (substring version 1)) v-versions))))))
> -      (begin
> -        (format #t (G_ "Package not found in opam repository: ~a~%") package)
> -        #f))))
> +  (let ((packages (string-append repository "/packages"))
> +        (filter (make-regexp (string-append "^" package "\\."))))
> +    (reduce keep-max-version #f
> +            (filter-map
> +             get-version-and-file
> +             (find-files packages filter #:directories? #t)))))
>  
>  (define (get-metadata opam-file)
>    (with-input-from-file opam-file
> @@ -266,28 +302,30 @@ path to the repository."
>  
>  (define (depends->native-inputs depends)
>    (filter (lambda (name) (not (equal? "" name)))
> -    (map dependency->native-input depends)))
> +          (map dependency->native-input depends)))
>  
>  (define (dependency-list->inputs lst)
>    (map
> -    (lambda (dependency)
> -      (list dependency (list 'unquote (string->symbol dependency))))
> -    (ocaml-names->guix-names lst)))
> -
> -(define* (opam-fetch name #:optional (repository (get-opam-repository)))
> -  (and-let* ((repository repository)
> -             (version (find-latest-version name repository))
> -             (file (string-append repository "/packages/" name "/" name "." version "/opam")))
> -    `(("metadata" ,@(get-metadata file))
> -      ("version" . ,(if (string-prefix? "v" version)
> -                        (substring version 1)
> -                        version)))))
> -
> -(define* (opam->guix-package name #:key (repo 'opam) version)
> -  "Import OPAM package NAME from REPOSITORY (a directory name) or, if
> -REPOSITORY is #f, from the official OPAM repository.  Return a 'package' sexp
> +   (lambda (dependency)
> +     (list dependency (list 'unquote (string->symbol dependency))))
> +   (ocaml-names->guix-names lst)))
> +
> +(define* (opam-fetch name #:optional (repositories-specs '("opam")))
> +  (or (fold (lambda (repository others)
> +              (match (find-latest-version name repository)
> +                ((_ version file) `(("metadata" ,@(get-metadata file))
> +                                    ("version" . ,version)))
> +                (_ others)))
> +            #f
> +            (filter-map get-opam-repository repositories-specs))
> +      (leave (G_ "Package '~a' not found~%") name)))

Nit: I wouldn’t capitalize “package”, otherwise the error message looks
like this

  guix import: error: Package 'equations' not found

Otherwise, LGTM!  Great to see more work on the importers!  :-)