Re: guix-package --roll-back

[Ludovic Courtès]

Hi,

Nikita Karetnikov <address@hidden> skribis:

> I'm attaching a slightly modified version.

Thanks!

>> Yeah, that’s expected.  Basically, if you do
>
>>   guix-package -p /dev/null --roll-back
>
>> it should fail with an error message saying that there is no previous
>> profile or something like that.
>
> 'profile-number' will fail if I call it with a bogus file name.
>
> For instance:
>
> scheme@(guile-user)> (profile-number "/foo/bar")
> ERROR: In procedure readlink:
> ERROR: In procedure readlink: No such file or directory
>
> What should I use to handle this error?  Please show an example.  The
> ones from the manual aren't helpful.

At the command-line level, an example would be:

  $ guix-package -p /dev/null --roll-back ; echo $?
  error: no previous profile to roll back to
  1

(Please, add this example to tests/guix-package.sh.)

Internally, I would expect this ‘profile-number’ to return #f.

> I don't understand how to add a command-line option that should accept
> an optional argument.  I commented out my attempts.

  (option '("foo")
          #f  ; no required argument
          #t  ; an optional argument
          (lambda (opt name arg result)
            ;; Function called when ‘--foo’ is passed.
            ;; ARG is #f or the optional argument.
            ;; ...
            ))

See SRFI-37 in Guile’s manual.

> +(define (profile-rx profile)

Perhaps ‘profile-regexp’ (since it’s a global variable.)

> +(define (profile-number profile)
> +  "Return PROFILE's number.  An absolute file name must be used."

I think it works even if PROFILE is not an absolute file name, no?

Just like ‘latest-profile-number’ returns 0 if there was no file
matching %PROFILE-RX, ‘profile-number’ should return 0.

So you’d want to enclose the whole body like this:

  (or (and=> ...) 0)

> +  (and=> (regexp-exec (profile-rx profile)
> +                      (basename (readlink profile)))
> +         (cut match:substring <> 1)))

First, you need to move ‘readlink’ above, in ‘false-if-exception’.

Second, it should return a number, not a string.

So the result should be along these lines:

  (let ((target (false-if-exception (readlink profile))))
    (and target
         (and=> (regexp-exec (profile-regexp profile) (basename target))
                (compose string->number (cut match:substring <> 1))

> +(define* (roll-back #:optional profile)

Make ‘profile’ mandatory.

> +  "Roll back to the previous profile."

Rather: “Roll back to the previous generation of PROFILE.”

> +  (let* ((current-profile-number
> +          (string->number (profile-number (or profile %current-profile))))

The ‘or’ and ‘string->number’ can now be removed.
Name the variable just ‘number’, because long names for local vars
hinder clarity.

Now you need to introduce an

  (if number
      ;; then do the thing
      (format (current-error-port) (_ "error: ‘~a’ is not a valid profile~%")
              profile))

> +         (previous-profile-number (number->string (1- 
> current-profile-number)))
> +         (previous-profile
> +          (string-append (or profile %current-profile) "-"
> +                         previous-profile-number "-link"))
> +         (manifest (string-append previous-profile "/manifest")))
> +
> +    (define (switch-link)
> +      ;; Switch to the previous generation.
> +      (let ((tmp-profile (string-append (dirname (or profile 
> %current-profile))
> +                                        "/tmp-"
> +                                        (basename previous-profile))))
> +
> +        (simple-format #t "guix-package: switching from generation ~a to 
> ~a~%"
> +                       current-profile-number previous-profile-number)
> +        (symlink previous-profile tmp-profile)
> +        (rename-file tmp-profile (or profile %current-profile))))

Looks good.
However, just use ‘format’, and also make sure that messages and i18n’d:

  (format #t (_ "switching from generation ~a to ~a...~%")
          number previous-number)

> +    (if (equal? (map (cut file-exists? <>)
> +                     (list previous-profile manifest))
> +                '(#t #t))

I think you just need to check for ‘previous-profile’.  If it turns out
not to be a genuine profile, that’s not our problem.

Besides, this is a mundane (and inefficient) way of writing:

  (every file-exists? (list previous-profile manifest))

or simply:

  (and (file-exists? previous-profile)
       (file-exists? manifest))

:-)

> +        (switch-link)
> +        (leave (_ (string-append
> +                   "guix-package: previous profile doesn't exist; "
> +                   "not rolling back~%"))))))

Strip “guix-package: ”.

Woow, this was long.

Thanks,
Ludo’.