[bug#50359] [PATCH 3/3] import: Add 'generic-git' updater.

[Xinglu Chen]

On Thu, Sep 16 2021, Sarah Morgensen wrote:

> Xinglu Chen <public@yoctocell.xyz> writes:
>
>>>> Maybe we could have a ‘release-tag-date-scheme?’ property, that way we
>>>> could just try to match dates?
>>>
>>> That seems like it might be the only way to handle it in some cases (if
>>> they have both versions and dates with a "." delimiter).
>>
>> It doesn’t have to be “.” delimiter though; if they both have the same
>> delimiter it would be difficult to distinguish a version from a date,
>> e.g., “1-2-3” vs “2021-03-23”.
>
> Sure, but I haven't seen the former :)
>
>>> (Though, we are actually interested in the *lack* of a date scheme.
>>> If they use a date scheme now, other versions will be disregarded, so
>>> we're fine; but if they use versions now and used a date scheme
>>> before, the versions will be discarded.)
>>
>> I am not sure what you are trying to say, could you elaborate?
>
> Just that the important case is disallowing dates when
> 'release-tag-date-scheme? is #f.
>
> If the tags of a repo are:
>
> 12.1
> 12.2
> 13.0
> 13.4
> 2018.01.01
> 2018.05.05
>
> and we do nothing, the 2018.05.05 tag will be selected.  This is correct
> if we do want dates, but incorrect if we don't (in which case we would
> set 'tag-version-date-scheme? to #f to get the correct result).

Ah, that makes sense.  :-)

>>> Though it would be nice to see when such updates are available, is it
>>> worth some bogus results?  Are false positives better or false negatives
>>> better?
>>
>> Hmm, good question!  If in the future we have some kind of bot that
>> automatically runs ‘guix refresh -u’, builds the updated package, and
>> send a patch to the mailing list, not having false positives might be
>> more important.  We could also have a ‘disable-tag-updater?’ property to
>> disable the updater for packages which gives false positive, or maybe
>> that will result in to many properties.
>
> For these packages, it would probably easier to just use the existing
> tag- properties.  In fact, instead of this or the date-scheme above,
> a 'tag-version-regex' would cover both cases.
>
> In fact, we could replace 'tag-version-delimiter' with
> 'tag-version-regex' and instead provide convencience functions such as
> (untested):
>
> (define (version-regex delim)
>   (let ((delim-rx (regexp-quote delim)))
>     (string-append "([[:digit:]][^" delim-rx "[:punct:]]*"
>                    "(" delim-rx "[^[:punct:]" delim-rx "]+)"
>                    (if (string=? delim-rx "") "*" "+"))))
>
> (define* (version-date-regex (delim "."))
>   (let ((delim-rx (regexp-quote delim)))
>     (string-append "([0-9]{4}" delim-rx "(0[1-9]|11|12)"
>                    delim-rx "(0[1-9]|[1-2][0-9])")))
>
> WDYT?

That sounds like a good idea!  Where would we put these procedures,
(guix packages)?

>>> Unless you/we want to pursue one or both of the above changes now, the
>>> latest patch LGTM (modulo my nits).
>>
>> I would prefer to wait a bit with the improvements mentioned above.  The
>> current patch has been in the works a week or two already, so it’s
>> probably a good idea to get it merged, and try to solve the less
>> important issues later.  :-)
>
> Sounds good to me, then!
>
>>> I discovered that this can segfault unless 'remote-disconnect' and
>>> possibly 'repository-close!' are called *after* copying the data out.
>>> I've attached a diff for this.
>>
>> I don’t see a diff attached; maybe you forgot?  :-)
>>
>
> I've actually attached it this time :)
>
>>>> +
>>>> +(define (git-package? package)
>>>> +  "Whether the origin of PACKAGE is a Git repostiory."
>>>
>>> "Return true if PACKAGE is..."
>>
>> “PACKAGE is a Git repository.” doesn’t really sound right, maybe “if
>> PACKAGE is hosted on a Git repository”?'
>
> Sorry, yes, that's what I meant, or "Return true if the origin..."; I
> was just suggesting making it a full sentence.
>
>>> I tested this updater on all packages in .scm files starting with f
>>> through z, and I found the following packages with possibly bogus
>>> updates:
>>>
>>> --8<---------------cut here---------------start------------->8---
>>> javaxom
>>
>> I assume you meant ‘java-xom’  :-)
>>
>> That’s a weird scheme; setting the delimiter to “.” doesn’t help since
>> it thinks that “127” is greater than “1.3.7”.
>
> 'tag-version-regex would allow fixing this ;)
>
>>
>>> luakit
>>> ocproxy
>>> pitivi
>>
>> ‘pitivi’ has a pretty weird version string to begin with; it may be
>> better to change it to the date: “0.999.0-2021-05.0” -> “2021-05.0”.
>>
>>> eid-mw
>>> libhomfly
>>> gnuradio
>>> welle-io
>>
>> Setting the delimiter to "." fixes the issue.
>>
>>> racket-minimal
>>
>> Setting the prefix to "v" fixes this.
>>
>>> milkytracker
>>> cl-portal
>>> kodi-cli
>>> openjdk
>>> java-bouncycastle
>>> hurd
>>> opencsg
>>
>> Setting the suffix to "-release" fixes this.
>>
>>> povray
>>> gpsbabel
>>
>> Setting the prefix to "gpsbabel_" fixes this.
>>
>>> go
>>> stepmania
>>> ocaml-mcl
>>>
>>> many minetest packages (minetest will have its own updater, though)
>>>
>>> ocaml4.07-core-kernel, ocamlbuild and many other ocaml packages
>>> (they seem to be covered by the github updater)
>>> --8<---------------cut here---------------end--------------->8---
>
> I'm glad to see that these are easily fixed with the properties, though!
> That's some good validation.

Yeah, it’s looking pretty good.  :-)

> Now I just have to give the (guix upstream) some attention...
>
> --
> Sarah
>
> diff --git a/guix/git.scm b/guix/git.scm
> index dc3d3afd02..bbff4fc890 100644
> --- a/guix/git.scm
> +++ b/guix/git.scm
> @@ -593,6 +593,11 @@ is true, limit to only refs/tags."
>      (and (ref? ref)
>           (or (not tags?) (tag? ref))))
>  
> +  (define (remote-head->ref remote)
> +    (let ((name (remote-head-name remote)))
> +      (and (include? name)
> +           name)))
> +
>    (with-libgit2
>     (call-with-temporary-directory
>      (lambda (cache-directory)
> @@ -600,14 +605,13 @@ is true, limit to only refs/tags."
>               ;; Create an in-memory remote so we don't touch disk.
>               (remote (remote-create-anonymous repository url)))
>          (remote-connect remote)
> -        (remote-disconnect remote)
> -        (repository-close! repository)
> -
> -        (filter-map (lambda (remote)
> -                      (let ((name (remote-head-name remote)))
> -                        (and (include? name)
> -                             name)))
> -                    (remote-ls remote)))))))
> +
> +        (let* ((remote-heads (remote-ls remote))
> +               (refs (filter-map remote-head->ref remote-heads)))
> +          ;; Wait until we're finished with the repository before closing it.
> +          (remote-disconnect remote)
> +          (repository-close! repository)
> +          refs))))))
>  
>
>  ;;;

signature.asc
Description: PGP signature