all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Sarah Morgensen <iskarian@mgsn.dev>
To: Xinglu Chen <public@yoctocell.xyz>
Cc: "Ludovic Courtès" <ludo@gnu.org>, 50359@debbugs.gnu.org
Subject: [bug#50359] [PATCH 3/3] import: Add 'generic-git' updater.
Date: Thu, 16 Sep 2021 16:42:38 -0700	[thread overview]
Message-ID: <867dfghytt.fsf@mgsn.dev> (raw)
In-Reply-To: <87h7ekr8iw.fsf@yoctocell.xyz> (Xinglu Chen's message of "Thu, 16 Sep 2021 14:48:23 +0200 (10 hours, 1 minute, 2 seconds ago)")

[-- Attachment #1: Type: text/plain, Size: 5293 bytes --]

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

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix undeterministic segfaults in remote-refs. --]
[-- Type: text/x-patch, Size: 1291 bytes --]

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

  reply	other threads:[~2021-09-16 23:43 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-03 15:50 [bug#50359] [PATCH] import: Add 'generic-git' updater Xinglu Chen
2021-09-05  0:19 ` Sarah Morgensen
2021-09-05  1:03   ` Sarah Morgensen
2021-09-05 10:36   ` Xinglu Chen
2021-09-06  5:40     ` Sarah Morgensen
2021-09-06 12:20       ` Xinglu Chen
2021-09-07  1:00         ` Sarah Morgensen
2021-09-07 19:13           ` Xinglu Chen
2021-09-08 18:28             ` Xinglu Chen
2021-09-10  8:36               ` Ludovic Courtès
2021-09-10 13:23                 ` Xinglu Chen
2021-09-05 13:11   ` Xinglu Chen
2021-09-06  3:14     ` Sarah Morgensen
2021-09-10 16:20 ` [bug#50359] [PATCH 0/3] " Xinglu Chen
2021-09-10 16:21   ` [bug#50359] [PATCH 1/3] tests: git: Don't read from the users global Git config file Xinglu Chen
2021-09-10 16:21   ` [bug#50359] [PATCH 2/3] tests: git: Make 'tag' directive non-interactive Xinglu Chen
2021-09-13  8:03     ` Ludovic Courtès
2021-09-10 16:21   ` [bug#50359] [PATCH 3/3] import: Add 'generic-git' updater Xinglu Chen
2021-09-13  8:07     ` Ludovic Courtès
2021-09-16  9:09     ` Sarah Morgensen
2021-09-16 12:48       ` Xinglu Chen
2021-09-16 23:42         ` Sarah Morgensen [this message]
2021-09-17  7:48           ` Xinglu Chen
2021-09-17  8:04   ` [bug#50359] [PATCH v3 0/3] " Xinglu Chen
2021-09-17  8:04     ` [bug#50359] [PATCH v3 1/3] tests: git: Don't read from the users global Git config file Xinglu Chen
2021-09-17  8:04     ` [bug#50359] [PATCH v3 2/3] tests: git: Make 'tag' directive non-interactive Xinglu Chen
2021-09-17  8:04     ` [bug#50359] [PATCH v3 3/3] import: Add 'generic-git' updater Xinglu Chen
2021-09-18 17:47     ` bug#50359: [PATCH v3 0/3] " Ludovic Courtès
2021-09-15  8:44 ` [bug#50359] [PATCH 3/3] import: " iskarian
2021-09-15 11:59   ` Xinglu Chen
2021-09-16  9:46     ` Sarah Morgensen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=867dfghytt.fsf@mgsn.dev \
    --to=iskarian@mgsn.dev \
    --cc=50359@debbugs.gnu.org \
    --cc=ludo@gnu.org \
    --cc=public@yoctocell.xyz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.