Marius Bakke <mbakke@fastmail.com> writes:

> Christopher Baines <mail@cbaines.net> writes:
>
>> One even more niche issue is that because I'm using this channel in my
>> system configuration, the patching happens as root, but it's the cached
>> channel in my users home directory that's patched. This means that
>> build-self.scm becomes owned by root.
>>
>> I noticed this when I went to pull:
>>
>>   → guix pull --branch=core-updates
>>   Updating channel 'guix' from Git repository at 'https://git.savannah.gnu.org/git/guix.git'...
>>   guix pull: error: Git error: could not open '/home/chris/.cache/guix/checkouts/pjmkglp4t7znuugeurpurzikxq3tnlaywmisyr27shj7apsnalwq/build-aux/build-self.scm' for writing: Permission denied
>>
>>
>> I'm not sure what the neat way of addressing this is, but maybe the file
>> ownership can be recorded prior to patching, and reset afterwards if
>> it's changed.
>
> I took a stab at exactly this:
>
> From 993dd0d36ba8e67af5c60d73cb1f9d60741f5418 Mon Sep 17 00:00:00 2001
> From: Marius Bakke <mbakke@fastmail.com>
> Date: Fri, 8 May 2020 16:23:55 +0200
> Subject: [PATCH] channels: Preserve permissions when patching
>  <https://bugs.gnu.org/41028>.
>
> * guix/channels.scm (%bug-41028-patch): Record permissions before invoking
> SUBSTITUTE* and reset afterwards if file permissions differ from the current user.
> ---
>  guix/channels.scm | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/guix/channels.scm b/guix/channels.scm
> index 0fa036446c..a102d5bc35 100644
> --- a/guix/channels.scm
> +++ b/guix/channels.scm
> @@ -393,10 +393,21 @@ to '%package-module-path'."
>             (not (string-contains content "(ice-9 threads)"))))
>  
>      (define (add-missing-ice-9-threads-import source)
> -      ;; Add (ice-9 threads) import in the gexp of 'compute-guix-derivation'.
> -      (substitute* (string-append source "/" %self-build-file)
> -        (("^ +\\(use-modules \\(ice-9 match\\)\\)")
> -         (object->string '(use-modules (ice-9 match) (ice-9 threads))))))
> +      (let* ((self-build-file (string-append source "/" %self-build-file))
> +             ;; Record permissions so that we can reset it afterwards in case
> +             ;; we run this as the root user (see <https://bugs.gnu.org/41028#29>).
> +             ;; TODO: Ideally SUBSTITUTE* would preserve permissions itself.
> +             (stat  (stat self-build-file))
> +             (owner (stat:uid stat))
> +             (group (stat:gid stat)))
> +
> +        ;; Add (ice-9 threads) import in the gexp of 'compute-guix-derivation'.
> +        (substitute* self-build-file
> +          (("^ +\\(use-modules \\(ice-9 match\\)\\)")
> +           (object->string '(use-modules (ice-9 match) (ice-9 threads)))))
> +
> +        (unless (and (eq? (getuid) owner) (eq? (getgid) group))
> +          (chown self-build-file owner group))))
>  
>     (patch missing-ice-9-threads-import? add-missing-ice-9-threads-import)))

Ludovic pointed out on IRC that 'sudo guix system build foo.scm' with an
inferior will update the cached checkout (typically
~/.cache/guix/checkouts/pjmkglp4t7znuugeurpurzikxq3tnlaywmisyr27shj7apsnalwq)
as root, and break subsequent unprivileged 'guix pull' operations anyway.

Which is partially correct: .git/index and .git/refs/heads/master will
be owned by root in that case, but you are still allowed to unlink() and
rename() such files as long as you own the directory, so libgit2 will
successfully reset permissions on the checkout even after pulling as
root!

It fails to do that for build-self.scm because it does not know it has
changed, and tries opening it directly with O_WRONLY (which fails).  I
can confirm that this patch solves that problem, and unprivileged 'guix
pull' works again even after previously using sudo.

Now, users are likely to run into other issues when using inferiors with
sudo (e.g. if new directories are added), so I'm not sure if this patch
is worth the effort.

Perhaps the Guix git cache should be per-uid, but that requires more
intrusive changes.

Thoughts?