From debbugs-submit-bounces@debbugs.gnu.org Thu Sep 16 19:42:47 2021 Received: (at 50359) by debbugs.gnu.org; 16 Sep 2021 23:42:48 +0000 Received: from localhost ([127.0.0.1]:57873 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mR11j-0004gZ-GR for submit@debbugs.gnu.org; Thu, 16 Sep 2021 19:42:47 -0400 Received: from out2.migadu.com ([188.165.223.204]:34083) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mR11f-0004gP-SU for 50359@debbugs.gnu.org; Thu, 16 Sep 2021 19:42:45 -0400 X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mgsn.dev; s=key1; t=1631835762; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=BS09ZLZPxojuNp4q9Or7B6VHTO2+qDBBXy5mFsL59N0=; b=kni2L2m0ReE2FRvBp97aIE9Wbos+i7+/cRHFHm9TXc3vmkTYYs0IwoUUA2hetSSX3aj8t2 3vfpVygdHsMYFmJpSPMw+bglPhy9OIdxVpGP9Vo6TioT6+go5wPLv+nQfm0la+Jk7e1vQq 18NE+A1cxvgzzgHjX3V/ara84EAmEac= From: Sarah Morgensen To: Xinglu Chen Subject: Re: [bug#50359] [PATCH 3/3] import: Add 'generic-git' updater. References: <5d10dd1e65b0a65ada4a8102310c10de42f53e8d.1631290349.git.public@yoctocell.xyz> <86czp8j38z.fsf@mgsn.dev> <87h7ekr8iw.fsf@yoctocell.xyz> Date: Thu, 16 Sep 2021 16:42:38 -0700 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)") Message-ID: <867dfghytt.fsf@mgsn.dev> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Migadu-Flow: FLOW_OUT X-Migadu-Auth-User: iskarian@mgsn.dev X-Spam-Score: 2.0 (++) X-Spam-Report: Spam detection software, running on the system "debbugs.gnu.org", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see the administrator of that system for details. Content preview: Xinglu Chen 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 [...] Content analysis details: (2.0 points, 10.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 SPF_PASS SPF: sender matches SPF record 2.0 PDS_OTHER_BAD_TLD Untrustworthy TLDs [URI: yoctocell.xyz (xyz)] 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record -0.0 RCVD_IN_MSPIKE_H2 RBL: Average reputation (+2) [188.165.223.204 listed in wl.mailspike.net] X-Debbugs-Envelope-To: 50359 Cc: Ludovic =?utf-8?Q?Court=C3=A8s?= , 50359@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: 1.0 (+) --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Xinglu Chen writes: >>> Maybe we could have a =E2=80=98release-tag-date-scheme?=E2=80=99 proper= ty, 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=E2=80=99t have to be =E2=80=9C.=E2=80=9D delimiter though; if th= ey both have the same > delimiter it would be difficult to distinguish a version from a date, > e.g., =E2=80=9C1-2-3=E2=80=9D vs =E2=80=9C2021-03-23=E2=80=9D. 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 =E2=80=98guix refresh -u=E2=80=99, 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 =E2=80=98disable-tag-updater?=E2=80= =99 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=3D? 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=E2=80=99s > 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=E2=80=99t 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..." > > =E2=80=9CPACKAGE is a Git repository.=E2=80=9D doesn=E2=80=99t really sou= nd right, maybe =E2=80=9Cif > PACKAGE is hosted on a Git repository=E2=80=9D?' 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 =E2=80=98java-xom=E2=80=99 :-) > > That=E2=80=99s a weird scheme; setting the delimiter to =E2=80=9C.=E2=80= =9D doesn=E2=80=99t help since > it thinks that =E2=80=9C127=E2=80=9D is greater than =E2=80=9C1.3.7=E2=80= =9D. 'tag-version-regex would allow fixing this ;) > >> luakit >> ocproxy >> pitivi > > =E2=80=98pitivi=E2=80=99 has a pretty weird version string to begin with;= it may be > better to change it to the date: =E2=80=9C0.999.0-2021-05.0=E2=80=9D -> = =E2=80=9C2021-05.0=E2=80=9D. > >> 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 --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=generic-git-fix-segfault.patch Content-Description: Fix undeterministic segfaults in remote-refs. 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)))))) ;;; --=-=-=--