Skip to content

refactor: use click in dependency_resolver.py #1071

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 1 commit into from
Nov 1, 2023
Merged

refactor: use click in dependency_resolver.py #1071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 10 additions & 5 deletions python/pip_install/requirements.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,14 +85,19 @@ def compile_pip_requirements(
args = [
loc.format(requirements_in),
loc.format(requirements_txt),
# String None is a placeholder for argv ordering.
loc.format(requirements_linux) if requirements_linux else "None",
loc.format(requirements_darwin) if requirements_darwin else "None",
loc.format(requirements_windows) if requirements_windows else "None",
"//%s:%s.update" % (native.package_name(), name),
"--resolver=backtracking",
"--allow-unsafe",
] + (["--generate-hashes"] if generate_hashes else []) + extra_args
]
if generate_hashes:
args.append("--generate-hashes")
if requirements_linux:
args.append("--requirements-linux={}".format(loc.format(requirements_linux)))
if requirements_darwin:
args.append("--requirements-darwin={}".format(loc.format(requirements_darwin)))
if requirements_windows:
args.append("--requirements-windows={}".format(loc.format(requirements_windows)))
args.extend(extra_args)

deps = [
requirement("build"),
Expand Down
55 changes: 31 additions & 24 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 @@ -19,7 +19,9 @@
import shutil
import sys
from pathlib import Path
from typing import Optional, Tuple

import click
import piptools.writer as piptools_writer
from piptools.scripts.compile import cli

Expand Down Expand Up @@ -77,24 +79,25 @@ def _locate(bazel_runfiles, file):
return bazel_runfiles.Rlocation(file)


if __name__ == "__main__":
if len(sys.argv) < 4:
print(
"Expected at least two arguments: requirements_in requirements_out",
file=sys.stderr,
)
sys.exit(1)

parse_str_none = lambda s: None if s == "None" else s
@click.command(context_settings={"ignore_unknown_options": True})
@click.argument("requirements_in")
@click.argument("requirements_txt")
@click.argument("update_target_label")
@click.option("--requirements-linux")
@click.option("--requirements-darwin")
@click.option("--requirements-windows")
@click.argument("extra_args", nargs=-1, type=click.UNPROCESSED)
def main(
requirements_in: str,
requirements_txt: str,
update_target_label: str,
requirements_linux: Optional[str],
requirements_darwin: Optional[str],
requirements_windows: Optional[str],
extra_args: Tuple[str, ...],
) -> None:
bazel_runfiles = runfiles.Create()

requirements_in = sys.argv.pop(1)
requirements_txt = sys.argv.pop(1)
requirements_linux = parse_str_none(sys.argv.pop(1))
requirements_darwin = parse_str_none(sys.argv.pop(1))
requirements_windows = parse_str_none(sys.argv.pop(1))
update_target_label = sys.argv.pop(1)

requirements_file = _select_golden_requirements_file(
requirements_txt=requirements_txt, requirements_linux=requirements_linux,
requirements_darwin=requirements_darwin, requirements_windows=requirements_windows
Expand Down Expand Up @@ -128,6 +131,8 @@ def _locate(bazel_runfiles, file):
os.environ["LC_ALL"] = "C.UTF-8"
os.environ["LANG"] = "C.UTF-8"

argv = []
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we lose sys.argv[0] compared to before. I'm not sure if the piptools cli expects argv to have argv[0] be the name of the original script but maybe we can add sys.argv[0] here in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be clear, are you suggesting we change this to the following?

Suggested change
argv = []
argv = [sys.argv[0]]

Copy link
Collaborator

Choose a reason for hiding this comment

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

From what I can tell, omitting argv[0] when calling cli() is correct.

Under the hood, piptools is using click, and the implementation is obscured by a ton of decorators. Digging into it...

  • compile.cli is an instance of the click Command class.
  • Command.__call__ is an alias for Command.main
  • Command.main docs say, for the first positionl arg (named args): "the arguments that should be used for parsing. If not provided sys.argv[1:] is used"

So it doesn't expect args to contain argv[0]. The docs go on further to say the optional prog_name arg is initialized to sys.argv[0] if not specified.

As a side note, there is a standalone_mode arg, which controls whether the command tries to exit or not.


UPDATE = True
# Detect if we are running under `bazel test`.
if "TEST_TMPDIR" in os.environ:
Expand All @@ -136,8 +141,7 @@ def _locate(bazel_runfiles, file):
# to the real user cache, Bazel sandboxing makes the file read-only
# and we fail.
# In theory this makes the test more hermetic as well.
sys.argv.append("--cache-dir")
sys.argv.append(os.environ["TEST_TMPDIR"])
argv.append(f"--cache-dir={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_file) + ".out"
Expand All @@ -153,14 +157,13 @@ def _locate(bazel_runfiles, file):
os.environ["CUSTOM_COMPILE_COMMAND"] = update_command
os.environ["PIP_CONFIG_FILE"] = os.getenv("PIP_CONFIG_FILE") or os.devnull

sys.argv.append("--output-file")
sys.argv.append(requirements_file_relative if UPDATE else requirements_out)
sys.argv.append(
argv.append(f"--output-file={requirements_file_relative if UPDATE else requirements_out}")
argv.append(
requirements_in_relative
if Path(requirements_in_relative).exists()
else resolved_requirements_in
)
print(sys.argv)
Copy link
Contributor

@hrfuller hrfuller Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unclear why this was here. Probably for debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my assumption too. Didn't seem like something that ought to stick around (adding clutter to stdout!).

Happy to add it back if someone has a good reason for it to stay there.

argv.extend(extra_args)

if UPDATE:
print("Updating " + requirements_file_relative)
Expand All @@ -176,7 +179,7 @@ def _locate(bazel_runfiles, file):
resolved_requirements_file, requirements_file_tree
)
)
cli()
cli(argv)
requirements_file_relative_path = Path(requirements_file_relative)
content = requirements_file_relative_path.read_text()
content = content.replace(absolute_path_prefix, "")
Expand All @@ -185,7 +188,7 @@ def _locate(bazel_runfiles, file):
# cli will exit(0) on success
try:
print("Checking " + requirements_file)
cli()
cli(argv)
print("cli() should exit", file=sys.stderr)
sys.exit(1)
except SystemExit as e:
Expand Down Expand Up @@ -219,3 +222,7 @@ def _locate(bazel_runfiles, file):
file=sys.stderr,
)
sys.exit(1)


if __name__ == "__main__":
main()