| Message ID | 867dfghytt.fsf@mgsn.dev |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#50359] import: Add 'generic-git' updater. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/applying patch | fail | View Laminar job |
| cbaines/issue | success | View issue |
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)))))) > > > ;;;
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)))))) ;;;