[bug#71371] [PATCH] gnu: svn-fetch: Make revision field optional.

[bug#71371] [PATCH] gnu: svn-fetch: Make revision field optional.

[Nicolas Goaziou via Guix-patches via]

* guix/svn-download.scm (<svn-reference>): Set default value for REVISION
field to #F.
(svn-fetch):
(svn-multi-fetch): Take into consideration the revision can be a number or #F.
* guix/build/svn.scm (svn-fetch): Skip "-r" argument when revision is #F.
* doc/guix.texi (origin Reference): Document changes about REVISION field.

Change-Id: Idb0eeb7ca4121db3caf789a9658ac79b321439d4
---
 doc/guix.texi         |  5 +++--
 guix/build/svn.scm    |  6 ++++--
 guix/svn-download.scm | 16 ++++++++++------
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 9b60e6c603..5e1173b8c6 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -8263,8 +8263,9 @@ origin Reference
 @item @code{url}
 The URL of the Subversion repository to clone.
 
-@item @code{revision}
-This string denotes revision to fetch specified as a number.
+@item @code{revision} (default: @code{#f})
+This field denotes the revision to fetch, as a number.  It can also be
+set to @code{#f}, for example when @var{url} contains a tag reference.
 
 @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..3ae6519628 100644
--- a/guix/build/svn.scm
+++ b/guix/build/svn.scm
@@ -46,8 +46,7 @@ (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"
            ;; Disable keyword substitutions (keywords are CVS-like strings
            ;; like "$Date$", "$Id$", and so on) for two reasons: (1) some
            ;; expansions depend on the local time zone, and (2) SWH disables
@@ -61,6 +60,9 @@ (define* (svn-fetch url revision directory
              ,@(if recursive?
                    '()
                    (list "--ignore-externals"))
+             ,@(if revision
+                   (list "-r" (number->string revision))
+                   '())
              ,url ,directory))
     #t))
 
diff --git a/guix/svn-download.scm b/guix/svn-download.scm
index bdd9c39eb5..dc9856cfb1 100644
--- a/guix/svn-download.scm
+++ b/guix/svn-download.scm
@@ -63,7 +63,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 (default #f))  ; number or #f
   (recursive? svn-reference-recursive? (default #f))
   (user-name  svn-reference-user-name (default #f))
   (password   svn-reference-password (default #f)))
@@ -120,7 +120,9 @@ (define* (svn-fetch ref hash-algo hash
 
             (or (and (download-method-enabled? 'upstream)
                      (svn-fetch (getenv "svn url")
-                                (string->number (getenv "svn revision"))
+                                (match (getenv "svn revision")
+                                  ("#f" #f)
+                                  (s (string->number s)))
                                 #$output
                                 #:svn-command #+(file-append svn "/bin/svn")
                                 #:recursive? (match (getenv "svn recursive?")
@@ -145,7 +147,7 @@ (define* (svn-fetch ref hash-algo hash
                       #:env-vars
                       `(("svn url" . ,(svn-reference-url ref))
                         ("svn revision"
-                         . ,(number->string (svn-reference-revision ref)))
+                         . ,(object->string (svn-reference-revision ref)))
                         ,@(if (svn-reference-recursive? ref)
                               `(("svn recursive?" . "yes"))
                               '())
@@ -173,7 +175,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 or #f
   (locations  svn-multi-reference-locations)           ; list of strings
   (recursive? svn-multi-reference-recursive? (default #f))
   (user-name  svn-multi-reference-user-name (default #f))
@@ -233,7 +235,9 @@ (define* (svn-multi-fetch ref hash-algo hash
                      (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"))
+                                   (match (getenv "svn revision")
+                                     ("#f" #f)
+                                     (s (string-to-number s)))
                                    (if (string-suffix? "/" location)
                                        (string-append #$output "/" location)
                                        (string-append #$output "/" (dirname location)))
@@ -271,7 +275,7 @@ (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)))
+                         . ,(object->string (svn-multi-reference-revision ref)))
                         ,@(if (svn-multi-reference-recursive? ref)
                               `(("svn recursive?" . "yes"))
                               '())

base-commit: bf202e8bdd10dbd01da391ef635d1a31197251c1
-- 
2.41.0

[bug#71371] [PATCH] gnu: svn-fetch: Make revision field optional.

[Ludovic Courtès]

Hello!

Nicolas Goaziou <mail@nicolasgoaziou.fr> skribis:

> -@item @code{revision}
> -This string denotes revision to fetch specified as a number.
> +@item @code{revision} (default: @code{#f})
> +This field denotes the revision to fetch, as a number.  It can also be
> +set to @code{#f}, for example when @var{url} contains a tag reference.

Hmm, IIRC, tags in svn are mutable, no?

My recollection is that there’s no distinction between a directory, a
branch, and a tag: tags and branches are just a copy (‘svn cp’) of a
directory that can change over time.  Thus, you can’t rely on a tag as
an unambiguous reference.

Am I right?

Thanks,
Ludo’.

[bug#71371] [PATCH] gnu: svn-fetch: Make revision field optional.

[Nicolas Goaziou via Guix-patches via]

Hello,

Ludovic Courtès <ludo@gnu.org> writes:

> Nicolas Goaziou <mail@nicolasgoaziou.fr> skribis:
>
>> -@item @code{revision}
>> -This string denotes revision to fetch specified as a number.
>> +@item @code{revision} (default: @code{#f})
>> +This field denotes the revision to fetch, as a number.  It can also be
>> +set to @code{#f}, for example when @var{url} contains a tag reference.
>
> Hmm, IIRC, tags in svn are mutable, no?

Disclaimer: prior to this patch, I knew nothing about SVN. So feel free
to take my answers with a grain of salt.

Now to answer your question, yes, they are mutable. But in practice,
altering tagged contents seems to be frowned upon in the
trunk-branch-tag trilogy.

> My recollection is that there’s no distinction between a directory, a
> branch, and a tag: tags and branches are just a copy (‘svn cp’) of a
> directory that can change over time.  Thus, you can’t rely on a tag as
> an unambiguous reference.
>
> Am I right?

You are certainly right, but I think this patch still makes sense for
projects that swear to never change tagged contents, which could
possibly be the case for most of the projects using SVN. Every now and
then, we will encounter a project that does modify such contents, but it
also currently happens with tarballs: sometimes, upstream modifies the
contents of a tarball without changing its name, inducing a hash
mismatch.

My concern here is that some Guix packages relying on `svn-fetch'
provide both a tag and a revision, which is redundant. We could get rid
of the tag, but the revision is not human-friendly.

Regards,
-- 
Nicolas Goaziou

[bug#71371] [PATCH] gnu: svn-fetch: Make revision field optional.

[Simon Tournier]

Hi Nicolas,

On Wed, 05 Jun 2024 at 07:52, Nicolas Goaziou via Guix-patches via <guix-patches@gnu.org> wrote:
> * guix/svn-download.scm (<svn-reference>): Set default value for REVISION
> field to #F.
> (svn-fetch):
> (svn-multi-fetch): Take into consideration the revision can be a number or #F.
> * guix/build/svn.scm (svn-fetch): Skip "-r" argument when revision is #F.
> * doc/guix.texi (origin Reference): Document changes about REVISION field.

Does it make sense to have a Subversion reference without any specific
revision?

> -                                (string->number (getenv "svn revision"))
> +                                (match (getenv "svn revision")
> +                                  ("#f" #f)
> +                                  (s (string->number s)))

[...]

> -                                   (string->number (getenv "svn revision"))
> +                                   (match (getenv "svn revision")
> +                                     ("#f" #f)
> +                                     (s (string-to-number s)))

I am probably missing something, why ’string-to-number’ and not
’string->number’?


Cheers,
simon

[bug#71371] [PATCH v2] gnu: svn-fetch: Allow specifying revisions as strings.

[Nicolas Goaziou via Guix-patches via]

* 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?

 doc/guix.texi         |  2 +-
 guix/build/svn.scm    |  7 ++++++-
 guix/svn-download.scm | 20 ++++++++++++++------
 3 files changed, 21 insertions(+), 8 deletions(-)

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"))
                               '())

base-commit: 30dbc8e7b0d94c0e38e8b7380260450e20035c58
-- 
2.45.1

[bug#71371] [PATCH v2] gnu: svn-fetch: Allow specifying revisions as strings.

[Maxim Cournoyer]

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?

-- 
Thanks,
Maxim

[bug#71371] [PATCH v2] gnu: svn-fetch: Allow specifying revisions as strings.

[Nicolas Goaziou via Guix-patches via]

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,
-- 
Nicolas Goaziou

[bug#71371] [PATCH v2] gnu: svn-fetch: Allow specifying revisions as strings.

[Maxim Cournoyer]

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.

-- 
Thanks,
Maxim

bug#71371: [PATCH v2] gnu: svn-fetch: Allow specifying revisions as strings.

[Nicolas Goaziou via Guix-patches via]

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,
-- 
Nicolas Goaziou

[bug#71371] [PATCH v2] gnu: svn-fetch: Allow specifying revisions as strings.

[Ludovic Courtès]

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’.