On Tue, Aug 10 2021, Alice BRENON wrote:
Toggle quote (58 lines)
> - 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.
?
Toggle quote (20 lines)
> @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?
Toggle quote (126 lines)
> 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?
Toggle quote (74 lines)
> +(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! :-)
