Skip to content

fix: update correct requirements lock file when using os specific lock files #1123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 9, 2023
Merged

fix: update correct requirements lock file when using os specific lock files #1123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a5e0c7
fix: update correct requirements lock file when using os specific loc…
Rasrack Mar 10, 2023
68722ac
Merge integration tests so that we under the limit of 80 tasks.
Rasrack May 20, 2023
90fbf50
Update lock file according to upstream changes
Rasrack May 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions .bazelci/presubmit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,12 @@ tasks:
- "! git diff --exit-code"
- "bazel run //:requirements.update"
- "git diff --exit-code"
# Make a change to the locked requirements and then assert that //:os_specific_requirements.update does the
# right thing.
- "echo '' > requirements_lock_linux.txt"
- "! git diff --exit-code"
- "bazel run //:os_specific_requirements.update"
- "git diff --exit-code"
integration_test_compile_pip_requirements_debian:
<<: *reusable_build_test_all
name: compile_pip_requirements integration tests on Debian
Expand All @@ -447,6 +453,12 @@ tasks:
- "! git diff --exit-code"
- "bazel run //:requirements.update"
- "git diff --exit-code"
# Make a change to the locked requirements and then assert that //:os_specific_requirements.update does the
# right thing.
- "echo '' > requirements_lock_linux.txt"
- "! git diff --exit-code"
- "bazel run //:os_specific_requirements.update"
- "git diff --exit-code"
integration_test_compile_pip_requirements_macos:
<<: *reusable_build_test_all
name: compile_pip_requirements integration tests on macOS
Expand All @@ -459,6 +471,12 @@ tasks:
- "! git diff --exit-code"
- "bazel run //:requirements.update"
- "git diff --exit-code"
# Make a change to the locked requirements and then assert that //:os_specific_requirements.update does the
# right thing.
- "echo '' > requirements_lock_darwin.txt"
- "! git diff --exit-code"
- "bazel run //:os_specific_requirements.update"
- "git diff --exit-code"
integration_test_compile_pip_requirements_windows:
<<: *reusable_build_test_all
name: compile_pip_requirements integration tests on Windows
Expand All @@ -471,6 +489,12 @@ tasks:
- "! git diff --exit-code"
- "bazel run //:requirements.update"
- "git diff --exit-code"
# Make a change to the locked requirements and then assert that //:os_specific_requirements.update does the
# right thing.
- "echo '' > requirements_lock_windows.txt"
- "! git diff --exit-code"
- "bazel run //:os_specific_requirements.update"
- "git diff --exit-code"

integration_test_pip_repository_entry_points_ubuntu_min:
<<: *minimum_supported_version
Expand Down
51 changes: 25 additions & 26 deletions python/pip_install/tools/dependency_resolver/dependency_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,25 +95,30 @@ def _locate(bazel_runfiles, file):
requirements_windows = parse_str_none(sys.argv.pop(1))
update_target_label = sys.argv.pop(1)

requirements_file = _select_golden_requirements_file(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this functionality. This allows users to not need to run the [[uname -r == foo ]] && cp requirements.txt requirements_foo.txt themselves.

requirements_txt=requirements_txt, requirements_linux=requirements_linux,
requirements_darwin=requirements_darwin, requirements_windows=requirements_windows
)

resolved_requirements_in = _locate(bazel_runfiles, requirements_in)
resolved_requirements_txt = _locate(bazel_runfiles, requirements_txt)
resolved_requirements_file = _locate(bazel_runfiles, requirements_file)

# Files in the runfiles directory has the following naming schema:
# Main repo: __main__/<path_to_file>
# External repo: <workspace name>/<path_to_file>
# We want to strip both __main__ and <workspace name> from the absolute prefix
# to keep the requirements lock file agnostic.
repository_prefix = requirements_txt[: requirements_txt.index("/") + 1]
absolute_path_prefix = resolved_requirements_txt[
: -(len(requirements_txt) - len(repository_prefix))
repository_prefix = requirements_file[: requirements_file.index("/") + 1]
absolute_path_prefix = resolved_requirements_file[
: -(len(requirements_file) - len(repository_prefix))
]

# As requirements_in might contain references to generated files we want to
# use the runfiles file first. Thus, we need to compute the relative path
# from the execution root.
# Note: Windows cannot reference generated files without runfiles support enabled.
requirements_in_relative = requirements_in[len(repository_prefix) :]
requirements_txt_relative = requirements_txt[len(repository_prefix) :]
requirements_in_relative = requirements_in[len(repository_prefix):]
requirements_file_relative = requirements_file[len(repository_prefix):]

# Before loading click, set the locale for its parser.
# If it leaks through to the system setting, it may fail:
Expand All @@ -135,11 +140,11 @@ def _locate(bazel_runfiles, file):
sys.argv.append(os.environ["TEST_TMPDIR"])
# 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"
os.environ["TEST_TMPDIR"], os.path.basename(requirements_file) + ".out"
)
# 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_txt, requirements_out)
shutil.copy(resolved_requirements_file, requirements_out)

update_command = os.getenv("CUSTOM_COMPILE_COMMAND") or "bazel run %s" % (
update_target_label,
Expand All @@ -150,7 +155,7 @@ def _locate(bazel_runfiles, file):

sys.argv.append("--generate-hashes")
sys.argv.append("--output-file")
sys.argv.append(requirements_txt_relative if UPDATE else requirements_out)
sys.argv.append(requirements_file_relative if UPDATE else requirements_out)
sys.argv.append(
requirements_in_relative
if Path(requirements_in_relative).exists()
Expand All @@ -159,28 +164,28 @@ def _locate(bazel_runfiles, file):
print(sys.argv)

if UPDATE:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about the broader picture, we could have the UPDATE being handled by a separate script and use the approach elsewhere where:

  1. There is a rule that generates the pinned requirements_file.
  2. There is a diff_test rule, which checks the source version with the output of 1.
  3. There is a rule that updates the source version.

Right now we use the same script for everything, which allows us to not use bazel-skylib for this functionality, but now we depend on it more heavily and thus we could refactor this code as well.

Whilst this suggestion is likely to be outside the scope of the PR, I would love to understand if the platform resolution complexity could be pushed to starlark and a bunch of select statements in the right places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It almost sound like you are aiming for how it is done in the pip_parse vendored example. To me it sounds like a sensible change.

print("Updating " + requirements_txt_relative)
print("Updating " + requirements_file_relative)
if "BUILD_WORKSPACE_DIRECTORY" in os.environ:
workspace = os.environ["BUILD_WORKSPACE_DIRECTORY"]
requirements_txt_tree = os.path.join(workspace, requirements_txt_relative)
# 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,
requirements_file_tree = os.path.join(workspace, requirements_file_relative)
# In most cases, requirements_file will be a symlink to the real file in the source tree.
# If symlinks are not enabled (e.g. on Windows), then requirements_file will be a copy,
# and we should copy the updated requirements back to the source tree.
if not os.path.samefile(resolved_requirements_txt, requirements_txt_tree):
if not os.path.samefile(resolved_requirements_file, requirements_file_tree):
atexit.register(
lambda: shutil.copy(
resolved_requirements_txt, requirements_txt_tree
resolved_requirements_file, requirements_file_tree
)
)
cli()
requirements_txt_relative_path = Path(requirements_txt_relative)
content = requirements_txt_relative_path.read_text()
requirements_file_relative_path = Path(requirements_file_relative)
content = requirements_file_relative_path.read_text()
content = content.replace(absolute_path_prefix, "")
requirements_txt_relative_path.write_text(content)
requirements_file_relative_path.write_text(content)
else:
# cli will exit(0) on success
try:
print("Checking " + requirements_txt)
print("Checking " + requirements_file)
cli()
print("cli() should exit", file=sys.stderr)
sys.exit(1)
Expand All @@ -194,13 +199,7 @@ def _locate(bazel_runfiles, file):
)
sys.exit(1)
elif e.code == 0:
golden_filename = _select_golden_requirements_file(
requirements_txt,
requirements_linux,
requirements_darwin,
requirements_windows,
)
golden = open(_locate(bazel_runfiles, golden_filename)).readlines()
golden = open(_locate(bazel_runfiles, requirements_file)).readlines()
out = open(requirements_out).readlines()
out = [line.replace(absolute_path_prefix, "") for line in out]
if golden != out:
Expand Down
30 changes: 30 additions & 0 deletions tests/compile_pip_requirements/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,33 @@ compile_pip_requirements(
requirements_in = "requirements.txt",
requirements_txt = "requirements_lock.txt",
)

genrule(
name = "generate_os_specific_requirements_in",
srcs = [],
outs = ["requirements_os_specific.in"],
cmd = """
cat > $@ <<EOF
pip==22.3.0 ; sys_platform == "linux"
pip==22.2.2 ; sys_platform == "darwin"
pip==22.2.1 ; sys_platform == "win32"
EOF
""",
)

compile_pip_requirements(
name = "os_specific_requirements",
data = [
"requirements_extra.in",
"requirements_os_specific.in",
],
extra_args = [
"--allow-unsafe",
"--resolver=backtracking",
],
requirements_darwin = "requirements_lock_darwin.txt",
requirements_in = "requirements_os_specific.in",
requirements_linux = "requirements_lock_linux.txt",
requirements_txt = "requirements_lock.txt",
requirements_windows = "requirements_lock_windows.txt",
)
10 changes: 10 additions & 0 deletions tests/compile_pip_requirements/requirements_lock_darwin.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#
# This file is autogenerated by pip-compile with Python 3.9
# by the following command:
#
# bazel run //:os_specific_requirements.update
#
pip==22.2.2 ; sys_platform == "darwin" \
--hash=sha256:3fd1929db052f056d7a998439176d3333fa1b3f6c1ad881de1885c0717608a4b \
--hash=sha256:b61a374b5bc40a6e982426aede40c9b5a08ff20e640f5b56977f4f91fed1e39a
# via -r requirements_os_specific.in
10 changes: 10 additions & 0 deletions tests/compile_pip_requirements/requirements_lock_linux.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#
# This file is autogenerated by pip-compile with Python 3.9
# by the following command:
#
# bazel run //:os_specific_requirements.update
#
pip==22.3 ; sys_platform == "linux" \
--hash=sha256:1daab4b8d3b97d1d763caeb01a4640a2250a0ea899e257b1e44b9eded91e15ab \
--hash=sha256:8182aec21dad6c0a49a2a3d121a87cd524b950e0b6092b181625f07ebdde7530
# via -r requirements_os_specific.in
10 changes: 10 additions & 0 deletions tests/compile_pip_requirements/requirements_lock_windows.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#
# This file is autogenerated by pip-compile with Python 3.9
# by the following command:
#
# bazel run //:os_specific_requirements.update
#
pip==22.2.1 ; sys_platform == "win32" \
--hash=sha256:0bbbc87dfbe6eed217beff0021f8b7dea04c8f4a0baa9d31dc4cff281ffc5b2b \
--hash=sha256:50516e47a2b79e77446f0d05649f0d53772c192571486236b1905492bfc24bac
# via -r requirements_os_specific.in