pip_compile: read-only requirements.txt breaks implicit py_test

[rbeasley-avgo]

🐞 bug report

Affected Rule

The issue is caused by the rule: compile_pip_requirements / pip_compile

Is this a regression?

Yes, the previous version in which this bug was not present was: 0.3.0. (We're in the middle of quite an upgrade!)

Description

https://github.com/bazelbuild/rules_python/blob/main/python/private/pypi/dependency_resolver/dependency_resolver.py#L151-L153

The if "TEST_TMPDIR" in os.environ branch takes care to create a temporary output file which may be modified later. However, it creates the temporary file using shutil.copyfile, which preserves the source file's permissions. If the source file is read-only (such as when managed by Perforce Helix Core), then this temporary file will also be read-only, causing the generated py_test target to fail.

🔬 Minimal Reproduction

$ cat >MODULE.bazel <<"EOF"
bazel_dep(name = "rules_python", version = "1.1.0")

pip = use_extension("@rules_python//python/extensions:pip.bzl", "pip")

pip.parse(
    hub_name = "pip",
    python_version = "3.11",
    requirements_lock = "//:requirements_lock.txt",
)

use_repo(pip, "pip")
EOF
$ cat > BUILD.bazel <<"EOF"
load("@rules_python//python:pip.bzl", "compile_pip_requirements")

compile_pip_requirements(
    name = "requirements",
    requirements_in = "requirements.in",
)
EOF
$ cat > requirements.in <<"EOF"
six==1.17.0
EOF
$ touch requirements.txt
$ bazelisk run :requirements.update  # populates requirements.txt
$ chmod 444 requirements.*  # make src, lock file read-only
$ bazelisk test :requirements_test

🔥 Exception or Error

Checking _main/requirements.txt
Traceback (most recent call last):
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/execroot/_main/bazel-out/k8-fastbuild/bin/requirements_test.runfiles/_main/../rules_python+/python/private/pypi/dependency_resolver/dependency_resolver.py", line 254, in 
    main()
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/execroot/_main/bazel-out/k8-fastbuild/bin/requirements_test.runfiles/rules_python++internal_deps+pypi__click/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/execroot/_main/bazel-out/k8-fastbuild/bin/requirements_test.runfiles/rules_python++internal_deps+pypi__click/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/execroot/_main/bazel-out/k8-fastbuild/bin/requirements_test.runfiles/rules_python++internal_deps+pypi__click/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/execroot/_main/bazel-out/k8-fastbuild/bin/requirements_test.runfiles/rules_python++internal_deps+pypi__click/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/execroot/_main/bazel-out/k8-fastbuild/bin/requirements_test.runfiles/_main/../rules_python+/python/private/pypi/dependency_resolver/dependency_resolver.py", line 208, in main
    cli(argv)
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/execroot/_main/bazel-out/k8-fastbuild/bin/requirements_test.runfiles/rules_python++internal_deps+pypi__click/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/execroot/_main/bazel-out/k8-fastbuild/bin/requirements_test.runfiles/rules_python++internal_deps+pypi__click/click/core.py", line 1077, in main
    with self.make_context(prog_name, args, **extra) as ctx:
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/execroot/_main/bazel-out/k8-fastbuild/bin/requirements_test.runfiles/rules_python++internal_deps+pypi__click/click/core.py", line 466, in __exit__
    self.close()
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/execroot/_main/bazel-out/k8-fastbuild/bin/requirements_test.runfiles/rules_python++internal_deps+pypi__click/click/core.py", line 595, in close
    self._exit_stack.close()
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/external/rules_python++python+python_3_11_x86_64-unknown-linux-gnu/lib/python3.11/contextlib.py", line 609, in close
    self.__exit__(None, None, None)
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/external/rules_python++python+python_3_11_x86_64-unknown-linux-gnu/lib/python3.11/contextlib.py", line 601, in __exit__
    raise exc_details[1]
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/external/rules_python++python+python_3_11_x86_64-unknown-linux-gnu/lib/python3.11/contextlib.py", line 586, in __exit__
    if cb(*exc_details):
       ^^^^^^^^^^^^^^^^
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/external/rules_python++python+python_3_11_x86_64-unknown-linux-gnu/lib/python3.11/contextlib.py", line 469, in _exit_wrapper
    callback(*args, **kwds)
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/execroot/_main/bazel-out/k8-fastbuild/bin/requirements_test.runfiles/rules_python++internal_deps+pypi__click/click/utils.py", line 176, in close_intelligently
    self.close()
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/execroot/_main/bazel-out/k8-fastbuild/bin/requirements_test.runfiles/rules_python++internal_deps+pypi__click/click/utils.py", line 169, in close
    self._f.close()
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/execroot/_main/bazel-out/k8-fastbuild/bin/requirements_test.runfiles/rules_python++internal_deps+pypi__click/click/_compat.py", line 469, in close
    os.replace(self._tmp_filename, self._real_filename)
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/external/rules_python++python+python_3_11_x86_64-unknown-linux-gnu/lib/python3.11/shutil.py", line 431, in copy
    copyfile(src, dst, follow_symlinks=follow_symlinks)
  File "$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/external/rules_python++python+python_3_11_x86_64-unknown-linux-gnu/lib/python3.11/shutil.py", line 258, in copyfile
    with open(dst, 'wb') as fdst:
         ^^^^^^^^^^^^^^^
PermissionError: [Errno 13] Permission denied: '$XDG_CACHE_HOME/bazel/99445a8dcf5a9088215729f61e4271eb/execroot/_main/_tmp/24e3066da9cc133a367098d5abc917e6/requirements.txt.out'

🌍 Your Environment

Operating System:

  
Rocky 9
  

Output of bazel version:

  
Build label: 8.0.1
Build target: @@//src/main/java/com/google/devtools/build/lib/bazel:BazelServer
Build time: Fri Jan 17 19:16:16 2025 (1737141376)
Build timestamp: 1737141376
Build timestamp as int: 1737141376
  

Rules_python version:

  
1.1.0
  

Anything else relevant?

I'm using the following as a workaround. Am happy to contribute a PR once the CLA is sorted.

diff --git a/python/private/pypi/dependency_resolver/dependency_resolver.py b/python/private/pypi/dependency_resolver/dependency_resolver.py
index 0ff9b2fb..8def79e7 100644
--- a/python/private/pypi/dependency_resolver/dependency_resolver.py
+++ b/python/private/pypi/dependency_resolver/dependency_resolver.py
@@ -148,9 +148,16 @@ def main(
         requirements_out = os.path.join(
             os.environ["TEST_TMPDIR"], os.path.basename(requirements_file) + ".out"
         )
+        # Why this uses shutil.copyfileobj:
+        #
         # Those two files won't necessarily be on the same filesystem, so we can't use os.replace
         # or shutil.copyfile, as they will fail with OSError: [Errno 18] Invalid cross-device link.
-        shutil.copy(resolved_requirements_file, requirements_out)
+        #
+        # Further, shutil.copy preserves the source file's mode, and so if
+        # our source file is read-only (the default under Perforce Helix),
+        # this scratch file will also be read-only, defeating its purpose.
+        with open(resolved_requirements_file, "rb") as fsrc, open(requirements_out, "wb") as fdst:
+            shutil.copyfileobj(fsrc, fdst)

     update_command = os.getenv("CUSTOM_COMPILE_COMMAND") or "bazel run %s" % (
         update_target_label,

If there's a compelling reason to use shutil.copy, then the following could work instead:

diff --git a/python/private/pypi/dependency_resolver/dependency_resolver.py b/python/private/pypi/dependency_resolver/dependency_resolver.py
index 0ff9b2fb..483671c4 100644
--- a/python/private/pypi/dependency_resolver/dependency_resolver.py
+++ b/python/private/pypi/dependency_resolver/dependency_resolver.py
@@ -18,6 +18,7 @@ import atexit
 import os
 import shutil
 import sys
+import tempfile
 from pathlib import Path
 from typing import Optional, Tuple

@@ -151,6 +152,11 @@ def main(
         # Those two files won't necessarily be on the same filesystem, so we can't use os.replace
         # or shutil.copyfile, as they will fail with OSError: [Errno 18] Invalid cross-device link.
         shutil.copy(resolved_requirements_file, requirements_out)
+        # shutil.copy preserves the original file's mode, and so if our
+        # original file is read-only (the default under Perforce Helix),
+        # this scratch file will also be read-only, defeating its purpose.
+        with tempfile.NamedTemporaryFile() as tmpf:
+            shutil.copymode(tmpf.name, requirements_out)

     update_command = os.getenv("CUSTOM_COMPILE_COMMAND") or "bazel run %s" % (
         update_target_label,

[aignas]

This part of the code is fraught with complexity and hard to handle corner cases.

My current thoughts/asks:

1. Could you please try with main to check that the issue is still present? I think it is as the part that you are talking about has not changed, but other things were reverted.
2. Having RBE and other setups still working with your changes might be challenging. I think we have enough coverage for those, so if the PR CI is happy, that might be good enough.
3. I would like to move to a different model where we use uv and run the locking in a build action and this problem would not be there.

[rbeasley-avgo]

Thanks for the followup.

1. Could you please try with main to check that the issue is still present? I think it is as the part that you are talking about has not changed, but other things were reverted.

Sure, no problem.

This issue reproduces on main;

commit edfb4b34de1c2602f8ae5c8d402384c8e36a03cd (HEAD -> main, origin/main)
Author: Ivo List <ilist@google.com>
Date:   Tue Feb 11 23:28:37 2025 +0100

2. Having RBE and other setups still working with your changes might be challenging. I think we have enough coverage for those, so if the PR CI is happy, that might be good enough.

Cool, that's totally fair. I'll whip up a PR (though CLA is still in-progress). Post-merge, if we discover any regressions and my PR is reverted, then I suppose I can come back with more test cases in my second attempt.

[aignas]

Thank you for willing to go all the way, looking forward to your PR.