[bug#50072] [PATCH WIP 0/4] Add upstream updater for git-fetch origins.

[Maxime Devos <maximedevos@telenet.be>]

[-- Attachment #1.1: Type: text/plain, Size: 8171 bytes --]

Sarah Morgensen schreef op ma 16-08-2021 om 12:56 [-0700]:
> Hi Maxime,
> 
> Thanks for taking a look at this. :)
> 
> Maxime Devos <maximedevos@telenet.be> writes:
> 
> > Sarah Morgensen schreef op zo 15-08-2021 om 16:25 [-0700]:
> > > * guix/git-download.scm (checkout-to-store): New procedure.
> > > * guix/upstream.scm (guess-version-transform)
> > > (package-update/git-fetch): New procedures.
> > > (%method-updates): Add GIT-FETCH mapping.
> > 
> > Does it support packages defined like (a)
> > 
> > (define-public gnash
> >   (let ((commit "583ccbc1275c7701dc4843ec12142ff86bb305b4")
> >         (revision "0"))
> >     (package
> >       (name "gnash")
> >       (version (git-version "0.8.11" revision commit))
> >       (source (git-reference
> >                 (url "https://example.org")
> >                 (commit commit)))
> >       [...])))
> 
> No, it doesn't.  Since the commit definition isn't part of the actual
> package definition, the current code has no way of updating it.  It
> would require a rewrite of the edit-in-place logic with probably a lot
> of special-casing.

Perhaps a 'surrounding-expression-location' procedure can be defined?

(define (surrounding-expression-location inner-location)
  "Determine the location of the S-expression that surrounds the S-expression
at INNER-LOCATION, or #false if the inner S-expression is at the top-level."
  ??? Something like 'read', but in reverse, maybe?
  Doesn't need to support every construct, just "string without escapes" and
  (parentheses other-things) might be good enough in practice for now)

Seems tricky to implement, but it would be more robust than relying
on conventions like ‘the surrounding 'let' can be found by moving two columns
and two lines backwards’.  Or see another method (let&) below that is actually
implemented ...

> There are currently ~1250 package which use this format, though, so it
> could be worth it...  Perhaps what we actually need is a better idiom to
> express this situation.  Package properties ('git-commit)?  A 'git-version*'?
> 
> --8<---------------cut here---------------start------------->8---
> (define (git-version* version revision)
>   (let* ((source (package-source this-package))
>          (commit (git-reference-commit (origin-uri source))))
>     (git-version version revision commit)))
> --8<---------------cut here---------------end--------------->8---
> 
> I'm not sure if binding order would be an issue with that.

The 'file-name' field of 'origin' is not thunked, and refers to the 'version'
field of the 'package' (also not thunked).  If 'version' would use the 'git-version*'
from above, then there would be a loop (I'm having the 'gnash' package in mind,
see "guix edit gnash").  And git-version* cannot be a procedure, it must be a macro,
as it used 'this-package', which can only be expanded inside a package definition.

Alternatively, what do you think of a let& macro, that adjusts the inner expression
to have the source location of the 'let&' form:

(define-syntax with-source-location
  (lambda (s)
    (syntax-case s ()
      ((_ (exp . exp*) source)
       "Expand to (EXP . EXP*), but with the source location replaced
by the source location of SOURCE."
       (datum->syntax s (cons #'exp #'exp*) #:source (syntax-source #'source))))))

(define-syntax let&
  (lambda (s)
    "Like 'let', but let the inner expression have the location
of the 'let&' form when it is expanded.  Only a single inner
expression is allowed."
    (syntax-case s ()
      ((_ bindings exp)
       #'(let bindings
           (with-source-location exp s))))))

That way, 'update-package-source' doesn't need to know about the surrounding
'let' form; it would simply use 'edit-expression' as usual (though something
like

                                    ,@(if (and old-commit new-commit)
                                          `((,old-commit . ,new-commit))
                                          '())

would need to be added, and something to replace ‘(revision "N")’ with
‘(revision "N+1")’.)

A complete example is attached (a.scm).  The previous usages of
(let ((commit ...) (revision ...)) ...) would need to be adjusted
to use let& instead (build-aux/update-guix-package.scm needs to
be adjusted as well).

Personally, I'd go with the 'let&' form

> > and (b)
> > 
> > (define-public gnash
> >   (package
> >     (name "gnash")
> >     (version "0.8.11")
> >     (source (git-reference
> >               (url "https://example.org")
> >               (commit commit))
> >     [...]))
> > ?
> 
> Is this missing a definition for commit? If it's like above, the same
> applies.  Or if you mean
> 
> --8<---------------cut here---------------start------------->8---
>      (source (git-reference
>                (url "https://example.org")
>                (commit "583ccbc1275c7701dc4843ec12142ff86bb305b"))
> --8<---------------cut here---------------end--------------->8---

The latter.

> Then that wouldn't be too hard to support.  There seem to be ~136
> packages with this idiom.

FWIW, the patch I sent modified 'update-package-source' to replace
the commit in this case (b) (but not case (a)).

> > [the patch Maxime sent]
> > 
> >    upstream-source?
> >    (package        upstream-source-package)        ;string
> >    (version        upstream-source-version)        ;string
> > -  (urls           upstream-source-urls)           ;list of strings
> > +  ; list of strings or a <git-reference>
> > +  (urls           upstream-source-urls)
> 
> Is it possible for an updater to want to return a list of
> <git-reference>?

No, 'git-fetch' from (guix git-download) only accepts a single <git-reference>
object, it doesn't support lists of <git-reference>.  It will throw a type
error if a list is passed.  Compare with 'url-fetch*', which does accept a list
of URLs (in which case it will fall-back to the second, the third, the fourth ...
entry when the first entry gives a 404 or something).

>   I'm still not sure what the purpose of multiple urls
> is, since nearly everthing seems to just take (first urls)...

As I understand it, the second, third, fourth ... URL (when using url-fetch)
are fall-backs.  Also, (guix upstream) sometimes distinguishes between the
different URLs, see e.g. package-update/url-fetch, which will try to choose a
tarball with the same kind of extension (.zip, .tar.gz, .tar.xz, ...) as the original
URI.

> >    (signature-urls upstream-source-signature-urls  ;#f | list of strings
> >                    (default #f))
> >    (input-changes  upstream-source-input-changes
> > @@ -361,6 +368,11 @@ values: 'interactive' (default), 'always', and 'never'."
> >                                                  system target)
> >    "Download SOURCE from its first URL and lower it as a fixed-output
> >  derivation that would fetch it."
> > +  (define url
> > +    (match (upstream-source-urls source)
> > +      ((first . _) first)
> > +      (_ (raise (formatted-message
> > +                 (G_ "git origins are unsupported by --with-latest"))))))
> >    (mlet* %store-monad ((url -> (first (upstream-source-urls source)))
> >                         (signature
> >                          -> (and=> (upstream-source-signature-urls source)
> > @@ -430,9 +442,23 @@ SOURCE, an <upstream-source>."
> >                                          #:key-download key-download)))
> >           (values version tarball source))))))
> 
> What is this 'upstream-source-compiler' actually used for?  I couldn't
> figure that out, so I just left it untouched.

It is used to ‘lower’ <upstream-source> objects.  More specifically,
transform-package-latest from (guix transformations) will sometimes
replace the 'source' of a package with a <upstream-source> object,
and 'upstream-source-compiler' is used to turn the <upstream-source>
into a (fixed-output) derivation that can be built into a
/gnu/store/...-checkout or /gnu/store/...-version.tar.gz file in the store.

Greetings,
Maxime

[-- Attachment #1.2: a.scm --]
[-- Type: text/x-scheme, Size: 2332 bytes --]

;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;;
;;; This file is part of GNU Guix.
;;;
;;; GNU Guix is free software; you can redistribute it and/or modify it
;;; under the terms of the GNU General Public License as published by
;;; the Free Software Foundation; either version 3 of the License, or (at
;;; your option) any later version.
;;;
;;; GNU Guix is distributed in the hope that it will be useful, but
;;; WITHOUT ANY WARRANTY; without even the implied warranty of
;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
;;; GNU General Public License for more details.
;;;
;;; You should have received a copy of the GNU General Public License
;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.

(use-modules (guix packages)
             (gnu packages animation)
             (guix git-download))

(define-syntax with-source-location
  (lambda (s)
    (syntax-case s ()
      ((_ (exp . exp*) source)
       "Expand to (EXP . EXP*), but with the source location replaced
by the source location of SOURCE."
       (datum->syntax s (cons #'exp #'exp*) #:source (syntax-source #'source))))))

(define-syntax let&
  (lambda (s)
    "Like 'let', but let the inner expression have the location
of the 'let&' form when it is expanded.  Only a single inner
expression is allowed."
    (syntax-case s ()
      ((_ bindings exp)
       #'(let bindings
           (with-source-location exp s))))))


(define-public gnash2
  ;; The last tagged release of Gnash was in 2013.
  (let& ((commit "583ccbc1275c7701dc4843ec12142ff86bb305b4")
         (revision "0"))
    (package
      (inherit gnash)
      (name "gnash2")
      (version (git-version "0.8.11" revision commit))
      (source
       (origin
         (method git-fetch)
         (uri (git-reference
               (url "https://git.savannah.gnu.org/git/gnash.git/")
               (commit commit)))
         (file-name (git-file-name name version))
         (patches (search-patches "gnash-fix-giflib-version.patch"))
         (sha256
          (base32 "0fh0bljn0i6ypyh6l99afi855p7ki7lm869nq1qj6k8hrrwhmfry")))))))

(format #t "old: ~a~%" (package-location gnash))
(format #t "new: ~a~%" (package-location gnash2))
;; ^ it says column 2, which is the column of the let& form.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]