On Fri, Sep 10 2021, Ludovic Courtès wrote: > Hello, > > This looks very cool! Thanks for taking a look! It’s still a WIP, but I think it’s getting there. :-) > Xinglu Chen skribis: > >> From f924dbb835425f6b9a5796918125592870391405 Mon Sep 17 00:00:00 2001 >> Message-Id: >> From: Xinglu Chen >> Date: Fri, 3 Sep 2021 17:50:56 +0200 >> Subject: [PATCH] import: Add 'generic-git' updater. >> >> * guix/import/git.scm: New file. >> * doc/guix.texi (Invoking guix refresh): Document it. >> * Makefile.am (MODULES): Register it. >> * guix/git.scm (ls-remote-refs): New procedure. >> >> Co-authored-by: Sarah Morgensen > > Overall LGTM; comments below. > >> diff --git a/doc/guix.texi b/doc/guix.texi >> index 36a0c7f5ec..26afb1607a 100644 >> --- a/doc/guix.texi >> +++ b/doc/guix.texi >> @@ -11920,6 +11920,33 @@ the updater for @uref{https://launchpad.net, Launchpad} packages. >> @item generic-html >> a generic updater that crawls the HTML page where the source tarball of >> the package is hosted, when applicable. >> +@item generic-git > > Please add a newline above. Noted. >> +@lisp >> +(package >> + (name "foo") >> + ;; ... >> + (properties >> + '((tag-prefix . "^release0-") >> + (tag-suffix . "[a-z]?$") >> + (tag-version-delimiter . ":")))) >> +@end lisp > > Very nice. Perhaps s/tag-/release-tag-/ for clarity? Good idea. >> +(define* (ls-remote-refs url #:key tags?) >> + "Return the list of references advertised at Git repository URL. If TAGS? >> +is true, limit to only refs/tags." > > To remain consistent with existing naming conventions, I’d call it > ‘remote-refs’. > >> + (with-libgit2 >> + (call-with-temporary-directory >> + (lambda (cache-directory) >> + (let* ((repository (repository-init cache-directory)) >> + ;; Create an in-memory remote so we don't touch disk. >> + (remote (remote-create-anonymous repository url))) > > Too bad we need to create an empty repo; hopefully it costs next to > nothing though. > >> + (remote-connect remote) >> + (remote-disconnect remote) >> + (repository-close! repository) >> + >> + (filter include? (map remote-head-name (remote-ls remote)))))))) > > Use ‘filter-map’. > >> +(define* (get-version-mapping tags #:key prefix suffix delim pre-releases?) > > Please add a docstring and remove ‘get-’ from the name. :-) > >> + (define (guess-delim) >> + (let ((total (length tags)) >> + (dots (reduce + 0 (map (cut string-count <> #\.) tags))) >> + (dashes (reduce + 0 (map (cut string-count <> #\-) tags))) >> + (underscores (reduce + 0 (map (cut string-count <> #\_) tags)))) >> + (display (format #t "total: ~d, dots: ~d, dashes ~d, underscores ~d~%" >> + total dots dashes underscores)) > > Leftover? (Also display + format.) Yep. :-) > Please spell out ‘delimiter’ (info "(guix) Formatting Code"). > >> + (cond >> + ((>= dots (* total 0.35)) ".") >> + ((>= dashes (* total 0.8)) "-") >> + ((>= underscores (* total 0.8)) "_") >> + (else "")))) > > That’s a fancy heuristic. :-) Yeah, it was suggested by Sarah, and in my testing it seems to work pretty well. :-) >> + (let ((mapping (fold alist-cons '() (map get-version tags) tags))) >> + (stable-sort! (filter car mapping) entry > It’s perhaps clearer written like this: > > (stable-sort (filter-map (lambda (tag) > (let ((version (get-version tag))) > (and version (cons version tag)))) > tags) > entry> +(define* (get-latest-tag url #:key prefix suffix delim pre-releases?) >> + "Return the latest tag available from the Git repository at URL." > > Maybe “the tag corresponding to the latest version”. Yeah, as the latest tag might not correspond to a release... > s/get-latest-tag/latest-tag/ > >> + (define (pre-release? tag) >> + (any (lambda (rx) (regexp-exec (make-regexp rx regexp/icase) tag)) >> + %pre-release-words)) > > Better call ‘make-regexp’ only once; so you could change > ‘%pre-release-words’ to be a list of regexp objects instead of a list of > strings. Noted. >> +(define (latest-git-tag-version package tag-prefix tag-suffix >> + tag-version-delimiter refresh-pre-releases?) >> + "Given a PACKAGE, the TAG-PREFIX, TAG-SUFFIX, TAG-VERSION-DELIMITER, and >> +REFRESH-PRE-RELEASES? properties of PACKAGE, returns the latest version of >> +PACKAGE." > > Maybe s/refresh-pre-releases?/accept-pre-preleases?/ ‘accept-pre-releases?’ ;-) > Since this procedure takes a package, it probably doesn’t need the other > arguments: it can extract them from the package properties, rather than > doing it at the call site. Good point. >> +(define (latest-git-release package) >> + "Return the latest release of PACKAGE." >> + (let* ((name (package-name package)) >> + (properties (package-properties package)) >> + (tag-prefix (assq-ref properties 'tag-prefix)) >> + (tag-suffix (assq-ref properties 'tag-suffix)) >> + (tag-version-delimiter (assq-ref properties 'tag-version-delimiter)) >> + (refresh-pre-releases? (assq-ref properties 'refresh-pre-releases?)) >> + (old-version (package-version package)) >> + (url (git-reference-url (origin-uri (package-source package)))) >> + (new-version (latest-git-tag-version package >> + tag-prefix >> + tag-suffix >> + tag-version-delimiter >> + refresh-pre-releases?))) >> + >> + (if new-version >> + (upstream-source >> + (package name) >> + (version new-version) >> + (urls (list url))) >> + ;; No new release or no tags available. >> + #f))) > > Simply: (and new-version (upstream-source …)). > > It would have been nice to have tests. I think testing > ‘latest-git-release’ should be feasible without too much hassle using > the (guix tests git) infrastructure, as is done in tests/git.scm, with a > package referring to a locally-created repo using a git-reference with a > file:// URL. Thanks for the pointers! I will look into it. > Could you send an updated patch? Sure! Thanks for the review! :-)