Ensure files are writable after copy to chroot.

[phaer]

Bootstrapping vendored dependencies seems to require files copied into the chroot to be writable, that however seems only to be true if the src directory is writable as well.

So this change intends to ensure files copied to the chroot are always writable by the owner, while leaving other permissions intact.

It seems to fix the issue described in NixOS/nixpkgs#246448 (comment)

What do you think, @jsirois?

[adisbladis]

I can confirm this solves running pex3 on NixOS for me.

[jsirois]

Thanks for taking a crack at this @phaer. Clearly this must do the trick but I want to dig a bit to see what is really going on. After all:

$ tree -p foo
[dr-xr-xr-x]  foo
└── [-r--r--r--]  bar

0 directories, 1 file
$ cp -r foo/ foo-dest
$ tree -p foo-dest/
[dr-xr-xr-x]  foo-dest/
└── [-r--r--r--]  bar

0 directories, 1 file
$ cp foo/bar foo-dest/
cp: cannot create regular file 'foo-dest/bar': Permission denied
$

So the issue can't be that the source files are read only, it must be that the destination file already exists and is read only; i.e. Pex is performing the copy operation for a given target file more than once IIUC and the second+ copy fails. Pex operations should generally be atomic and non-dup - even in the face of multiprocess parallelism; so this is concerning if I have this assesment about right.

I'll investigate a bit and report back.

[jsirois]

Yeah, this is a dup copy:

jsirois@Gill-Windows:~/support/pex/phaer $ diff -u /nix/store/hsjy0mbj4j7kyd0ycsrfadlp6r367inf-python3.10-pex-2.1.137/lib/python3.10/site-packages/pex/vendor/__init__.py.orig /nix/store/hsjy0mbj4j7kyd0ycsrfadlp6r367inf-python3.10-pex-2.1.137/lib/python3.10/site-packages/pex/vendor/__init__.py
--- /nix/store/hsjy0mbj4j7kyd0ycsrfadlp6r367inf-python3.10-pex-2.1.137/lib/python3.10/site-packages/pex/vendor/__init__.py.orig 2023-08-03 11:35:57.944502359 -0600
+++ /nix/store/hsjy0mbj4j7kyd0ycsrfadlp6r367inf-python3.10-pex-2.1.137/lib/python3.10/site-packages/pex/vendor/__init__.py      2023-08-03 11:39:12.244492441 -0600
@@ -290,6 +290,7 @@
                     src = os.path.join(VendorSpec.ROOT, pkg_file)
                     dest = os.path.join(dest_basedir, pkg_file)
                     if os.path.exists(src):
+                        print(f">>> [{spec}@{label}] {src} -> {dest}", file=sys.stderr)
                         chroot.copy(src, dest, label)
                     else:
                         # We delete `pex/vendor/_vendored/<dist>/__init__.py` when isolating
jsirois@Gill-Windows:~/support/pex/phaer $ pex
>>> [VendorSpec(key='attrs', requirement='git+https://github.com/python-attrs/attrs@947bfb542104209a587280701d8cb389c813459d#egg=attrs', import_path='attrs', rewrite=True, constrain=False, constraints=())@bootstrap] /nix/store/hsjy0mbj4j7kyd0ycsrfadlp6r367inf-python3.10-pex-2.1.137/lib/python3.10/site-packages/pex/__init__.py -> .bootstrap/pex/__init__.py
>>> [VendorSpec(key='attrs', requirement='git+https://github.com/python-attrs/attrs@947bfb542104209a587280701d8cb389c813459d#egg=attrs', import_path='attrs', rewrite=True, constrain=False, constraints=())@bootstrap] /nix/store/hsjy0mbj4j7kyd0ycsrfadlp6r367inf-python3.10-pex-2.1.137/lib/python3.10/site-packages/pex/vendor/__init__.py -> .bootstrap/pex/vendor/__init__.py
>>> [VendorSpec(key='attrs', requirement='git+https://github.com/python-attrs/attrs@947bfb542104209a587280701d8cb389c813459d#egg=attrs', import_path='attrs', rewrite=True, constrain=False, constraints=())@bootstrap] /nix/store/hsjy0mbj4j7kyd0ycsrfadlp6r367inf-python3.10-pex-2.1.137/lib/python3.10/site-packages/pex/vendor/_vendored/__init__.py -> .bootstrap/pex/vendor/_vendored/__init__.py
>>> [VendorSpec(key='attrs', requirement='git+https://github.com/python-attrs/attrs@947bfb542104209a587280701d8cb389c813459d#egg=attrs', import_path='attrs', rewrite=True, constrain=False, constraints=())@bootstrap] /nix/store/hsjy0mbj4j7kyd0ycsrfadlp6r367inf-python3.10-pex-2.1.137/lib/python3.10/site-packages/pex/vendor/_vendored/attrs/__init__.py -> .bootstrap/pex/vendor/_vendored/attrs/__init__.py
>>> [VendorSpec(key='packaging', requirement='packaging==20.9', import_path='packaging_20_9', rewrite=True, constrain=True, constraints=('pyparsing<3',))@bootstrap] /nix/store/hsjy0mbj4j7kyd0ycsrfadlp6r367inf-python3.10-pex-2.1.137/lib/python3.10/site-packages/pex/__init__.py -> .bootstrap/pex/__init__.py
[Errno 13] Permission denied: '/tmp/tmp944zx8hs/.bootstrap/pex/__init__.py'

So a better fix is to fix that. Pex should not be over-writing files willy-nilly. In other places it actually checks that a file hash matches before overwriting or else warns or errors depending on settings. Behaving like that here would be good, but even better, since this is Pex's own files its dealing with here, just avoiding the dup is proably the way to go. I'll spin a it more and see what I can come up with and report back.

[jsirois]

Ok, bug tracked here: #2203
It's very easy to repro without nix as done in that ticket example and using a similar technique in a test will form a nice basis for ensuring this buggy behavior doesn't backslide going forward. I'll take up that ticket presently and see if I can't make the underlying .bootstrap preparation more robust to being repetitive / over-writing itself.

[jsirois]

@phaer I've put up an alternate approach here: #2205
If you and / or @adisbladis could give it a look and a spin, I'd be grateful.

[phaer]

@jsirois Thanks for checking and the quick patch! Can confirm that it works with nix :)

Definitely the better fix, glad your fix uncovered a deeper issue.

Nitpick regarding the "cp" test: I think that's not entirely accurate as shutil.copy does copy the files mode, so it's more comparable to cp -r iiuc.

[jsirois]

@phaer excellent.

On the nit, I'm a bit confused. Modern cp on Linux (my case) does preserve perms by default (no need for -p, although you mention -r, which I used, presumably as a typo). Further, my example runs tree -p after the cp to prove the new tree has the same read only perms.

[phaer]

@jsirois You are right, I misread - sorry for the noise. I also overlooked that the files don't need to be writable at all as long as there's no duplicate copy, just saw that your new tests cover that - perfect :)