Skip to content
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.

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

Fixed extra_pip_arg parsing error in pip_repository rules. #613

Merged
merged 1 commit into from
Jan 28, 2022

Conversation

UebelAndre
Copy link
Contributor

@UebelAndre UebelAndre commented Jan 28, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

I ran into an issue where pip_repository rules would fail whenever flags were added to requirements.txt (or requirements_lock). With the diff below, the following exception occurs when running bazel test //... in any workspace (here I just happen to have chosen an example at random).

diff --git a/examples/pip_repository_annotations/requirements.txt b/examples/pip_repository_annotations/requirements.txt
index 51d1dfc..db762cb 100644
--- a/examples/pip_repository_annotations/requirements.txt
+++ b/examples/pip_repository_annotations/requirements.txt
@@ -4,6 +4,8 @@
 #
 #    bazel run //:requirements.update
 #
+--extra-index-url https://pypi.python.org/simple/
+
 wheel==0.37.1 \
     --hash=sha256:4bdcd7d840138086126cd09254dc6195fb4fc6f01c050a1d7236f2630db1d22a \
     --hash=sha256:e9a504e793efbca1b8e0e9cb979a249cf4a0a7b5b8c9e8b65a5e39d49529c1c4
ERROR: no such package '@pypi//': rules_python failed:  (Traceback (most recent call last):
  File "/usr/local/Cellar/python@3.9/3.9.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9/runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/local/Cellar/python@3.9/3.9.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/private/var/tmp/_bazel_user/1364d13d3d56e9f951a1c2e7037fc439/external/rules_python/python/pip_install/parse_requirements_to_bzl/__main__.py", line 5, in <module>
    main()
  File "/private/var/tmp/_bazel_user/1364d13d3d56e9f951a1c2e7037fc439/external/rules_python/python/pip_install/parse_requirements_to_bzl/__init__.py", line 213, in main
    install_requirements = parse_install_requirements(
  File "/private/var/tmp/_bazel_user/1364d13d3d56e9f951a1c2e7037fc439/external/rules_python/python/pip_install/parse_requirements_to_bzl/__init__.py", line 43, in parse_install_requirements
    extra_pip_args.extend(shlex.split(line))
AttributeError: 'str' object has no attribute 'extend'
)

Issue Number: N/A

What is the new behavior?

This fixes this exception by correcting the regression introduced in #589 and adds regression tests.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

# Check for any annotations which match packages in the locked requirements file
install_requirements = parse_install_requirements(
args.requirements_lock, args.extra_pip_args
args.requirements_lock, whl_library_args["extra_pip_args"]
Copy link
Contributor Author

@UebelAndre UebelAndre Jan 28, 2022

Choose a reason for hiding this comment

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

@alexeagle alexeagle merged commit 28cfb11 into bazelbuild:main Jan 28, 2022
@UebelAndre UebelAndre deleted the args branch January 28, 2022 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants