refactor(pip_install): make the API surface of deps parsing smaller #1657
Conversation
aignas
force-pushed
the
fix/make-deps-api-surface-smaller
branch
from
57aeec2 to
d1c7bc9
Compare
|
When you have time, please take a look @rickeylev. My end-goal would be to support parsing the metadata to support select statements with version aware python toolchain config settings, which should hopefully land in another PR. |
| key=lambda x: f"{x.name}:{sorted(x.extras)}", | ||
| ) | ||
|
|
||
| # Resolve any extra extras due to self-edges, empty string means not extras |
There was a problem hiding this comment.
I think I commented on the empty string thing in an earlier PR; this empty string thing is confusing.
Does the presence of the empty string mean no extras? i.e. are {"", "foo"} Or is it assumed that an empty string element is mutually exclusive with other elements? Why is "no extras" communicated using set-with-only-empty-string as opposed to empty-set ?
(I've authored and had to maintain code where None vs empty set vs non-empty set have 3 different meanings, and it was always a cognitive burden)
There was a problem hiding this comment.
Right now the case of {"", "foo"} and {"foo"} has the same meaning. I moved this detail into the method implementation to better isolate it from the rest of the code as it is an implementation detail. The return value of _resolve_extras should be always a set with at least one element in it and it just makes the matching with any(...) easier to write.
| # Add the platform-specific dep | ||
| self._select[platform].add(dep) | ||
|
|
||
| def add(self, *wheel_reqs: str) -> None: | ||
| reqs = [Requirement(wheel_req) for wheel_req in wheel_reqs] | ||
| # Add the dep to more specific constraints | ||
| for p in platform: | ||
| if p not in self._select: | ||
| continue | ||
|
|
||
| # Resolve any extra extras due to self-edges | ||
| self._want_extras = self._resolve_extras(reqs) | ||
| self._select[p].add(dep) |
There was a problem hiding this comment.
The if p not in self._select part of this block of code is confusing me.
The iteration will produce one of two list of values:
- Exactly one element: self (may or may not have arch set)
- Two or more elements: self (without an arch) and one or more with arch set.
In case one, L279 duplicates what L272 did: the loop is one element long, p is present, and L279 executes, and we're done.
In case two, the same thing happens: the loop is 1 + N long. The first element does L279 (duplicating L272), and then subsequent iterations do nothing (the arch-bound platform isn't present).
What am I missing? That some earlier add_req() call added an entry? But then wouldn't that mean some of these were ignored when they shouldn't have been...? or maybe the loop below has something to do with this? I think if I could see the requirements lines being processed it'd be easier for me to reason about.
There was a problem hiding this comment.
Added a unit-test and more comments to hopefully make the code more self-explanatory.
With this change we also fix a minor bug where the self edge dependencies within the wheels are correctly handled as per unit tests.
Use the fact that the selects are chosen based on the specificity of the condition and also make the manipulation of the deps correct through iteration of each Requires-Dist so that we don't need to do any post-processing in the build() function. This should hopefully make the extension to supporting python version, interpreter implementation and others easier as we are generalizing what the Platform and Deps data structures can do. I have added a test to ensure coverage of the new code.
It tries to cleanup the usage of generators and refactors the tests a little bit more so that we have more isolated test cases.
aignas
force-pushed
the
fix/make-deps-api-surface-smaller
branch
from
d1c7bc9 to
fef4ac5
Compare
With this change we also fix a minor bug where the self edge dependencies
within the wheels are correctly handled as per unit tests.
Also refactor the code to use the fact that the selects are chosen
based on the specificity of the condition. This can make the addition
of deps incrementally correct without the need for extra processing in
the
buildmethod. This should hopefully make the implementation morefuture-proof as it makes use of these generic concepts and is likely to
help to extend this code to work with selecting on multiple Python versions.
I have added a test to ensure coverage of the new code.