all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Xinglu Chen <public@yoctocell.xyz>
To: "Ludovic Courtès" <ludo@gnu.org>
Cc: Sarah Morgensen <iskarian@mgsn.dev>, 50359@debbugs.gnu.org
Subject: [bug#50359] [PATCH] import: Add 'generic-git' updater.
Date: Fri, 10 Sep 2021 15:23:46 +0200	[thread overview]
Message-ID: <87bl50zhr1.fsf@yoctocell.xyz> (raw)
In-Reply-To: <87o890ddz7.fsf_-_@gnu.org>

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

On Fri, Sep 10 2021, Ludovic Courtès wrote:

> Hello,
>
> This looks very cool!

Thanks for taking a look!

It’s still a WIP, but I think it’s getting there.  :-)

> Xinglu Chen <public@yoctocell.xyz> skribis:
>
>> From f924dbb835425f6b9a5796918125592870391405 Mon Sep 17 00:00:00 2001
>> Message-Id: <f924dbb835425f6b9a5796918125592870391405.1631125652.git.public@yoctocell.xyz>
>> From: Xinglu Chen <public@yoctocell.xyz>
>> Date: Fri, 3 Sep 2021 17:50:56 +0200
>> Subject: [PATCH] import: Add 'generic-git' updater.
>>
>> * guix/import/git.scm: New file.
>> * doc/guix.texi (Invoking guix refresh): Document it.
>> * Makefile.am (MODULES): Register it.
>> * guix/git.scm (ls-remote-refs): New procedure.
>>
>> Co-authored-by: Sarah Morgensen <iskarian@mgsn.dev>
>
> Overall LGTM; comments below.
>
>> diff --git a/doc/guix.texi b/doc/guix.texi
>> index 36a0c7f5ec..26afb1607a 100644
>> --- a/doc/guix.texi
>> +++ b/doc/guix.texi
>> @@ -11920,6 +11920,33 @@ the updater for @uref{https://launchpad.net, Launchpad} packages.
>>  @item generic-html
>>  a generic updater that crawls the HTML page where the source tarball of
>>  the package is hosted, when applicable.
>> +@item generic-git
>
> Please add a newline above.

Noted.

>> +@lisp
>> +(package
>> +  (name "foo")
>> +  ;; ...
>> +  (properties
>> +    '((tag-prefix . "^release0-")
>> +      (tag-suffix . "[a-z]?$")
>> +      (tag-version-delimiter . ":"))))
>> +@end lisp      
>
> Very nice.  Perhaps s/tag-/release-tag-/ for clarity?

Good idea.

>> +(define* (ls-remote-refs url #:key tags?)
>> +  "Return the list of references advertised at Git repository URL.  If TAGS?
>> +is true, limit to only refs/tags."
>
> To remain consistent with existing naming conventions, I’d call it
> ‘remote-refs’.
>
>> +  (with-libgit2
>> +   (call-with-temporary-directory
>> +    (lambda (cache-directory)
>> +      (let* ((repository (repository-init cache-directory))
>> +             ;; Create an in-memory remote so we don't touch disk.
>> +             (remote (remote-create-anonymous repository url)))
>
> Too bad we need to create an empty repo; hopefully it costs next to
> nothing though.
>
>> +        (remote-connect remote)
>> +        (remote-disconnect remote)
>> +        (repository-close! repository)
>> +
>> +        (filter include? (map remote-head-name (remote-ls remote))))))))
>
> Use ‘filter-map’.
>
>> +(define* (get-version-mapping tags #:key prefix suffix delim pre-releases?)
>
> Please add a docstring and remove ‘get-’ from the name.  :-)
>
>> +  (define (guess-delim)
>> +    (let ((total (length tags))
>> +          (dots (reduce + 0 (map (cut string-count <> #\.) tags)))
>> +          (dashes (reduce + 0 (map (cut string-count <> #\-) tags)))
>> +          (underscores (reduce + 0 (map (cut string-count <> #\_) tags))))
>> +      (display (format #t "total: ~d, dots: ~d, dashes ~d, underscores ~d~%"
>> +              total dots dashes underscores))
>
> Leftover?  (Also display + format.)

Yep.  :-)

> Please spell out ‘delimiter’ (info "(guix) Formatting Code").
>
>> +      (cond
>> +       ((>= dots (* total 0.35)) ".")
>> +       ((>= dashes (* total 0.8)) "-")
>> +       ((>= underscores (* total 0.8)) "_")
>> +       (else ""))))
>
> That’s a fancy heuristic.  :-)

Yeah, it was suggested by Sarah, and in my testing it seems to work
pretty well.  :-)

>> +  (let ((mapping (fold alist-cons '() (map get-version tags) tags)))
>> +    (stable-sort! (filter car mapping) entry<?)))
>
> It’s perhaps clearer written like this:
>
>   (stable-sort (filter-map (lambda (tag)
>                              (let ((version (get-version tag)))
>                                (and version (cons version tag))))
>                             tags)
>                entry<?)

Agreed, I will use your suggested version.

>> +(define* (get-latest-tag url #:key prefix suffix delim pre-releases?)
>> +  "Return the latest tag available from the Git repository at URL."
>
> Maybe “the tag corresponding to the latest version”.

Yeah, as the latest tag might not correspond to a release...

> s/get-latest-tag/latest-tag/
>
>> +  (define (pre-release? tag)
>> +    (any (lambda (rx) (regexp-exec (make-regexp rx regexp/icase) tag))
>> +              %pre-release-words))
>
> Better call ‘make-regexp’ only once; so you could change
> ‘%pre-release-words’ to be a list of regexp objects instead of a list of
> strings.

Noted.

>> +(define (latest-git-tag-version package tag-prefix tag-suffix
>> +                                tag-version-delimiter refresh-pre-releases?)
>> +  "Given a PACKAGE, the TAG-PREFIX, TAG-SUFFIX, TAG-VERSION-DELIMITER, and
>> +REFRESH-PRE-RELEASES?  properties of PACKAGE, returns the latest version of
>> +PACKAGE."
>
> Maybe s/refresh-pre-releases?/accept-pre-preleases?/

‘accept-pre-releases?’ ;-)

> Since this procedure takes a package, it probably doesn’t need the other
> arguments: it can extract them from the package properties, rather than
> doing it at the call site.

Good point.

>> +(define (latest-git-release package)
>> +  "Return the latest release of PACKAGE."
>> +  (let* ((name (package-name package))
>> +         (properties (package-properties package))
>> +         (tag-prefix (assq-ref properties 'tag-prefix))
>> +         (tag-suffix (assq-ref properties 'tag-suffix))
>> +         (tag-version-delimiter (assq-ref properties 'tag-version-delimiter))
>> +         (refresh-pre-releases? (assq-ref properties 'refresh-pre-releases?))
>> +         (old-version (package-version package))
>> +         (url (git-reference-url (origin-uri (package-source package))))
>> +         (new-version (latest-git-tag-version package
>> +                                              tag-prefix
>> +                                              tag-suffix
>> +                                              tag-version-delimiter
>> +                                              refresh-pre-releases?)))
>> +
>> +    (if new-version
>> +        (upstream-source
>> +         (package name)
>> +         (version new-version)
>> +         (urls (list url)))
>> +        ;; No new release or no tags available.
>> +        #f)))
>
> Simply: (and new-version (upstream-source …)).
>
> It would have been nice to have tests.  I think testing
> ‘latest-git-release’ should be feasible without too much hassle using
> the (guix tests git) infrastructure, as is done in tests/git.scm, with a
> package referring to a locally-created repo using a git-reference with a
> file:// URL.

Thanks for the pointers!  I will look into it.

> Could you send an updated patch?

Sure!  Thanks for the review!  :-)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

  reply	other threads:[~2021-09-10 13:25 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 [this message]
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
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=87bl50zhr1.fsf@yoctocell.xyz \
    --to=public@yoctocell.xyz \
    --cc=50359@debbugs.gnu.org \
    --cc=iskarian@mgsn.dev \
    --cc=ludo@gnu.org \
    /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.