Stop explicitly listing wheel as a build dependency #12728
Conversation
|
This should have no impact on users. I deem it a trivial change. If a news entry is required anyway, what category should I use? |
|
Yeah, it won't have any impact. I wonder how I missed it earlier.. Not sure if a change note is needed — I proposed having more granular categories some time ago but that discussion is stuck.. I'll let the maintainers decide. |
webknjaz
added
type: enhancement
|
@uranusjr @pradyunsg this is an easy merge FYI |
|
A feature changelog entry would be appropriate. |
|
I can certainly add it, but this adds no feature. |
|
None of the categories fit, so let's put it down as a feature since this is worth calling out in the changelog IMO. |
setuptools' get_requires_for_build_wheel() hook already injects this dependency when building wheels is requested. See also 64d8938.
|
For the record, I don't think this is worth calling out in the changelog. It only impacts users who build sdists of pip. Nevertheless, the changelog fragment has been added. |
|
FWIW, the audience is downstreams, which is separate from the end-users. Hence, my proposal @ #12555. |
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
|
@pradyunsg easy merge plz? |
|
I'll leave it to @pradyunsg as RM to decide if this is OK to go in 24.1 when we're already at beta 2. |
| ``setuptools`` already injects this dependency in the ``get_requires_for_build_wheel()`` hook. | ||
| This makes no difference for users of ``pip``. | ||
| This makes no difference when building wheels of ``pip``. | ||
| This avoids an unnecessary dependency on ``wheel`` when building the source distribution of ``pip``. |
There was a problem hiding this comment.
Since this is not a user facing feature, I'd rename this news/12728.process.rst which is the best category we have now for this kind of change to the build process
There was a problem hiding this comment.
One could argue that it's a bugfix. But a "process"? This is a rather weird workaround.
|
For such a trivial change, the changelog discussion kinda makes me regret proposing it. To whoever is merging this: feel free to adjust the changlog fragment however you desire. |
I don't think you should be angry or regret things. I think communicating a change is as important (or even more important) than the change itself. This is actually a very good discussion here, and various perspective have been presented, so @hroncok - if I were you I would be rather happy to see it and learn how open source maintainers think. I think it's worth considering different maintainers perspectives on it, because they want to make sure that all kinds of users (pip end users but also - as mentioned earlier - downstream maintainers) - are aware of the change - and it's perfectly OK that different people have different opinions, so I would rather wait for @pradyunsg as RM to make final decision on it :). |
|
I am not angry. It's just a bit frustrating. |
|
I don't wish to spark any conflict here, but I do want to remind everyone that we're all volunteers. I can understand where @hroncok is coming from. I've been frustrated myself when one of my PRs is stuck in review hell, taking way longer than it should. I get that you're simply trying to offer another perspective @potiuk (which we can all agree is useful!) but saying someone shouldn't feel their anger right off the bat doesn't seem super nice (even though the second half of your message is cordial!). Anyway, @hroncok apologies for the considerable back and forth on this PR. We've historically been burnt by various seemingly minor changes causing issues for downstream repackagers, so we are wary of making changes like this one. We appreciate the PR :) |
|
Thank you. |
setuptools' get_requires_for_build_wheel() hook already injects this dependency when building wheels is requested.
See also 64d8938.
cc @webknjaz