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