bug#36375: [PATCH] Re: ‘guix package’ should lock the profile

[Julien Lepiller]

Le Wed, 06 Nov 2019 14:24:54 +0100,
Ludovic Courtès <address@hidden> a écrit :

> Hello!
> 
> Julien Lepiller <address@hidden> skribis:
> 
> >>From 5d86226f318a111cc1bdf5a6f044c6f540f51b45 Mon Sep 17 00:00:00
> >>2001  
> > From: Julien Lepiller <address@hidden>
> > Date: Fri, 25 Oct 2019 21:39:21 +0200
> > Subject: [PATCH] guix: package: lock profiles when processing them.
> >
> > * guix/scripts/package.scm (process-actions): Get a per-profile
> > lock to prevent concurrent actions on profiles.
> > * guix/build/syscalls.scm (with-file-lock/no-wait): New procedure.
> > (lock-file): Take a #:wait? key.  
> 
> Nice!  Could you make the syscalls.scm changes a separate patch?
> 
> > +(define (call-with-file-lock/no-wait file thunk handler)
> > +  (let ((port (catch 'system-error
> > +                (lambda ()
> > +             (catch 'flock-error
> > +                    (lambda ()
> > +                 (lock-file file #:wait? #f))
> > +               handler))
> > +                (lambda args
> > +                  ;; When using the statically-linked Guile in the
> > initrd,
> > +                  ;; 'fcntl-flock' returns ENOSYS
> > unconditionally.  Ignore
> > +                  ;; that error since we're typically the only
> > process running
> > +                  ;; at this point.
> > +                  (if (or (= ENOSYS (system-error-errno args)) (=
> > 'flock-error args))  
> 
> Please remove tabs.  :-)
> 
> This is wrong because (1) ‘args’ is always a list, and (2) ‘=’ is
> defined for numbers, not for symbols and lists.
> 
> I think you actually want to catch two exceptions here: ‘system-error’
> and ‘flock-error’.  For that, you have to do:
> 
>   (catch #t
>     (lambda ()
>       (lock-file …))
>     (lambda (key . args)
>       (match key
>         ('flock-error …)
>         ('system-error
>          (if (= ENOSYS (system-error-errno (cons key args)))
>              …))
>         (_
>          (apply throw key args)))))
>         
> Does that make sense?
> 
> > +  ;; First, acquire a lock on the profile, to ensure only one guix
> > process
> > +  ;; is modifying it at a time.
> > +  (with-file-lock/no-wait
> > +    (string-append profile ".lock")  
> 
> Nitpick: I’d move the lock file name on the same line as
> ‘with-file-lock/no-wait’.
> 
> > +    (lambda (key . args)
> > +      (leave (G_ "profile ~a is locked by another guix process.~%")
> > +                 profile))  
> 
> s/guix// and remove the trailing period.
> 
> Could you add a test for that in tests/guix-package.sh?
> 
> One way to do it may be to do something like:
> 
>   echo '(sleep 60) > /…/manifest.scm
>   guix package -m /…/manifest.scm -p whatever &
>   pid=$!
>   if guix install emacs -p whatever; then false; else true; fi
>   kill $pid
> 
> Could you send updated patches?
> 
> Thanks!
> 
> Ludo’.

Attached are updated patches! I also made sure the new test passes.

0001-guix-Add-file-locking-with-no-wait.patch
Description: Text Data

0002-guix-package-lock-profiles-when-processing-them.patch
Description: Text Data