| Message ID | 4885bc5c95529991846db147ca58f7445d3cd864.1718699410.git.mail@nicolasgoaziou.fr |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#71371,v2] gnu: svn-fetch: Allow specifying revisions as strings. | expand |
Hello, Nicolas Goaziou <mail@nicolasgoaziou.fr> writes: > * guix/svn-download.scm (<svn-reference>): > (svn-fetch): > (svn-multi-fetch): > * guix/build/svn.scm (svn-fetch): Revision can also be a string, not only > a number. > * doc/guix.texi (origin Reference): Document changes about REVISION field. > > Change-Id: Ibb17b539575fdf3daf895bd1ce39a40dd9b495cb > --- > v2: No longer ignore "-r" argument. Instead, allow strings, such as "HEAD". Yes, in practice, it means this relies on the tag being stable, which is the same assumption as for, e.g., tarballs. WDYT? While it may be useful to point to volatile references such as HEAD for internal projects (like I believe is also possible for our git fetcher) or the likes (with a hash of #f for example), I wouldn't like to see this used in the Guix tree (I'd consider it a bad practice). I'm not against merging this, but I think we should add a 'guix lint' rule that'd warn that some SVN reference should be specified if it wasn't. Does that sound reasonable?
Hello, Maxim Cournoyer <maxim.cournoyer@gmail.com> writes: > Nicolas Goaziou <mail@nicolasgoaziou.fr> writes: > >> * guix/svn-download.scm (<svn-reference>): >> (svn-fetch): >> (svn-multi-fetch): >> * guix/build/svn.scm (svn-fetch): Revision can also be a string, not only >> a number. >> * doc/guix.texi (origin Reference): Document changes about REVISION field. >> >> Change-Id: Ibb17b539575fdf3daf895bd1ce39a40dd9b495cb >> --- >> v2: No longer ignore "-r" argument. Instead, allow strings, such as "HEAD". Yes, in practice, it means this relies on the tag being stable, which is the same assumption as for, e.g., tarballs. WDYT? > > While it may be useful to point to volatile references such as HEAD for > internal projects (like I believe is also possible for our git fetcher) > or the likes (with a hash of #f for example), I wouldn't like to see > this used in the Guix tree (I'd consider it a bad practice). I'm not > against merging this, but I think we should add a 'guix lint' rule > that'd warn that some SVN reference should be specified if it wasn't. > > Does that sound reasonable? To be clear, I intend to use it in Guix tree, _in conjunction with a tag_. The purpose of this patch is to put the reference on the human-readable repository tag, not on the revision number. Sure, as already pointed out, some evil repository could modify contents associated to a tag because Subversion allows it. But that would defeat the whole purpose of tags, so I don't think that's common at all. Tags are not meant to be volatile. Again, as also pointed out, we trust tarballs, but the situation is exactly the same for them: one evil project could modify the contents of the tarball without changing its name. The consequence would then be the same: a hash mismatch. Anyway, this patch is there to make my life less miserable while writing a TeX Live updater. It is of no use (to me) if it is restricted to internal projects. I'm not putting pressure on anyone though, if it isn't accepted, I'll find other, probably more tedious, ways to complete the updater. Regards,
Hi, Nicolas Goaziou <mail@nicolasgoaziou.fr> writes: > Hello, > > Maxim Cournoyer <maxim.cournoyer@gmail.com> writes: > >> Nicolas Goaziou <mail@nicolasgoaziou.fr> writes: >> >>> * guix/svn-download.scm (<svn-reference>): >>> (svn-fetch): >>> (svn-multi-fetch): >>> * guix/build/svn.scm (svn-fetch): Revision can also be a string, not only >>> a number. >>> * doc/guix.texi (origin Reference): Document changes about REVISION field. >>> >>> Change-Id: Ibb17b539575fdf3daf895bd1ce39a40dd9b495cb >>> --- >>> v2: No longer ignore "-r" argument. Instead, allow strings, such as "HEAD". Yes, in practice, it means this relies on the tag being stable, which is the same assumption as for, e.g., tarballs. WDYT? >> >> While it may be useful to point to volatile references such as HEAD for >> internal projects (like I believe is also possible for our git fetcher) >> or the likes (with a hash of #f for example), I wouldn't like to see >> this used in the Guix tree (I'd consider it a bad practice). I'm not >> against merging this, but I think we should add a 'guix lint' rule >> that'd warn that some SVN reference should be specified if it wasn't. >> >> Does that sound reasonable? > > To be clear, I intend to use it in Guix tree, _in conjunction with > a tag_. The purpose of this patch is to put the reference on the > human-readable repository tag, not on the revision number. [...] > Again, as also pointed out, we trust tarballs, but the situation is > exactly the same for them: one evil project could modify the contents of > the tarball without changing its name. The consequence would then be the > same: a hash mismatch. > > Anyway, this patch is there to make my life less miserable while writing > a TeX Live updater. It is of no use (to me) if it is restricted to > internal projects. I'm not putting pressure on anyone though, if it > isn't accepted, I'll find other, probably more tedious, ways to complete > the updater. I see. I don't have a strong opinion, if it eases the maintenance of the many texlive packages we carry, but it seems to me that an updater could do the work of associating an immutable SVN reference to a tag at the time of the import.
Hello, Maxim Cournoyer <maxim.cournoyer@gmail.com> writes: > Nicolas Goaziou <mail@nicolasgoaziou.fr> writes: >> Anyway, this patch is there to make my life less miserable while writing >> a TeX Live updater. It is of no use (to me) if it is restricted to >> internal projects. I'm not putting pressure on anyone though, if it >> isn't accepted, I'll find other, probably more tedious, ways to complete >> the updater. > I see. I don't have a strong opinion, if it eases the maintenance of > the many texlive packages we carry, but it seems to me that an updater > could do the work of associating an immutable SVN reference to a tag at > the time of the import. That's indeed the plan B I had in mind when I wrote about other ways. It's a bit more involved, though. Fair enough. I'll close this door and open a new one. Regards,
Hello, Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > I see. I don't have a strong opinion, if it eases the maintenance of > the many texlive packages we carry, but it seems to me that an updater > could do the work of associating an immutable SVN reference to a tag at > the time of the import. I share that sentiment too. But Nicolas, do let us know if doing the work in the importer turns out to be more difficult than expected. Ludo’.
diff --git a/doc/guix.texi b/doc/guix.texi index 0102fd0fad..0b30ed0a72 100644 --- a/doc/guix.texi +++ b/doc/guix.texi @@ -8283,7 +8283,7 @@ origin Reference The URL of the Subversion repository to clone. @item @code{revision} -This string denotes revision to fetch specified as a number. +This string or number denotes revision to fetch. @item @code{recursive?} (default: @code{#f}) This Boolean indicates whether to recursively fetch Subversion diff --git a/guix/build/svn.scm b/guix/build/svn.scm index 875d3c50ca..58b74fd37a 100644 --- a/guix/build/svn.scm +++ b/guix/build/svn.scm @@ -46,7 +46,12 @@ (define* (svn-fetch url revision directory ;; Trust the server certificate. This is OK as we ;; verify the checksum later. This can be removed when ;; ca-certificates package is added. - "--trust-server-cert" "-r" (number->string revision) + "--trust-server-cert" + + ;; Revision. It can be a number or a string. + "-r" (if (number? revision) + (number->string revision) + revision) ;assume string ;; Disable keyword substitutions (keywords are CVS-like strings ;; like "$Date$", "$Id$", and so on) for two reasons: (1) some diff --git a/guix/svn-download.scm b/guix/svn-download.scm index 62649e4374..e21091defc 100644 --- a/guix/svn-download.scm +++ b/guix/svn-download.scm @@ -64,7 +64,7 @@ (define-record-type* <svn-reference> svn-reference make-svn-reference svn-reference? (url svn-reference-url) ; string - (revision svn-reference-revision) ; number + (revision svn-reference-revision) ; number|string (recursive? svn-reference-recursive? (default #f)) (user-name svn-reference-user-name (default #f)) (password svn-reference-password (default #f))) @@ -113,7 +113,8 @@ (define (svn-fetch-builder svn hash-algo) (or (and (download-method-enabled? 'upstream) (svn-fetch (getenv "svn url") - (string->number (getenv "svn revision")) + (let ((r (getenv "svn revision"))) + (or (string->number r) r)) #$output #:svn-command #+(file-append svn "/bin/svn") #:recursive? (match (getenv "svn recursive?") @@ -152,7 +153,10 @@ (define* (svn-fetch ref hash-algo hash #:env-vars `(("svn url" . ,(svn-reference-url ref)) ("svn revision" - . ,(number->string (svn-reference-revision ref))) + . ,(let ((revision (svn-reference-revision ref))) + (if (number? revision) + (number->string revision) + revision))) ,@(if (svn-reference-recursive? ref) `(("svn recursive?" . "yes")) '()) @@ -187,7 +191,7 @@ (define-record-type* <svn-multi-reference> svn-multi-reference make-svn-multi-reference svn-multi-reference? (url svn-multi-reference-url) ; string - (revision svn-multi-reference-revision) ; number + (revision svn-multi-reference-revision) ; number|string (locations svn-multi-reference-locations) ; list of strings (recursive? svn-multi-reference-recursive? (default #f)) (user-name svn-multi-reference-user-name (default #f)) @@ -240,7 +244,8 @@ (define (svn-multi-fetch-builder svn hash-algo) (mkdir-p (string-append #$output "/" (dirname location)))) (and (download-method-enabled? 'upstream) (svn-fetch (string-append (getenv "svn url") "/" location) - (string->number (getenv "svn revision")) + (let ((r (getenv "svn revision"))) + (or (string->number r) r)) (if (string-suffix? "/" location) (string-append #$output "/" location) (string-append #$output "/" (dirname location))) @@ -292,7 +297,10 @@ (define* (svn-multi-fetch ref hash-algo hash ("svn locations" . ,(object->string (svn-multi-reference-locations ref))) ("svn revision" - . ,(number->string (svn-multi-reference-revision ref))) + . ,(let ((revision (svn-multi-reference-revision ref))) + (if (number? revision) + (number->string revision) + revision))) ,@(if (svn-multi-reference-recursive? ref) `(("svn recursive?" . "yes")) '())