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

Embed typing_extensions package to support Python < 3.8 #10

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

KoltesDigital
Copy link
Contributor

When running bazel run //:venv env whereby Python 3.7 is invoked:

Traceback (most recent call last):
  File "C:\Users\XYZ\AppData\Local\Temp\Bazel.runfiles_dcrvwwbl\runfiles\rules_pyvenv\vendor\importlib_metadata\importlib_metadata\_compat.py", line 9, 
in <module>
    from typing import Protocol
ImportError: cannot import name 'Protocol' from 'typing' (C:\Python37\lib\typing.py)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:\Users\XYZ\AppData\Local\Temp\Bazel.runfiles_dcrvwwbl\runfiles\rules_pyvenv\build_env.py", line 26, in <module>
    import importlib_metadata
  File "C:\Users\XYZ\AppData\Local\Temp\Bazel.runfiles_dcrvwwbl\runfiles\rules_pyvenv\vendor\importlib_metadata\importlib_metadata\__init__.py", line 17, in <module>
    from . import _adapters, _meta, _py39compat
  File "C:\Users\XYZ\AppData\Local\Temp\Bazel.runfiles_dcrvwwbl\runfiles\rules_pyvenv\vendor\importlib_metadata\importlib_metadata\_meta.py", line 1, in <module>
    from ._compat import Protocol
  File "C:\Users\XYZ\AppData\Local\Temp\Bazel.runfiles_dcrvwwbl\runfiles\rules_pyvenv\vendor\importlib_metadata\importlib_metadata\_compat.py", line 12, in <module>
    from typing_extensions import Protocol  # type: ignore
ModuleNotFoundError: No module named 'typing_extensions'

Occurs here:

typing_extensions is imported, but this is not a built-in package: https://pypi.org/project/typing-extensions/

This patch adds that lib to the vendor directory. I believe its license allows it.

While this patch makes it work, I find a bit sad to always depend on a new lib only to support old Python versions. I've also tried not to embed any third-party lib but to fetch them with http_archive, which then could have been combined with select to only fetch typing_extensions if needed. However this would require changing the WORKSPACE system to a MODULE.bazel system to make a dependency chain, publishing the module to the registry... Apparently Bazel will push towards this change over time anyway.

BTW, I tried to run the example as a test, and some packages were missing from requirements.txt so venv creation was blocked. Running bazel run //requirements.update would only add colorama (why was it missing anyway?), whereas importlib-metadata, typing-extensions, and zipp were also needed due to black, but because they are the three vendor libs some conflict may have occurred. I haven't committed that change, because you'd have a better sight on that.

@KoltesDigital
Copy link
Contributor Author

KoltesDigital commented Mar 19, 2023

Well I was really eager to explore Bazel. I've updated the PR to download the dependency packages rather than embedding them.

BTW I've taken the latest versions, which may not be what you want, all I can say is it's working on my machine (TM).

colorama is still missing from requirements.txt but maybe that's because I'm Windows? Anyway the three vendor libs are not needed in there anymore.

@jvolkman
Copy link
Contributor

Hi, thanks for the PR.

I'm not opposed to the change in spirit. I don't recall why I went with the vendoring approach but I'm guessing it was to avoid requiring a rules_pyvenv_requirements line in WORKSPACE. But, adopting that certainly simplifies other things.

A couple of suggestions though:

  1. I'd skip the templating and just prefix the depenency workspace names with something like cedarai_rules_pyvenv_*. This seems to be standard practice.
  2. You can use the requirements.txt rules from rules_python to vendor a generated .bzl file, so you don't have to write the http rules yourself. This is the approach I'd take. You can see a small tool I wrote to streamline this in another repository: https://github.com/jvolkman/rules_pycross/blob/main/update_pypi_requirements_bzl.sh

@KoltesDigital
Copy link
Contributor Author

Hi. I continued a bit practicing Bazel and am coming up with some new commits.

Regarding your first point, I'm wondering, my WORKSPACE reads exactly like that:

http_archive(name = "rules_python", ...)
load("@rules_python//python:pip.bzl", "pip_parse")
pip_parse(name = "pip_deps", ...)
load("@pip_deps//:requirements.bzl", "install_deps")
install_deps()

So it seems to me that the standard practice is to pass a name (rules_python, pip_deps) to a function which will generate a repository with that name.

I understand you had to suggest that if your boss was looking over your shoulder, or if you are this person :)

For the second point, I've never tried before, will do.

@alexeagle
Copy link

I ran into this as well, trying to exercise the examples at HEAD

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.

None yet

3 participants