Re: [PATCH] draft addition of github updater

[Ludovic Courtès]

Ben Woodcroft <address@hidden> skribis:

> It seems I miscounted before, but now it is 129 of 146 github
> "release" packages recognised with 28 suggesting an update - see the
> end of email for details. There is one false positive:
>
> gnu/packages/ocaml.scm:202:13: camlp4 would be upgraded from 4.02+6 to
> 4.02.0+1
>
> This happens because the newer versions were not made as official
> releases just tags, so the newer versions are omitted from the API
> response, plus there's the odd version numbering scheme. Guix is up to
> date.

I guess we could filter out such downgrades by adding a call to
‘version>?’, no?

>>> I have two questions:
>>>
>>> 1. Some guess-work is required to get between the version as it is
>>> defined in guix, and that presented in the github json, where only the
>>> "tag_name" is available. Is it OK to be a little speculative in this
>>> conversion e.g. "v1.0" => "1.0"?
>> I guess so.  What I would do is do that conversion when the tag matches
>> “^v[0-9]” and leave the tag as-is in other cases.  WDYT?
>>
>> We can always add more heuristics later if we find that there’s another
>> widely-used convention for tag names.
> Most seem to follow those few conventions, but there's still repos
> that decided to be different e.g.
>
> https://github.com/vapoursynth/vapoursynth/archive/R28.tar.gz
> https://github.com/synergy/synergy/archive/v1.7.4-stable.tar.gz
>
> Having gotten this far, I wonder if I've gone about it
> backwards. Currently the updater works by asserting it is a
> refreshable package by interrogating the source URI only. But it might
> be easier to determine this with an API response on hand, by matching
> the current release version number to a tag. Then if we assume the
> same transformation of tag to version holds in the newest release, the
> reverse transformation can be used on the newest tag to convert it
> back into a version number. By transformation I mean addition of
> [a-z\.\-] characters before and after the version number. This is
> easier because guesswork is only needed to convert between the tag and
> version number, without reference to a URI.
>
> This means more work for me, is it a good idea? As I understand it
> would involve returning #t more often from "github-package?". If #f is
> returned by an updater, do the updaters further down the chain get a
> bite at the cherry too? It doesn't matter for now since the github
> updater is last, but it might in the future.

I’m not sure I completely follow ;-), but it’s fine to hard-code the
v[0-9\.]+ convention for now, esp. if it works for most packages.

>> I guess (guix import github) could contain something like:
>>
>>    (define %github-token
>>      ;; Token to be passed to Github.com to avoid the 60-request per hour
>>      ;; limit, or #f.
>>      (make-parameter (getenv "GUIX_GITHUB_TOKEN")))
>>
>> and we’d need to document that, or maybe write a message hinting at it
>> when we know the limit has been reached.
>>
>> WDYT?
> Seems we were all thinking the same thing - I've integrated
> this. Should we check that the token matches ^[0-9a-f]+$ for security
> and UI?

I think it’s fine as is.  There’s no security issue on the client side
AFAICS.

>> I was thinking we could have a generic Git updater that would look
>> for available tags upstream. I wonder how efficient that would be
>> compared to using the GitHub-specific API, and if there would be
>> other differences. What are your thoughts on this?
> This sounds like an excellent idea, but I was unable to find any way
> to fetch tags without a clone first. A clone could take a long time
> and a lot of bandwidth I would imagine. Also there's no way to discern
> regular releases from pre-releases I don't think. It is a bit unclear
> to me how conservative these updaters should be, are tags sufficiently
> synonymous with releases so as to be reported by refresh?

I think we’d have to hard-code heuristics to distinguish release tags
from other tags.  Typically, again, considering only tags that match
‘v[0-9\.]+’.

Well, future work!  :-)

> From a42eda6b9631cc28dfdd02d2c8bb02eabb2626b9 Mon Sep 17 00:00:00 2001
> From: Ben Woodcroft <address@hidden>
> Date: Sun, 15 Nov 2015 10:18:05 +1000
> Subject: [PATCH] import: Add github-updater.
>
> * guix/import/github.scm: New file.
> * guix/scripts/refresh.scm (%updaters): Add %GITHUB-UPDATER.
> * doc/guix.texi (Invoking guix refresh): Mention it.

[...]

> +The @code{github} updater uses the
> address@hidden://developer.github.com/v3/, GitHub API} to query for new
> +releases. When used repeatedly e.g. when refreshing all packages, GitHub
> +will eventually refuse to answer any further API requests. By default 60
> +API requests per hour are allowed, and a full refresh on all GitHub
> +packages in Guix requires more than this. Authentication with GitHub
> +through the use of an API token alleviates these limits. To use an API
> +token, set the environment variable @code{GUIX_GITHUB_TOKEN} to a token
> +procured from @uref{https://github.com/settings/tokens} or otherwise.

Good!  Please make sure to leave two spaces after end-of-sentence periods.

Also, maybe this paragraph should be moved after the @table that lists
updaters?  Otherwise it mentions the ‘github’ updater before it has been
introduced.

> +;; TODO: Are all of these imports used?
> +(define-module (guix import github)

Should be easily checked.  ;-)

> +(define (json-fetch* url)
> +  "Return a list/hash representation of the JSON resource URL, or #f on
> +failure."
> +  (call-with-output-file "/dev/null"
> +    (lambda (null)
> +      (with-error-to-port null
> +        (lambda ()
> +          (call-with-temporary-output-file
> +           (lambda (temp port)
> +             (and (url-fetch url temp)
> +                  (call-with-input-file temp json->scm)))))))))

Rather use (guix http-client) and something like:

  (let ((port (http-fetch url)))
    (dynamic-wind
      (const #t)
      (lambda ()
        (json->scm port))
      (lambda ()
        (close-port port))))

This avoids the temporary file creation etc.

> +;; TODO: is there some code from elsewhere in guix that can be used instead 
> of
> +;; redefining?
> +(define (find-extension url)
> +  "Return the extension of the archive e.g. '.tar.gz' given a URL, or
> +false if none is recognized"
> +  (find (lambda x (string-suffix? (first x) url))
> +        (list ".tar.gz" ".tar.bz2" ".tar.xz" ".zip" ".tar")))

Remove this procedure and use (file-extension url) instead, from (guix utils).

> +(define (github-user-slash-repository url)
> +  "Return a string e.g. arq5x/bedtools2 of the owner and the name of the
> +repository separated by a forward slash, from a string URL of the form
> +'https://github.com/arq5x/bedtools2/archive/v2.24.0.tar.gz'"
> +  (let ((splits (string-split url #\/)))
> +    (string-append (list-ref splits 3) "/" (list-ref splits 4))))

Rather write it as:

  (match (string-split (uri-path (string->uri url)) #\/)
    ((owner project . rest)
     (string-append owner "/" project)))

> +    (if (eq? json #f)

Rather: (if (not json).

However, ‘http-fetch’ raises an &http-error condition when something
goes wrong (it never returns #f.)  So…

> +        (if token
> +            (error "Error downloading release information through the GitHub
> +API when using a GitHub token")
> +            (error "Error downloading release information through the GitHub
> +API. This may be fixed by using an access token and setting the environment
> +variable GUIX_GITHUB_TOKEN, for instance one procured from
> +https://github.com/settings/tokens";))

… this can be removed, and the whole thing becomes:

  (guard (c ((http-get-error? c)
             (warning (_ "failed to access ~a: ~a (~a)~%")
                      (uri->string (http-get-error-uri c))
                      (http-get-error-code c)
                      (http-get-error-reason c))))
    …)

> +        (let ((proper-releases
> +               (filter
> +                (lambda (x)
> +                  ;; example pre-release:
> +                  ;; https://github.com/wwood/OrfM/releases/tag/v0.5.1
> +                  ;; or an all-prerelease set
> +                  ;; https://github.com/powertab/powertabeditor/releases
> +                  (eq? (assoc-ref (hash-table->alist x) "prerelease") #f))

Simply: (not (hash-ref x "prerelease")).

> +          (if (eq? (length proper-releases) 0) #f ;empty releases list
> +              (let*
> +                  ((tag (assoc-ref (hash-table->alist (first 
> proper-releases))
> +                                   "tag_name"))

Rather:

  (match proper-releases
    (()                       ;empty release list
     #f)
    ((release . rest)         ;one or more releases
     (let* ((tag (hash-ref release "tag_name")) …)
       …)))

> +(define (latest-release guix-package)
> +  "Return an <upstream-source> for the latest release of GUIX-PACKAGE."
> +  (let* ((pkg (specification->package guix-package))

Someone (Ricardo?) proposed recently to pass a package object instead of
a package name to ‘latest-release’.

We should do that ideally before this patch goes in, or otherwise soon.

> -                 ((guix import pypi) => %pypi-updater)))
> +                 ((guix import pypi) => %pypi-updater)
> +                 %github-updater))

Write it as:

  ((guix import github) => %github-updater)

so that users who do not have guile-json can still use ‘guix refresh’.

Could you send an updated patch?  Looks like we’re almost there.

Thank you!

Ludo’.