On Sat, Sep 04 2021, Sarah Morgensen wrote:

> Hello,
>
> Thanks for the patch!  Glad to see this idea becoming more polished.
>
> Xinglu Chen <public@yoctocell.xyz> writes:
>
>> * guix/import/git.scm: New file.
>> * doc/guix.texi (Invoking guix refresh): Document it.
>> * Makefile.am (MODULES): Register it.
>> ---
>> This patch adds a new ‘generic-git’ updater which can check for new tags
>> for package hosted on Git repos.  However, it cannot download Git repos
>> and update the package definitions, i.e. ‘guix refresh -u’.  There is a
>> pending patch that would add this feature though[1].
>>
>> ‘guix refresh -L’ now reports
>>
>>   Available updaters:
>>   […]
>>   94.5% of the packages are covered by these updaters.
>>
>> We are getting close to 100% :-)
>
> Wow, that is close!
>
>>
>> See it in action!
>>
>> $ ./pre-inst-env guix refresh harmonist scdoc gmnisrv
>> gnu/packages/web.scm:7931:4: warning: no tags were found for package `gmnisrv'
>> gnu/packages/web.scm:7931:4: warning: 'generic-git' updater failed to determine available releases for gmnisrv
>> gnu/packages/man.scm:339:12: scdoc would be upgraded from 1.10.1 to 1.11.1
>> gnu/packages/games.scm:9433:2: warning: failed to fetch Git repository for package `harmonist'
>> gnu/packages/games.scm:9433:2: warning: 'generic-git' updater failed to determine available releases for harmonist
>
> FWIW, harmonist and a few other packages fail to work because they use
> an old git protocol which is not supported by libgit2.
>
> [...]
>> +
>> +@itemize
>> +@item @code{tag-prefix}: a regular expression for matching a prefix of
>> +the tag name.
>> +
>> +@item @code{tag-suffix}: a regular expression for matching a suffix of
>> +the tag name.
>> +
>> +@item @code{tag-version-delimiter}: a string used as the delimiter in
>> +the tag name for separating the numbers of the version.
>> +@end itemize
>> +
>> +@lisp
>> +(package
>> +  (name "foo")
>> +  ;; ...
>> +  (properties
>> +    '((tag-prefix . "^release0-")
>> +      (tag-suffix . "[a-z]?$")
>> +      (tag-version-delimiter . ":"))))
>> +@end lisp
>              ^ extra whitespace
>
> I do like the selection of (prefix, suffix, delimiter), though I think
> there are only one or two packages which use a different delimiter.
>
> [...]
>> +;;; Errors & warnings
>> +
>> +(define-condition-type &git-tag-error &error
>> +  git-tag-error?
>> +  (kind git-tag-error-kind))
>> +
>> +(define (git-tag-error kind)
>> +  (raise (condition (&message (message (format "bad `~a' property")))
>> +                    (&git-tag-error
>> +                     (kind kind)))))
>
> When I trigger this error, I get:
> --8<---------------cut here---------------start------------->8---
> In ice-9/exceptions.scm:
>    406:15  6 (latest-git-release _)
> In ice-9/boot-9.scm:
>   1752:10  5 (with-exception-handler _ _ #:unwind? _ # _)
> In guix/import/git.scm:
>     59:39  4 (get-version _ _ #:prefix _ #:suffix _ #:delim _)
> In unknown file:
>            3 (simple-format #f "bad `~a' property")
> In ice-9/boot-9.scm:
>   1685:16  2 (raise-exception _ #:continuable? _)
>   1683:16  1 (raise-exception _ #:continuable? _)
>   1685:16  0 (raise-exception _ #:continuable? _)
>
> ice-9/boot-9.scm:1685:16: In procedure raise-exception:
> In procedure simple-format: FORMAT: Missing argument for ~a
> --8<---------------cut here---------------end--------------->8---

Oops, it should be

  (format "bad `~a' property" kind)
  
>> +
>> +(define (git-tag-warning package c)
>> +  (warning (package-location package)
>> +           (G_ "~a for package `~a'~%")
>> +           (condition-message c)
>> +           (package-name package)))
>> +
>> +(define-condition-type &git-no-tags-error &error
>> +  git-no-tags-error?)
>> +
>> +(define (git-no-tags-error)
>> +  (raise (condition (&message (message "no tags were found"))
>> +                    (&git-no-tags-error))))
>> +
>> +(define (git-no-tags-warning package c)
>> +  (warning (package-location package)
>> +           (G_ "~a for package `~a'~%")
>> +           (condition-message c)
>> +           (package-name package)))
>> +
>> +(define (git-fetch-warning package)
>> +  (warning (package-location package)
>> +           (G_ "failed to fetch Git repository for package `~a'~%")
>> +           (package-name package)))
>> +
>> +
>> +;;; Helper functions
>> +
>> +(define (string-split* str delim)
>> +  "Like `string-split', but DELIM is a string instead of a
>> +char-set."
>> +  (filter (lambda (str) (not (equal? str "")))
>> +          (string-split str (string->char-set delim))))
>
>   (string-split* "1:2.3" ":.") -> ("1" "2" "3")
>   (string-split* "1a2b3" "ab") -> ("1" "2" "3")
>
> Is this what you intended?  The documentation above makes it sound like
> the whole string serves as the delimiter.

It’s not what I wanted, indeed.  I will try to fix it.

>> +
>> +(define* (get-version package tag #:key prefix suffix delim)
>
> PACKAGE is not used by this procedure.

Good catch, it was some leftover I forgot to remove.

>> +  (define delim* (if delim delim "."))
>> +  (define prefix-regexp "^[^0-9]*")
>> +  (define suffix-regexp (string-append "[^0-9" (regexp-quote delim*) "]*$"))
>
> With a delimiter of '.', this would say the suffix of '1.2.3.prerelease'
> is 'prerelease', not '.prerelease'.  Is this correct?  (I would be
> tempted to just remove delim* from this.)

Good point, I think removing ‘delim*’ would be a good idea.

>> +  (define delim-regexp (string-append "^[0-9]+" (regexp-quote delim*) "[0-9]+"))
>
> This fails to account for versions which use non-numerics, such as (all
> taken from the package-version field of packages using git-fetch and
> which use this version as the tag):
>
> 1.0.0-beta.0
> 0.0.9.4f
> 4.4-git.1
> 5.2.0-alpha
> 0.2.0-alpha-199-g3e7a475
> 20200701.154658.b0d6223
> 12-068oasis4
> 4.0.0.dev8
> 0.32-14-gcdfe14e
> 2.8-fix-2
>
> There are about 50-60 packages like this.
>
> I'm not sure how much effort should be spent including them, and for
> some of them I'm not sure what our ideal behavior *is*.  Even if we
> could reliably detect them, should "alpha" or "dev" packages be returned
> by the updater?

I don’t think we usually include alpha or rc releases, so updater
probably shouldn’t return them either.  Not sure how we would try to
detect alpha/beta/rc releases, though, besides running something like

  (string-match? "(alpha|beta|rc|dev)" TAG)

On each tag.

> Upon investigation, there is a deeper problem: version-compare thinks
> "5.2.0" is a lower version than "5.2.0-alpha", and that "4.0.0" is lower
> than "4.0.0.dev8".
>
> scheme@(guile-user)> (version-compare "5.1.9" "5.2.0")
> $5 = <
> scheme@(guile-user)> (version-compare "5.2.0" "5.2.0-alpha")
> $6 = <
> scheme@(guile-user)> (version-compare "4.0.0" "4.0.0.dev8")
> $7 = <

Maybe we should filter the tags before comparing them; that should
get rid of these pre-release tags.

>> +
>> +  (define no-prefix
>> +    (let ((match (string-match (or prefix prefix-regexp) tag)))
>> +      (if match
>> +          (regexp-substitute #f match 'post)
>> +          (git-tag-error 'tag-prefix))))
>> +
>> +  (define no-suffix
>> +    (let ((match (string-match (or suffix suffix-regexp) no-prefix)))
>> +      (if match
>> +          (regexp-substitute #f match 'pre)
>> +          (git-tag-error 'tag-suffix))))
>> +
>> +  (define no-delims
>> +    (if (string-match delim-regexp no-suffix)
>> +        (string-split* no-suffix delim*)
>> +        (git-tag-error 'tag-version-delimiter)))
>
> This throws an error if the version doesn't have any delimiter.

Setting the ‘tag-version-delimiter’ prefix to an empty string would
solve this, right?  Or, maybe we should just get rid of the delimiter
thing since only a few packages use a different delimiter.

> Actually, it throws an error in a lot of other cases too, often saying
> the 'tag-version-delimiter is wrong when it's something else.  Consider
> the tags from the "openjpeg" package, sorted by 'sort-tags':
>
> arelease
> opj0-97
> start
> v2.1.1
> v2.1.2
> v2.2.0
> v2.3.0
> v2.3.1
> v2.4.0
> version.1.0
> version.1.1
> version.1.2
> version.1.3
> version.1.4
> version.1.5
> version.1.5.1
> version.1.5.2
> version.2.0
> version.2.0.1
> version.2.1
> wg1n6848
>
> At first, 'get-version' throws an error because "wg1n6848" doesn't have
> a delimiter. But even disregarding that, it would return "version.2.1"
> -> "2.1" as the latest version.
>
> Probably we should process all tags with 'get-version' (simply skipping
> any that don't parse) and use that to sort the tags.  If none parse with
> 'get-version' we could use the "no tags" error or have a separate error
> for "there were tags but we couldn't process them".

Ah, yes, that would be a good idea.

> And this lets us just do something like (untested):
>
> (define* (get-version tag #:key prefix suffix delim)
>   (define delim-rx (regexp-quote (or delim ".")))
>   (define prefix-rx (or prefix "[^[:digit:]]*"))
>   (define suffix-rx (or suffix ".*"))
>   (define version-char-rx
>    (string-append "[^" delim-rx "[:punct:]]"))
>
>   (define tag-rx
>    (string-append "^" prefix "(" version-char-rx "+("
>                   delim-rx version-char-rx ")*)" suffix-rx "$"))
>
>   (and=> (string-match tag-rx tag)
>          (cut match-substring <> 1)))
>
> Though at this point, 'tag-rx' should probably be constructed and
> compiled outside the loop.
>
>> +
>> +  (string-join no-delims "."))
>> +
>> +(define (sort-tags tags)
>> +  "Sort TAGS, a list if Git tags, such that the latest tag is the last element."
>> +  (sort tags (lambda (a b)
>> +               (eq? (version-compare a b) '<))))
>> +
>> +
>> +;;; Updater
>> +
>> +(define (get-remote url git-uri)
>> +  "Given a URL and GIT-URI, a <git-reference> record, return the ``origin'' remote."
>> +  (let* ((checkout (update-cached-checkout url
>> +                                           #:recursive?
>> +                                           (git-reference-recursive? git-uri)))
>> +         (repository (repository-open checkout)))
>> +    (remote-lookup repository "origin")))
>
> We surely don't want 'update-cached-checkout' since that fetches the
> whole repo history!  I've attached a patch below (based on top of this
> one) which brings the total time-per-package to under 1s.  I moved it to
> (guix git) to make use of 'with-libgit2' which ensures we use system
> certificates.
>
> Apologies for such a long reply. I hope it was helpful :)

No worries, it definitely helped a lot, thank you!