-
-
Notifications
You must be signed in to change notification settings - Fork 584
pip.parse(python_version=)
should not be mandatory
#1708
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
Comments
Could you explain a little more how having a requirement on this mandatory argument make it more difficult to use |
When you create a module which uses a |
Did you find a solution for this? https://github.com/mvukov/rules_ros2/blob/1e16de8c3797d94b54ea589524fa7be0e45481d9/MODULE.bazel#L34-L67 is a workaround, but I hate it :P |
This may be possible once I finish #1837, but it is not in scope, so
contributions in a form of PRs are definitely welcome. I would be more
than happy to review those.
That said, I am curious of the usecase that you are trying to cover by
using all of the Python versions and requirements. Is it for testing
that your dependencies work on all python versions or is it something
else?
|
Isn't this required when you don't know which Python version other modules which depend on your module are using? Let's say, I develop module X which has pip dependencies. When module A wants to depend on X and uses Python version 3.8, this version needs to be declared in X's |
It seems that a few things broke in recent commits: - We are not using the `MODULE.bazel.lock` file and it seems that it is easy to miss when the components in the PyPI extension stop integrating well together. This happened during the switch to `{abi}_{os}_{plat}` target platform passing within the code. - The logger code stopped working in the extension after the recent additions to add the `rule_name`. - `repo_utils.getenv` was always getting `PATH` env var on bazel `6.x`. This PR fixes both cases and updates docs to serve as a better reminder. By fixing the `select_whls` code and we can just rely on target platform triples (ABI, OS, CPU). This gets one step closer to maybe supporting optional `python_version` which would address #1708. Whilst at it we are also adding different status messages for building the wheel from `sdist` vs just extracting or downloading the wheel. Tests: - Added more unit tests and brought them in line with the rest of the code. - Checked manually for differences between the `MODULE.bazel.lock` files in our `rules_python` extension before #2069 and after this PR and there are no differences except in the `experimental_target_platforms` attribute in `whl_library`. Before this PR you would see that we do not select any wheels for e.g. `MarkupSafe` and we are always building from `sdist`. Work towards #260.
With this change we set the default value of `--python_version` when the `python.toolchain` is used in `bzlmod` and we generate the appropriate config settings based on the registered toolchains and given overrides by the root module. This means that we expect the `--python_version` to be always set to a non-empty value under `bzlmod` and we can cleanup code which was handling `//conditions:default` case. This also means that we can in theory drop the requirement for `python_version` in `pip.parse` and just setup dependencies for all packages that we find in the `requirements.txt` file and move on. This is left as future work by myself or anyone willing to contribute. We can also start reusing the same `whl_library` instance for multi-platform packages because the python version flag is always set - this will simplify the layout and makes the feature non-experimental anymore under bzlmod. Summary: * Add `@pythons_hub` to the `WORKSPACE` with dummy data to make pythons_hub work. * Add `MINOR_MAPPING` and `PYTHON_VERSIONS` to pythons_hub to generate the config settings. * Remove handling of the default version in `pypi` code under bzlmod. Work towards #2081, #260, #1708 --------- Co-authored-by: Richard Levasseur <[email protected]>
Since we have #2530, I want to write down extra thoughts here on what we can do:
|
|
Could we just fall back to the default Python version in case this is not specified but needed? Can't we just use the default Python toolchain for running |
Ah shoot, I have commented on the wrong issue - regarding the I think we could fallback on the default version, however, that would only work in root modules, because the root module defines the default. Otherwise the behaviour of So we actually have only 2 choices here:
I somewhat think the first option is simpler but not sure if preferable. In the future when #2629 is merged and when we don't need Python to extract simple wheels, we can do either of the above. My design goal for PyPI related things is to have somewhat immutable repository rule artifacts (i.e. if the defaults change, the rules don't need to be refetched) and then do the selecting between the defaults in the Hence my preference for the second approach where we just use all of the registered python toolchains and configure the wheels for all of them. |
I think the second option is even better. I have often seen a list comprehension with all versions for pip.parse(), so this would make it easier to get it right in such situations. |
🐞 bug report
Affected Rule
Bzlmod
pip.parse()
Is this a regression?
Yes, was not a problem in WORKSPACE.
Description
The arg documentation says the following:
https://github.com/bazelbuild/rules_python/blob/711186f144af06b431bd416b2d742874de3a2dea/python/private/bzlmod/pip.bzl#L391-L398
It specifically describes
If not specified, then the default Python version (as set by the root module or rules_python) will be used
. So this attribute should be optional but currently is mandatory.🔬 Minimal Reproduction
pip.parse()
withoutpython_version
.🔥 Exception or Error
🌍 Your Environment
Operating System:
Output of
bazel version
:Rules_python version:
Anything else relevant?
The text was updated successfully, but these errors were encountered: