GNU bug report logs - #49958
[PATCH] More flexibility in opam importer

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 49958 in the body.
You can then email your comments to 49958 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Alice BRENON <alice.brenon <at> ens-lyon.fr>
To: guix-patches <at> gnu.org
Subject: [PATCH] More flexibility in opam importer
Date: Mon, 9 Aug 2021 14:04:07 +0200

[Message part 1 (text/plain, inline)]

Hello,

I'd like to submit this patch for review, discussion and hopefully
inclusion to guix' code. I recently tried to import grew[0], a NLP tool
written in ocaml and distributed with opam but from a custom repository.

The current importer prevented me to do so for several reasons:

- the available repositories were hard-coded to be either opam's
  official repository or coq's
- the repositories were expected to be distributed with git: while
  public git repositories do exist for coq and opam's official
  repository, they are not the source of truth for the opam tool one
  can use in an imperative setup like this:
    `opam repo add coq-released https://coq.inria.fr/opam/released`
  it entailed that assumptions were made about the freshness of the git
  repositories and the actual files served to opam, hence differences
  could theoretically be observed
- it appears that the opam tool doesn't enforce as strict a structure on
  its repositories as its current documentation[1] suggests. Grew's own
  repository has all versions of each package directly under
  `/packages/` instead of in a separate subdirectory. While this
  deserves a clarification from opam's part, this patch hardens guix
  opam importer against such exotic layouts.
- the unability to query several repositories at once rendered
  recursive imports inefficient, as some packages on a custom
  opam repository may still need dependencies from the official opam
  repository even if no guix package has been imported for it yet (this
  was the case with ocaml-ANSITerminal in my case)

The current proposal attempts to solve these difficulties, and allowed
me to actually import the guix declaration for grew and its
dependencies. I'm still fixing the imported declaration and intend to
submit a separate patch to add it to guix packages when it works. I
added grew's custom opam repository to the list of known-repositories
because it was my immediate target but this is of course not important
and could be reverted in favour of coq and opam's official repository
only.

I used the emacs scripted auto-indenter to save a little time to my
reviewers and had to discard many changes that weren't related to my
changes to cut the noise. Maybe this file should be reindented in a
separate commit following the approval or rejection of this patch
proposal.

Cheers,


Alice

[0]: https://grew.fr/
[1]: https://opam.ocaml.org/doc/Manual.html#Repositories

[0001-guix-opam.patch (text/x-patch, attachment)]

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

From: Alice BRENON <alice.brenon <at> ens-lyon.fr>
To: 49958 <at> debbugs.gnu.org
Subject: [PATCH] More flexibility in opam importer
Date: Mon, 9 Aug 2021 17:19:35 +0200

[Message part 1 (text/plain, inline)]

First update of my patch thanks to the excellent remarks from roptat on
IRC: the signature change for opam->guix-package (namely that the #:key
parameter `repo` now expects a list instead of a single element) broke
the opam test, so here's a version passing a list containing `test-repo`
instead of `test-repo` itself directly to fix the corresponding test.


Alice

Le Mon, 09 Aug 2021 15:04:02 +0000,
help-debbugs <at> gnu.org (GNU bug Tracking System) a écrit :

> Thank you for filing a new bug report with debbugs.gnu.org.
> 
> This is an automatically generated reply to let you know your message
> has been received.
> 
> Your message is being forwarded to the package maintainers and other
> interested parties for their attention; they will reply in due course.
> 
> Your message has been sent to the package maintainer(s):
>  guix-patches <at> gnu.org
> 
> If you wish to submit further information on this problem, please
> send it to 49958 <at> debbugs.gnu.org.
> 
> Please do not send mail to help-debbugs <at> gnu.org unless you wish
> to report a problem with the Bug-tracking system.
> 

[0001-guix-opam.patch (text/x-patch, attachment)]

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

From: Alice BRENON <alice.brenon <at> ens-lyon.fr>
To: 49958 <at> debbugs.gnu.org
Subject: [PATCH] More flexibility in opam importer
Date: Tue, 10 Aug 2021 14:04:13 +0200

[Message part 1 (text/plain, inline)]

Following further discussion on IRC, here is a version which

- removes an unneeded mocked value in the test
- updates the documentation to explain the new behaviour of the --repo option for
  this importer
- improves the general quality of code and commit message

[0001-guix-opam-More-flexibility-in-the-importer.patch (text/x-patch, attachment)]

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

From: Alice BRENON <alice.brenon <at> ens-lyon.fr>
To: 49958 <at> debbugs.gnu.org
Subject: [PATCH] More flexibility in opam importer
Date: Tue, 10 Aug 2021 18:48:26 +0200

[Message part 1 (text/plain, inline)]

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

[0001-guix-opam-More-flexibility-in-the-importer.patch (text/x-patch, attachment)]

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

From: Xinglu Chen <public <at> yoctocell.xyz>
To: Alice BRENON <alice.brenon <at> ens-lyon.fr>, 49958 <at> debbugs.gnu.org
Subject: Re: [bug#49958] [PATCH] More flexibility in opam importer
Date: Fri, 13 Aug 2021 09:37:43 +0200

[Message part 1 (text/plain, inline)]

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 <at> 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 <at> lepiller.eu>
>  ;;; Copyright © 2020 Martin Becze <mjbecze <at> riseup.net>
>  ;;; Copyright © 2021 Xinglu Chen <public <at> yoctocell.xyz>
> +;;; Copyright © 2021 Alice Brenon <alice.brenon <at> 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!  :-)

[signature.asc (application/pgp-signature, inline)]

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

From: Alice BRENON <alice.brenon <at> ens-lyon.fr>
To: Xinglu Chen <public <at> yoctocell.xyz>
Cc: 49958 <at> debbugs.gnu.org
Subject: Re: [bug#49958] [PATCH] More flexibility in opam importer
Date: Fri, 13 Aug 2021 13:11:09 +0200

[Message part 1 (text/plain, inline)]

Le Fri, 13 Aug 2021 09:37:43 +0200,
Xinglu Chen <public <at> yoctocell.xyz> a écrit :

> On Tue, Aug 10 2021, Alice BRENON wrote:
> 
> […]
> > 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.
> 
> ?

Ok, as discussed on IRC, trying "lets you add other repositories which
will be searched for packages".


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

That is the beauty of it: the repositories are assumed to be passed by
order of preference, defaulting to the official opam repositories only
if packages haven't been found anywhere else. Writing this makes me
realize that indeed, starting with --repo=opam isn't entirely
redundant: it could be used to prevent an otherwise interesting repo
from overriding stuff if opam already provides it (let's assume some
"super-opam" with a couple additional packages, and custom versions of
existing opam packages).

Calling `--repo=super-opam` would use the super-opam versions as soon
as a package exists in super-opam, while `--repo=opam
--repo=super-opam` would take the super-opam versions only when none
exist in opam.

A much simpler use-case would be to locally override only some
packages in a repo, and pass --repo=overriden-repo --repo=normal-repo.

This behaviour relies on the implementation of opam-fetch and how folds
work in guile.

Since in the importer script options are stacked as they are retrieved
from the CLI arguments, and repositories are then just filter-maped from
that list, they end up in a list by reverse order of preference. In
opam->guix-package, 'opam gets push on the top if it's not already
there somewhere. So what we get as input of opam-fetch is a list of
repositories-specs by reverse order of preference. Now fold applies the
accumulator to each item in order, so, last elements has the final say,
i.e. the last elements which yield results in find-latests are
preferred over the earlier elements of the list. This works for the
same reason why `(lambda (l) (fold cons '() l)` will reverse its input
list. It's slightly inefficient because it means all repositories are
searched, in reverse order of preference, but I haven't figured how to
get a lazy fold in guile. Granted, I could have written the recursion
explicitly to get that. Will fix if performance matters.

Also, versions are not compared between repositories, as soon as a repo
provides one version of a given package, the latest of all the versions
this one provides is used in the output, no matter the contents of
other repositories. This is useful to allow "downgrades" by masking
parts of repository which have too recent versions.

So, thanks for your remark, the documentation deserved a clearer
explanation of it.

> […]
> > -(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-”?
> 

It would get marked as a 'digit. In a previous draft before I started
sending this series of patches, it was called 'R, standing for
"regular", then I thought it was not very meaningful, and, since the
versions were to my knowledge supposed to be either v[0-9]+(\.[0-9]+)*
or [0-9]+(\.[0-9]+)*, I thought I could call that default case "digits"
to clearly indicate what I was trying to refer to. I could change it to
'other if it matters too much, but the important thing here is that we
distinguish between v-prefixed (the so-called "janestreet
re-versionning" mentioned inside the implementation of find-latest on
current d87d6d6 master) and other versions because ⬇️

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

yes we do ! the former latest-version finder handled strings, and
dropped this prefix or put it back on the fly, but the logic
implemented was: if there are v-prefixed versions, find the greatest of
them, without the initial "v", if there aren't, just find the greatest
of all versions. This implies that v-prefixed versions are considered
more important and automatically greater than non-prefixed versions, no
matter what the numbers, which is why this information must be kept.

I'm just playing ADTs in guile here, "parsing" the version string only
once to retain a symbolic representation of it that will at first
glance allow to identify the type of version used and access the
relevant digits for comparison. The comparison is used right after:

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

and used in the reduce in find-latest-version. So keeping this 'V is
what will help janestreet-re-versionned packages "skip the line" by
being automatically greater than any non v-prefixed package (thus,
v0.0.1 is greater than 13.2, which is the current behaviour).

> >  (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)))))
> >  
> […]
> > +            (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

a very neat tip, thank you ! : )

[0001-guix-opam-More-flexibility-in-the-importer.patch (text/x-patch, attachment)]

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

From: Xinglu Chen <public <at> yoctocell.xyz>
To: Alice BRENON <alice.brenon <at> ens-lyon.fr>
Cc: 49958 <at> debbugs.gnu.org
Subject: Re: [bug#49958] [PATCH] More flexibility in opam importer
Date: Fri, 13 Aug 2021 15:13:14 +0200

[Message part 1 (text/plain, inline)]

On Fri, Aug 13 2021, Alice BRENON wrote:

> Le Fri, 13 Aug 2021 09:37:43 +0200,
> Xinglu Chen <public <at> yoctocell.xyz> a écrit :
>
>> On Tue, Aug 10 2021, Alice BRENON wrote:
>> 
>> […]
>> > 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.
>> 
>> ?
>
> Ok, as discussed on IRC, trying "lets you add other repositories which
> will be searched for packages".
>
>
>> 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?
>
> That is the beauty of it: the repositories are assumed to be passed by
> order of preference, defaulting to the official opam repositories only
> if packages haven't been found anywhere else. Writing this makes me
> realize that indeed, starting with --repo=opam isn't entirely
> redundant: it could be used to prevent an otherwise interesting repo
> from overriding stuff if opam already provides it (let's assume some
> "super-opam" with a couple additional packages, and custom versions of
> existing opam packages).
>
> Calling `--repo=super-opam` would use the super-opam versions as soon
> as a package exists in super-opam, while `--repo=opam
> --repo=super-opam` would take the super-opam versions only when none
> exist in opam.
>
> A much simpler use-case would be to locally override only some
> packages in a repo, and pass --repo=overriden-repo --repo=normal-repo.
>
> This behaviour relies on the implementation of opam-fetch and how folds
> work in guile.
>
> Since in the importer script options are stacked as they are retrieved
> from the CLI arguments, and repositories are then just filter-maped from
> that list, they end up in a list by reverse order of preference. In
> opam->guix-package, 'opam gets push on the top if it's not already
> there somewhere. So what we get as input of opam-fetch is a list of
> repositories-specs by reverse order of preference. Now fold applies the
> accumulator to each item in order, so, last elements has the final say,
> i.e. the last elements which yield results in find-latests are
> preferred over the earlier elements of the list. This works for the
> same reason why `(lambda (l) (fold cons '() l)` will reverse its input
> list. It's slightly inefficient because it means all repositories are
> searched, in reverse order of preference, but I haven't figured how to
> get a lazy fold in guile. Granted, I could have written the recursion
> explicitly to get that. Will fix if performance matters.
>
> Also, versions are not compared between repositories, as soon as a repo
> provides one version of a given package, the latest of all the versions
> this one provides is used in the output, no matter the contents of
> other repositories. This is useful to allow "downgrades" by masking
> parts of repository which have too recent versions.
>
> So, thanks for your remark, the documentation deserved a clearer
> explanation of it.

Thanks for the explanation!  And great that you also documented this to
avoid ambiguity.

>> > -(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-”?
>> 
>
> It would get marked as a 'digit. In a previous draft before I started
> sending this series of patches, it was called 'R, standing for
> "regular", then I thought it was not very meaningful, and, since the
> versions were to my knowledge supposed to be either v[0-9]+(\.[0-9]+)*
> or [0-9]+(\.[0-9]+)*, I thought I could call that default case "digits"
> to clearly indicate what I was trying to refer to. I could change it to
> 'other if it matters too much, but the important thing here is that we
> distinguish between v-prefixed (the so-called "janestreet
> re-versionning" mentioned inside the implementation of find-latest on
> current d87d6d6 master) and other versions because ⬇️

Oh, OK, I wasn’t aware of this “janestreet re-versionning” thing, so
only janestreet package have the “v” prefix, right?  That explains why
versions prefixed with “v” are always greater than those not prefixed
with anything (in ‘keep-max-version’).

>> Also, why not just return the version number and the metadata file; we
>> don’t really care about the prefix do we?
>> 
>
> yes we do ! the former latest-version finder handled strings, and
> dropped this prefix or put it back on the fly, but the logic
> implemented was: if there are v-prefixed versions, find the greatest of
> them, without the initial "v", if there aren't, just find the greatest
> of all versions. This implies that v-prefixed versions are considered
> more important and automatically greater than non-prefixed versions, no
> matter what the numbers, which is why this information must be kept.

Ah, understood.

> I'm just playing ADTs in guile here, "parsing" the version string only
> once to retain a symbolic representation of it that will at first
> glance allow to identify the type of version used and access the
> relevant digits for comparison. The comparison is used right after:
>
>> > +(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)))) 
>
> and used in the reduce in find-latest-version. So keeping this 'V is
> what will help janestreet-re-versionned packages "skip the line" by
> being automatically greater than any non v-prefixed package (thus,
> v0.0.1 is greater than 13.2, which is the current behaviour).
>
>> >  (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)))))
>> >  
>> […]
>> > +            (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
>
> a very neat tip, thank you ! : )

You are welcome, and thank you for working on this!

> From cde8b2a5d88d89bfea31c86d3ae94d37c1d3c83f Mon Sep 17 00:00:00 2001
> From: Alice BRENON <alice.brenon <at> 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.

Nit: The word after the “:” is usually capitalized, so “Pass” instead of
“pass” in this case.  Sorry for not noticing this earlier; the person
committing the patch can probably fixup the commit message, so no need
to send a reroll just for this small fix.  :-)

> * 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                |  30 +++++--
>  guix/import/opam.scm         | 158 +++++++++++++++++++++--------------
>  guix/scripts/import/opam.scm |   8 +-
>  tests/opam.scm               |  68 ++++++++-------
>  4 files changed, 160 insertions(+), 104 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 78c1c09858..2d36561186 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,31 @@ 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 which will be searched for packages.  It accepts as valid
> +arguments:
> +
>  @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
> +
> +Repositories must be passed to this option by order of preference and do
> +not replace the default @code{opam} which is always failed-back to.

I suggest

  Repositories should be passed to this option by the order of
  preference.  The additional repositories will not replace the default
  @code{opam} repository, which is always kept as a fallback.

WDYT?

> +Also, please note that versions are not compared accross repositories.
> +The first repository (from left to right) that has at least one version
> +of a given package will prevail over any others and the version imported
                                                  ^
Missing comma.

The rest looks good!  :-)

[signature.asc (application/pgp-signature, inline)]

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

From: Alice BRENON <alice.brenon <at> ens-lyon.fr>
To: Xinglu Chen <public <at> yoctocell.xyz>
Cc: 49958 <at> debbugs.gnu.org
Subject: [bug#49958] [PATCH] More flexibility in opam importer
Date: Fri, 13 Aug 2021 15:47:17 +0200

[Message part 1 (text/plain, inline)]

Re-phrased the new documentation explaining how versions are selected
from the repositories in case several contain the same package.

Sincerely hope it's the last one : )

[0001-guix-opam-More-flexibility-in-the-importer.patch (text/x-patch, attachment)]

Message #31 received at 49958-done <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: Alice BRENON <alice.brenon <at> ens-lyon.fr>
Cc: 49958-done <at> debbugs.gnu.org, Xinglu Chen <public <at> yoctocell.xyz>
Subject: Re: [bug#49958] [PATCH] More flexibility in opam importer
Date: Sat, 21 Aug 2021 00:07:48 +0200

Le Fri, 13 Aug 2021 15:47:17 +0200,
Alice BRENON <alice.brenon <at> ens-lyon.fr> a écrit :

> Re-phrased the new documentation explaining how versions are selected
> from the repositories in case several contain the same package.
> 
> Sincerely hope it's the last one : )

Pushed to master as fc29c80b9635ff490bcc768c774442043cb1e231, thanks!

Previous Next

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