feat(pypi): actually start using env_marker_setting #2873
Conversation
Summary: - `pep508_deps` is now much simpler, because the hard work is done in analysis phase - `whl_library` BUILD.bazel tests now also have a test for the legacy flow. One thing that I noticed is that now we have an implicit dependency on the python toolchain when getting all of the `whl` target tree. This is a filegroup target that includes dependent wheels. However, we fallback to the flag values if we don't have the toolchain, so we should be good in general. Overall I like how this is turning out because we don't need to pipe the `target_platforms` anymore when we enable `PIPSTAR` feature. This means that we can start creating fewer whl_library instances - e.g. a `py3-none-any` wheel can be fetched once instead of once per python interpreter version. I'll leave this optimization for a later time. Work towards bazel-contrib#260
| name = "", | ||
| target = "requirements.bzl", | ||
| ), | ||
| "all_whl_requirements_by_package", |
There was a problem hiding this comment.
TODO: check if this will contain all of the packages or only the ones that are present on all platforms.
We may need to re-include the config.bzl.tmpl.bzlmod and put the list of all packages there.
|
Marking as draft due to the TODO note. Extra notes:
|
This reverts commit 9b8e736.
|
I'll do the |
| rules.env_marker_setting( | ||
| name = "include_{}".format(dep), |
There was a problem hiding this comment.
I was expecting to a list of extras passed in. But I guess the extras are being handled earlier?
The case I had in mind was a more complicated boolean expression e.g. (extra == foo and python_version ~= 3.10) or (extra = bar and python_version ~= 3.9); something that requires the target config to be evaluated.
That seems rare, though? And can probably be ignored?
There was a problem hiding this comment.
I think we could split the handling of the requires_dist and create one target for each requested extra:
- We create a no extras target named
pkg__then for each requested extrapkg__{extra}and then we make thepkgtarget depend on["pkg__"] + ["pkg_{}".format(x) for x in extras]. - Then we can split the deps and the marker targets to be more correct.
I think this may be win-win and in the future we could also expose the extra targets, what do you think?
There was a problem hiding this comment.
Realized that this should be still fine as is, at least for your example. Leaving the comment unresolved in case something comes along.
Summary:
pep508_depsis now much simpler, because the hard work is done inanalysis phase
whl_libraryBUILD.bazel tests now also have a test for the legacyflow.
One thing that I noticed is that now we have an implicit dependency on
the python toolchain when getting all of the
whltarget tree. This isa filegroup target that includes dependent wheels. However, we fallback
to the flag values if we don't have the toolchain, so we should be good
in general.
Overall I like how this is turning out because we don't need to pipe the
target_platformsanymore when we enablePIPSTARfeature. This meansthat we can start creating fewer whl_library instances - e.g. a
py3-none-anywheel can be fetched once instead of once per pythoninterpreter version. I'll leave this optimization for a later time.
Work towards #260