fix: checked-in requirements imports generated requirements (#1053)

* fix: checked-in requirements imports generated requirements Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: shutil.copy instead of shutil.copyfile This allows copying from one filesystem to another, as the `os.rename` (used by copyfile) doesn't work with multiple filesystems. Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: patch os.replace to use shutil.copy Same as the previous commit. Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: runfiles.Rlocation requires paths to be normalized Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: drop rules_python from import This is not compatible with bzlmod. Importing python.runfiles works for both ways. Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: remove unnecessary runfiles Signed-off-by: Thulio Ferraz Assis <[email protected]> * doc: why os.replace = shutil.copy Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: allow the test to still be remote cacheable Signed-off-by: Thulio Ferraz Assis <[email protected]> * doc: why shutil.copy Signed-off-by: Thulio Ferraz Assis <[email protected]> * doc: add missing punctuation Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: remove unnecessary _fix_up_requirements_in_path Signed-off-by: Thulio Ferraz Assis <[email protected]> * test: make sure the locked requirements is updated Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: copy requirements back into src tree if needed Signed-off-by: Thulio Ferraz Assis <[email protected]> * fix: make sure windows uses forward slashes Signed-off-by: Thulio Ferraz Assis <[email protected]> --------- Signed-off-by: Thulio Ferraz Assis <[email protected]>
bazelbuild · Feb 13, 2023 · 767b050 · 767b050
1 parent 0051393
commit 767b050
Show file tree

Hide file tree

Showing 6 changed files with 76 additions and 55 deletions.
diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml
@@ -289,21 +289,49 @@ tasks:
     name: compile_pip_requirements integration tests on Ubuntu
     working_directory: tests/compile_pip_requirements
     platform: ubuntu2004
+    shell_commands:
+    # Make a change to the locked requirements and then assert that //:requirements.update does the
+    # right thing.
+    - "echo '' > requirements_lock.txt"
+    - "! git diff --exit-code"
+    - "bazel run //:requirements.update"
+    - "git diff --exit-code"
   integration_test_compile_pip_requirements_debian:
     <<: *reusable_build_test_all
     name: compile_pip_requirements integration tests on Debian
     working_directory: tests/compile_pip_requirements
     platform: debian11
+    shell_commands:
+    # Make a change to the locked requirements and then assert that //:requirements.update does the
+    # right thing.
+    - "echo '' > requirements_lock.txt"
+    - "! git diff --exit-code"
+    - "bazel run //:requirements.update"
+    - "git diff --exit-code"
   integration_test_compile_pip_requirements_macos:
     <<: *reusable_build_test_all
     name: compile_pip_requirements integration tests on macOS
     working_directory: tests/compile_pip_requirements
     platform: macos
+    shell_commands:
+    # Make a change to the locked requirements and then assert that //:requirements.update does the
+    # right thing.
+    - "echo '' > requirements_lock.txt"
+    - "! git diff --exit-code"
+    - "bazel run //:requirements.update"
+    - "git diff --exit-code"
   integration_test_compile_pip_requirements_windows:
     <<: *reusable_build_test_all
     name: compile_pip_requirements integration tests on Windows
     working_directory: tests/compile_pip_requirements
     platform: windows
+    shell_commands:
+    # Make a change to the locked requirements and then assert that //:requirements.update does the
+    # right thing.
+    - "echo '' > requirements_lock.txt"
+    - "! git diff --exit-code"
+    - "bazel run //:requirements.update"
+    - "git diff --exit-code"
 
   integration_test_pip_repository_entry_points_ubuntu:
     <<: *reusable_build_test_all

diff --git a/python/pip_install/requirements.bzl b/python/pip_install/requirements.bzl
@@ -103,6 +103,8 @@ def compile_pip_requirements(
 
     tags = tags or []
     tags.append("requires-network")
+    tags.append("no-remote-exec")
+    tags.append("no-sandbox")
     attrs = {
         "args": args,
         "data": data,

diff --git a/python/pip_install/tools/dependency_resolver/dependency_resolver.py b/python/pip_install/tools/dependency_resolver/dependency_resolver.py
@@ -14,14 +14,39 @@
 
 "Set defaults for the pip-compile command to run it under Bazel"
 
+import atexit
 import os
-import re
+import shutil
 import sys
 from pathlib import Path
-from shutil import copyfile
 
+import piptools.writer as piptools_writer
 from piptools.scripts.compile import cli
 
+# Replace the os.replace function with shutil.copy to work around os.replace not being able to
+# replace or move files across filesystems.
+os.replace = shutil.copy
+
+# Next, we override the annotation_style_split and annotation_style_line functions to replace the
+# backslashes in the paths with forward slashes. This is so that we can have the same requirements
+# file on Windows and Unix-like.
+original_annotation_style_split = piptools_writer.annotation_style_split
+original_annotation_style_line = piptools_writer.annotation_style_line
+
+
+def annotation_style_split(required_by) -> str:
+    required_by = set([v.replace("\\", "/") for v in required_by])
+    return original_annotation_style_split(required_by)
+
+
+def annotation_style_line(required_by) -> str:
+    required_by = set([v.replace("\\", "/") for v in required_by])
+    return original_annotation_style_line(required_by)
+
+
+piptools_writer.annotation_style_split = annotation_style_split
+piptools_writer.annotation_style_line = annotation_style_line
+
 
 def _select_golden_requirements_file(
     requirements_txt, requirements_linux, requirements_darwin, requirements_windows
@@ -41,19 +66,6 @@ def _select_golden_requirements_file(
         return requirements_txt
 
 
-def _fix_up_requirements_in_path(absolute_prefix, output_file):
-    """Fix up references to the input file inside of the generated requirements file.
-
-    We don't want fully resolved, absolute paths in the generated requirements file.
-    The paths could differ for every invocation. Replace them with a predictable path.
-    """
-    output_file = Path(output_file)
-    contents = output_file.read_text()
-    contents = contents.replace(absolute_prefix, "")
-    contents = re.sub(r"\\(?!(\n|\r\n))", "/", contents)
-    output_file.write_text(contents)
-
-
 if __name__ == "__main__":
     if len(sys.argv) < 4:
         print(
@@ -75,7 +87,6 @@ def _fix_up_requirements_in_path(absolute_prefix, output_file):
     # absolute prefixes in the locked requirements output file.
     requirements_in_path = Path(requirements_in)
     resolved_requirements_in = str(requirements_in_path.resolve())
-    absolute_prefix = resolved_requirements_in[: -len(str(requirements_in_path))]
 
     # Before loading click, set the locale for its parser.
     # If it leaks through to the system setting, it may fail:
@@ -86,7 +97,7 @@ def _fix_up_requirements_in_path(absolute_prefix, output_file):
     os.environ["LANG"] = "C.UTF-8"
 
     UPDATE = True
-    # Detect if we are running under `bazel test`
+    # Detect if we are running under `bazel test`.
     if "TEST_TMPDIR" in os.environ:
         UPDATE = False
         # pip-compile wants the cache files to be writeable, but if we point
@@ -95,31 +106,13 @@ def _fix_up_requirements_in_path(absolute_prefix, output_file):
         # In theory this makes the test more hermetic as well.
         sys.argv.append("--cache-dir")
         sys.argv.append(os.environ["TEST_TMPDIR"])
-        # Make a copy for pip-compile to read and mutate
+        # Make a copy for pip-compile to read and mutate.
         requirements_out = os.path.join(
             os.environ["TEST_TMPDIR"], os.path.basename(requirements_txt) + ".out"
         )
-        copyfile(requirements_txt, requirements_out)
-
-    elif "BUILD_WORKSPACE_DIRECTORY" in os.environ:
-        # This value, populated when running under `bazel run`, is a path to the
-        # "root of the workspace where the build was run."
-        # This matches up with the values passed in via the macro using the 'rootpath' Make variable,
-        # which for source files provides a path "relative to your workspace root."
-        #
-        # Changing to the WORKSPACE root avoids 'file not found' errors when the `.update` target is run
-        # from different directories within the WORKSPACE.
-        os.chdir(os.environ["BUILD_WORKSPACE_DIRECTORY"])
-    else:
-        err_msg = (
-            "Expected to find BUILD_WORKSPACE_DIRECTORY (running under `bazel run`) or "
-            "TEST_TMPDIR (running under `bazel test`) in environment."
-        )
-        print(
-            err_msg,
-            file=sys.stderr,
-        )
-        sys.exit(1)
+        # 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(requirements_txt, requirements_out)
 
     update_command = os.getenv("CUSTOM_COMPILE_COMMAND") or "bazel run %s" % (
         update_target_label,
@@ -137,12 +130,17 @@ def _fix_up_requirements_in_path(absolute_prefix, output_file):
 
     if UPDATE:
         print("Updating " + requirements_txt)
-        try:
-            cli()
-        except SystemExit as e:
-            if e.code == 0:
-                _fix_up_requirements_in_path(absolute_prefix, requirements_txt)
-            raise
+        if "BUILD_WORKSPACE_DIRECTORY" in os.environ:
+            workspace = os.environ["BUILD_WORKSPACE_DIRECTORY"]
+            requirements_txt_tree = os.path.join(workspace, requirements_txt)
+            # In most cases, requirements_txt will be a symlink to the real file in the source tree.
+            # If symlinks are not enabled (e.g. on Windows), then requirements_txt will be a copy,
+            # and we should copy the updated requirements back to the source tree.
+            if not os.path.samefile(requirements_txt, requirements_txt_tree):
+                atexit.register(
+                    lambda: shutil.copy(requirements_txt, requirements_txt_tree)
+                )
+        cli()
     else:
         # cli will exit(0) on success
         try:
@@ -160,7 +158,6 @@ def _fix_up_requirements_in_path(absolute_prefix, output_file):
                 )
                 sys.exit(1)
             elif e.code == 0:
-                _fix_up_requirements_in_path(absolute_prefix, requirements_out)
                 golden_filename = _select_golden_requirements_file(
                     requirements_txt,
                     requirements_linux,

diff --git a/tests/compile_pip_requirements/BUILD.bazel b/tests/compile_pip_requirements/BUILD.bazel
@@ -22,12 +22,13 @@ EOF
 compile_pip_requirements(
     name = "requirements",
     data = [
+        "requirements.in",
         "requirements_extra.in",
     ],
     extra_args = [
         "--allow-unsafe",
         "--resolver=backtracking",
     ],
-    requirements_in = "requirements.in",
+    requirements_in = "requirements.txt",
     requirements_txt = "requirements_lock.txt",
 )
diff --git a/tests/compile_pip_requirements/requirements.txt b/tests/compile_pip_requirements/requirements.txt
@@ -0,0 +1 @@
+-r requirements.in
diff --git a/tools/publish/BUILD.bazel b/tools/publish/BUILD.bazel
@@ -4,12 +4,4 @@ compile_pip_requirements(
     name = "requirements",
     requirements_darwin = "requirements_darwin.txt",
     requirements_windows = "requirements_windows.txt",
-    # This fails on RBE right now, and we don't need coverage there:
-    # WARNING: Retrying (Retry(total=0, connect=None, read=None, redirect=None, status=None))
-    # after connection broken by 'NewConnectionError('<pip._vendor.urllib3.connection.HTTPSConnection object at 0x7f3784e08110>:
-    # Failed to establish a new connection: [Errno -3] Temporary failure in name resolution')': /simple/twine/
-    #
-    # ERROR: Could not find a version that satisfies the requirement twine==4.0.2
-    # (from -r tools/publish/requirements.in (line 1)) (from versions: none)
-    tags = ["no-remote-exec"],
 )