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

[Alice BRENON]

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.

> […]
> > -(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
Description: Text Data