[PATCH] gnu-maintenance: update-package-source: Only update the desired package.

[PATCH] gnu-maintenance: update-package-source: Only update the desired package.

[宋文武]

Fixes <http://bugs.gnu.org/22693>.
Suggested by Andy Wingo.

* guix/upstream.scm (update-package-source): Use a customized 'substitute'
to work within lines of the package's source.
---
 guix/upstream.scm | 81 +++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 14 deletions(-)

diff --git a/guix/upstream.scm b/guix/upstream.scm
index cea23fe..f4bc09c 100644
--- a/guix/upstream.scm
+++ b/guix/upstream.scm
@@ -23,7 +23,11 @@
   #:use-module ((guix download)
                 #:select (download-to-store))
   #:use-module ((guix build utils)
-                #:select (substitute))
+                #:select (with-atomic-file-replacement))
+  #:use-module ((ice-9 rdelim)
+                #:select (read-line))
+  #:use-module ((rnrs io ports)
+                #:select (get-string-all))
   #:use-module (guix gnupg)
   #:use-module (guix packages)
   #:use-module (guix ui)
@@ -205,6 +209,37 @@ and 'interactive' (default)."
   "Modify the source file that defines PACKAGE to refer to VERSION,
 whose tarball has SHA256 HASH (a bytevector).  Return the new version string
 if an update was made, and #f otherwise."
+  (define (substitute+ file start end pattern+procs)
+    ;; Same as substitute, but within lines from START to END.
+    (let ((rx+proc  (map (match-lambda
+                        (((? regexp? pattern) . proc)
+                         (cons pattern proc))
+                        ((pattern . proc)
+                         (cons (make-regexp pattern regexp/extended)
+                               proc)))
+                       pattern+procs)))
+    (with-atomic-file-replacement file
+      (lambda (in out)
+        (while (< (port-line in) (- start 1))
+          (display (read-line in 'concat) out))
+
+        (let loop ((line (read-line in 'concat)))
+          (let ((line (fold (lambda (r+p line)
+                              (match r+p
+                                ((regexp . proc)
+                                 (match (list-matches regexp line)
+                                   ((and m+ (_ _ ...))
+                                    (proc line m+))
+                                   (_ line)))))
+                            line
+                            rx+proc)))
+            (display line out)
+            (if (< (port-line in) end)
+                (loop (read-line in 'concat))
+                (begin
+                  (display (get-string-all in) out)
+                  #t))))))))
+
   (define (new-line line matches replacement)
     ;; Iterate over MATCHES and return the modified line based on LINE.
     ;; Replace each match with REPLACEMENT.
@@ -222,24 +257,40 @@ if an update was made, and #f otherwise."
                       (substring line o (match:start m))
                       r))))))
 
-  (define (update-source file old-version version
+  (define (update-source file start end old-version version
                          old-hash hash)
-    ;; Update source file FILE, replacing occurrences OLD-VERSION by VERSION
-    ;; and occurrences of OLD-HASH by HASH (base32 representation thereof).
-
-    ;; TODO: Currently this is a bit of a sledgehammer: if VERSION occurs in
-    ;; different unrelated places, we may modify it more than needed, for
-    ;; instance.  We should try to make changes only within the sexp that
-    ;; corresponds to the definition of PACKAGE.
+    ;; Update source file FILE within lines from START to END,
+    ;; replacing occurrences OLD-VERSION by VERSION and occurrences
+    ;; of OLD-HASH by HASH (base32 representation thereof).
     (let ((old-hash (bytevector->nix-base32-string old-hash))
           (hash     (bytevector->nix-base32-string hash)))
-      (substitute file
-                  `((,(regexp-quote old-version)
-                     . ,(cut new-line <> <> version))
-                    (,(regexp-quote old-hash)
-                     . ,(cut new-line <> <> hash))))
+      (substitute+ file start end
+                   `((,(regexp-quote old-version)
+                      . ,(cut new-line <> <> version))
+                     (,(regexp-quote old-hash)
+                      . ,(cut new-line <> <> hash))))
       version))
 
+
+  (define (package-location-line-start package)
+    (location-line (package-location package)))
+
+  (define (package-location-line-end package)
+    (define (goto port line column)
+      (unless (and (= (port-column port)) (- column 1)
+                   (= (port-line port) (- line 1)))
+        (unless (eof-object? (read-char port))
+          (goto port line column))))
+
+    (match (package-location package)
+      (($ <location> file line column)
+       (call-with-input-file (search-path %load-path file)
+         (lambda (port)
+           (goto port line column)
+           (match (read port)
+             (('package _ ...)
+              (1+ (port-line port)))))))))
+
   (let ((name (package-name package))
         (loc  (package-field-location package 'version)))
     (if loc
@@ -249,6 +300,8 @@ if an update was made, and #f otherwise."
                                   (cut search-path %load-path <>))))
           (if file
               (update-source file
+                             (package-location-line-start package)
+                             (package-location-line-end package)
                              old-version version
                              old-hash hash)
               (begin
-- 
2.6.3

Re: [PATCH] gnu-maintenance: update-package-source: Only update the desired package.

[Andy Wingo]

Hi 宋文武 :)

On Tue 05 Apr 2016 06:35, 宋文武 <iyzsong@gmail.com> writes:

> diff --git a/guix/upstream.scm b/guix/upstream.scm
> index cea23fe..f4bc09c 100644
> --- a/guix/upstream.scm
> +++ b/guix/upstream.scm
> @@ -205,6 +209,37 @@ and 'interactive' (default)."
>    "Modify the source file that defines PACKAGE to refer to VERSION,
>  whose tarball has SHA256 HASH (a bytevector).  Return the new version string
>  if an update was made, and #f otherwise."
> +  (define (substitute+ file start end pattern+procs)

Sorry to tell you mixed signals, but did you consider Ludovic's
suggestion to use `package-field-location' ?

If it turns out that this more textual approach is best, consider
factoring the helper out to a top-level function.  Someone else is going
to want to use it later :)

Andy

Re: [PATCH] gnu-maintenance: update-package-source: Only update the desired package.

[Ludovic Courtès]

Andy Wingo <wingo@igalia.com> skribis:

> Hi 宋文武 :)
>
> On Tue 05 Apr 2016 06:35, 宋文武 <iyzsong@gmail.com> writes:
>
>> diff --git a/guix/upstream.scm b/guix/upstream.scm
>> index cea23fe..f4bc09c 100644
>> --- a/guix/upstream.scm
>> +++ b/guix/upstream.scm
>> @@ -205,6 +209,37 @@ and 'interactive' (default)."
>>    "Modify the source file that defines PACKAGE to refer to VERSION,
>>  whose tarball has SHA256 HASH (a bytevector).  Return the new version string
>>  if an update was made, and #f otherwise."
>> +  (define (substitute+ file start end pattern+procs)
>
> Sorry to tell you mixed signals, but did you consider Ludovic's
> suggestion to use `package-field-location' ?

I mentioned it mostly as a possible source of inspiration, but I think
it cannot be used as-is here.

The trick to determine the boundaries of the ‘package’ form looks
reasonable to me.

Thanks,
Ludo’.

Re: [PATCH] gnu-maintenance: update-package-source: Only update the desired package.

[Andy Wingo]

On Tue 05 Apr 2016 11:47, ludo@gnu.org (Ludovic Courtès) writes:

> Andy Wingo <wingo@igalia.com> skribis:
>
>> Hi 宋文武 :)
>>
>> On Tue 05 Apr 2016 06:35, 宋文武 <iyzsong@gmail.com> writes:
>>
>>> diff --git a/guix/upstream.scm b/guix/upstream.scm
>>> index cea23fe..f4bc09c 100644
>>> --- a/guix/upstream.scm
>>> +++ b/guix/upstream.scm
>>> @@ -205,6 +209,37 @@ and 'interactive' (default)."
>>>    "Modify the source file that defines PACKAGE to refer to VERSION,
>>>  whose tarball has SHA256 HASH (a bytevector).  Return the new version string
>>>  if an update was made, and #f otherwise."
>>> +  (define (substitute+ file start end pattern+procs)
>>
>> Sorry to tell you mixed signals, but did you consider Ludovic's
>> suggestion to use `package-field-location' ?
>
> I mentioned it mostly as a possible source of inspiration, but I think
> it cannot be used as-is here.

Why not?  Using that, you could limit the edit to the field and not the
whole package.  Anyway, just a thought.

> The trick to determine the boundaries of the ‘package’ form looks
> reasonable to me.

In that case I think the ideal is something like "edit-expression",
which takes a source location that starts an expression, uses "read" to
advance to the end of the expression, then edits the intervening string
using whatever but verifies that the result can still be read as one
expression.  Having recorded the absolute byte positions of the start
and end of the expression, you can then create the output by doing an
efficient bytevector read of the prologue, then display the string for
the edited expression, then the bytevector tail.

Again, just a thought!  Feel free to incorporate or not as you like,
宋文武 :)

Andy

Re: [PATCH] gnu-maintenance: update-package-source: Only update the desired package.

[宋文武]

Andy Wingo <wingo@igalia.com> writes:

> On Tue 05 Apr 2016 11:47, ludo@gnu.org (Ludovic Courtès) writes:
>
>> Andy Wingo <wingo@igalia.com> skribis:
>> [...]
>>>
>>> Sorry to tell you mixed signals, but did you consider Ludovic's
>>> suggestion to use `package-field-location' ?
>>
>> I mentioned it mostly as a possible source of inspiration, but I think
>> it cannot be used as-is here.
>
> Why not?  Using that, you could limit the edit to the field and not the
> whole package.  Anyway, just a thought.
>
>> The trick to determine the boundaries of the ‘package’ form looks
>> reasonable to me.
Yes, I copy the `goto' function from `package-field-location' and
happily notice that after do a `read' it's at the end location of the
object.  Without more thinking :-)


> In that case I think the ideal is something like "edit-expression",
> which takes a source location that starts an expression, uses "read" to
> advance to the end of the expression, then edits the intervening string
> using whatever but verifies that the result can still be read as one
> expression.  Having recorded the absolute byte positions of the start
> and end of the expression, you can then create the output by doing an
> efficient bytevector read of the prologue, then display the string for
> the edited expression, then the bytevector tail.
>
> Again, just a thought!  Feel free to incorporate or not as you like,
> 宋文武 :)
Indeed this is better!  To create the output from bytevector, it's
using `get-bytevector-n' to read and `utf8->string' to write, right?

And I think `edit-expression' could be:
--8<---------------cut here---------------start------------->8---
(define (edit-expression source-properties proc)
  "Edit the object specified by SOURCE-PROPERTIS using PROC, which
should be a procedure that take the original object and returns
a new one."
  ...)
--8<---------------cut here---------------end--------------->8---

I'll try it :-)

Re: [PATCH] gnu-maintenance: update-package-source: Only update the desired package.

[Andy Wingo]

Hi :)

On Tue 05 Apr 2016 16:05, iyzsong@member.fsf.org (宋文武) writes:

> Andy Wingo <wingo@igalia.com> writes:
>
>> In that case I think the ideal is something like "edit-expression",
>> which takes a source location that starts an expression, uses "read" to
>> advance to the end of the expression, then edits the intervening string
>> using whatever but verifies that the result can still be read as one
>> expression.  Having recorded the absolute byte positions of the start
>> and end of the expression, you can then create the output by doing an
>> efficient bytevector read of the prologue, then display the string for
>> the edited expression, then the bytevector tail.
>>
>> Again, just a thought!  Feel free to incorporate or not as you like,
>> 宋文武 :)
> Indeed this is better!  To create the output from bytevector, it's
> using `get-bytevector-n' to read and `utf8->string' to write, right?

Hmm, good question.  I think I would use put-bytevector, if I read using
get-bytevector-n, assuming that the encoding of the input and the output
ports were the same.  Probably it is worth making sure that you set the
encodings to be the same, but I don't know :)  Hopefully everything is
UTF-8 these days.

> And I think `edit-expression' could be:
> (define (edit-expression source-properties proc)
>   "Edit the object specified by SOURCE-PROPERTIS using PROC, which
> should be a procedure that take the original object and returns
> a new one."
>   ...)

Yeah :)  However probably the "proc" should take and return a string, so
as to preserve formatting and comments.

Good luck :)

Andy

Re: [PATCH] gnu-maintenance: update-package-source: Only update the desired package.

[Ludovic Courtès]

Andy Wingo <wingo@igalia.com> skribis:

> In that case I think the ideal is something like "edit-expression",
> which takes a source location that starts an expression, uses "read" to
> advance to the end of the expression, then edits the intervening string
> using whatever but verifies that the result can still be read as one
> expression.  Having recorded the absolute byte positions of the start
> and end of the expression, you can then create the output by doing an
> efficient bytevector read of the prologue, then display the string for
> the edited expression, then the bytevector tail.

Indeed, that sounds like the right abstraction!

Thanks,
Ludo’.

Re: [PATCH] gnu-maintenance: update-package-source: Only update the desired package.

[Ludovic Courtès]

宋文武 <iyzsong@gmail.com> skribis:

> Fixes <http://bugs.gnu.org/22693>.
> Suggested by Andy Wingo.
>
> * guix/upstream.scm (update-package-source): Use a customized 'substitute'
> to work within lines of the package's source.

[...]

> +  (define (substitute+ file start end pattern+procs)
> +    ;; Same as substitute, but within lines from START to END.

Please make it a top-level procedure, for clarity.

> +  (define (package-location-line-start package)
> +    (location-line (package-location package)))
> +
> +  (define (package-location-line-end package)
> +    (define (goto port line column)
> +      (unless (and (= (port-column port)) (- column 1)
> +                   (= (port-line port) (- line 1)))
> +        (unless (eof-object? (read-char port))
> +          (goto port line column))))
> +
> +    (match (package-location package)
> +      (($ <location> file line column)
> +       (call-with-input-file (search-path %load-path file)
> +         (lambda (port)
> +           (goto port line column)
> +           (match (read port)
> +             (('package _ ...)
> +              (1+ (port-line port)))))))))

I think you should add a ‘match’ case here, for when (read port) returns
something that is not a ‘package’ form (that could happen.)

The docstring should specify that this can return #f, and callers should
fall back to a wild guess, like:

  (or (package-location-end-line p)
      (+ (package-location-start-line p) 30))

But overall the approach looks good to me!

Could you:

  1. Call these two procedure ‘package-location-start-line’ and
     ‘package-location-end-line’ (I think it sounds better from a
     grammatical standpoint ;-))?

  2. Move them to (guix packages), next to ‘package-field-location’?

  3. Add tests in tests/packages.scm?

The (guix upstream) changes would come in a separate patch.

WDYT?

Thank you!

Ludo’.