GNU bug report logs - #50359
[PATCH] import: Add 'generic-git' updater.

Previous Next

Full log

Message #77 received at 50359 <at> debbugs.gnu.org (full text, mbox):

From: Sarah Morgensen <iskarian <at> mgsn.dev>
To: Xinglu Chen <public <at> yoctocell.xyz>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 50359 <at> debbugs.gnu.org
Subject: Re: [bug#50359] [PATCH 3/3] import: Add 'generic-git' updater.
Date: Thu, 16 Sep 2021 16:42:38 -0700

[Message part 1 (text/plain, inline)]

Xinglu Chen <public <at> 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).

>> 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?

>> 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.

Now I just have to give the (guix upstream) some attention...

--
Sarah

[generic-git-fix-segfault.patch (text/x-patch, inline)]

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))))))
 
 
 ;;;

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.