[bug#49958] [PATCH] More flexibility in opam importer

[Xinglu Chen <public@yoctocell.xyz>]

[-- Attachment #1: Type: text/plain, Size: 13209 bytes --]

On Fri, Aug 13 2021, Alice BRENON wrote:

> Le Fri, 13 Aug 2021 09:37:43 +0200,
> Xinglu Chen <public@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@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!  :-)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]