-
Notifications
You must be signed in to change notification settings - Fork 3k
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
PEP 518: Behavior when pyproject.toml doesn't have build-system.requires #5416
Comments
I think this came up in one of the discussions, and I think the decision was to isolate it anyways, but I don't remember why. |
@dstufft #4144 (comment) (and the comments before that) A quick skim doesn't show any mention/discussion on not isolating when |
@pypa/pip-committers Now that we've seen multiple bug reports about this behaviour, I think pip should be refusing to install packages if the The former is sort of pushy, forcing people to use PEP 518 if there's a |
@pradyunsg I agree. I'm not 100% clear what use cases there are for a If it's just people using the "you can put project config in here" part of PEP 518 without following the full spec, I'm +1 on rejecting And yes, it is pushy, but "please conform to the standards we took ages to define, when using a brand new file that never existed before the standard" seems like a perfectly OK point to be pushy on 😄 |
From #5402 (comment):
@RonnyPfannschmidt has correctly stated that making pip compliant with PEP 518's current language would break existing released versions of various packages invalid. @asottile is of the opinion that PEP 518 should be updated to allow |
i would like to propose to simply fall back to non-isolated building if the pyproject.toml is incorrect, but warning about it that way things keep "working" but people see it @asottile im wondering, could pyupgrade be thaught to match pyproject.toml and set up correct metadata if its missing, i'd prefer strict standards, |
this makes the file valid and prepares for pypa/pip#5416 and pypa/pip#5512
I agree with @RonnyPfannschmidt that the pragmatic solution is to warn that It really shouldn't be such a huge imposition to ask for the standard to be changed - it's not like the change is massively controversial. If no-one can be bothered to step up to champion that change, I don't see that we should be expected to act as if it had been made. Personally, I'd prefer that we just assumed the spec was being followed, and triggered build isolation and the new code paths when we saw the new-style config file. But I can see that some projects like the idea of |
My intent is to bring pip in line with the PEP. I like what @pfmoore proposes as a compromise since it seems a lot of tools have come to rely upon non-compliant pyproject.toml files now. It also gives users the opportunity to push to update PEP 518 if having the "build-system.requires" key is to be made optional. As long as we come around to being in compliance with what the PEP says eventually (read by the end of this year) I'm happy. :) |
As stated in #5512 (comment) already (but since discussion should happen here): PEP 518 only states that Therefore there should be no warning / later rejection of a pyproject.toml file missing this section. That's not really what this issue is about, but it was brought up here and resulted in #5512. |
I'd read it differently. It does say "for illustrative purposes only" right above the JSON schema. And it even states "build tools are expected to use the example configuration file above as their default semantics when a To be clear, I'm not against making |
Shot up a mail to distutils-sig: https://mail.python.org/mm3/archives/list/distutils-sig@python.org/thread/7A4QHWCHR54TSIO2DQZUVNZHZS6ZPBLY/ |
It seems to me that it's clear what we want to do here: /cc @brettcannon |
Add `build-system.requires` key to `pyproject.toml`: - this is necessary with pip 10.0.1, as otherwise the defaults will be to require both `setuptools` and `wheel` (and we only need the later) - a `pyproject.toml` with no `build-system.requires` key will be invalid and rejected by a future version of pip (see pypa/pip#5416 (comment))
Add `build-system.requires` key to `pyproject.toml`: - this is necessary with pip 10.0.1, as otherwise the defaults will be to require both `setuptools` and `wheel` (and we only need the later) - a `pyproject.toml` with no `build-system.requires` key will be invalid and rejected by a future version of pip (see pypa/pip#5416 (comment))
Trying to track back through this discussion. @pradyunsg commented:
Can you include links? The only two I can find are #5402 and #5511. They are both simple cases of users using The discussion here has spread to a lot of places, and it certainly feels like there's a wider issue, but I'm failing to find the actual evidence - and I'm starting to feel that in actual fact, there really isn't a big problem here at all. Before I say "never mind, pip's behaviour here is fine" can you confirm whether or not there are other issues that relate to this one? And provide links here, so that the chain of logic is clearer? Regardless of the above, this discussion did pick up on an area where the PEP's wording was unclear, and it's good to fix that. But it's quite possible that all we need to do is to tidy up the wording in the PEP and there's nothing more to do here. My original position was that the PEP said that omitting |
@pfmoore without |
@brettcannon That's certainly an option. As I say though, I'm unclear as to whether there's actually a significant enough issue here to warrant changing pip's behaviour at all. (Isolation is basically a pip implementation/PEP 517 issue, and I'm struggling to find evidence that we need to change what we currently have). In terms of PEP 518 I have no problem with saying that "if you have |
@pfmoore yeah, I'm conflating PEP 518 details with pip details since this discussion has gotten a bit intertwined. 😉 If you want me to kick off a new thread on distutils-sig on this last idea then let me know and I can do that. |
I've just commented on distutils-sig, I doubt it needs a new thread. |
I felt that the details of the implementation and the PEP wording are a bit intertwined since they're both going to influence each other both ways. I'm still calibrating where the line between standards/implementation is here. |
Closed by mistake. |
As for the scale of this issue, the two issues are from the user's end -- that isolation needs them to provide setuptools and wheel as well. I don't think there's anything we need to change for this. Making the key mandatory would make it easier for us to deal with such issues since we can just point to the fact that the project's metadata says that you need to provide these packages to install it. That is cleaner than saying, the towncrier configuration the project added triggers a newer code path which means you need to provide wheels for packages (you already have installed those in your environment but we're isolating the builds now). (edited for clarity + typos) To summarize my understanding here:
That leaves us with basically 2 reasonable ways we can go:
PS: The discussion does seem to have blown a little out of scale to me now and I might have contributed to that. I now feel while mentioning disabling isolation as an option when I started the thread, I should have explained why we shouldn't do that or just not mentioned it at all since it's an implementation detail and not a PEP detail. PPS: TOML 1.0 will have dotted keys, so |
I have to say I'm still not clear what option you actually support here. Regarding the PEP, I think this whole discussion has proved that a PEP change is needed, at a minimum to explicitly confirm that the If you believe that PEP 518 should mandate the For pip, we have to use the new PEP 518 code path (isolation) for projects that use
If (and only if) the consensus is that PEP 518 should allow for As a separate point, with regard to ease of explaining the behaviour, I take your point that it's easier to explain if [1] OK, so that's TOML 1.0. But it's a convenient shorthand, and I don't know any other concise way to say what I mean. |
(I had written a similar comment earlier today but it seems I'd gotten logged out of the network or something and it never got posted) Okay. Right. PEP wise, I'd prefer it be changed to make the table (and key) mandatory. If it's instead made optional, I do feel (more strongly than I realized earlier) that pip should keep doing what pip 18.0 would -- isolate on presence of I realize now that I've not really explained why I think the breakage it causes isn't a major issue and the advantage outweighs it (without going into pip's implementation details). The breakage would be pretty simple to work around -- just pass
The advantage is that by making people specify the key, it brings familiarity and more tools that use pyproject.toml, more projects would mention I guess this still has some (indirect) reference to how pip has isolation tied-in with this. I guess I'll think about how to say this after you post your summary.
I helped get it added to TOML (made the issue, engaged in discussion, made the spec change and all that) so I just mentioned this out of excitement. It's nice to see that this is the way we ended up referring to this key organically. :) |
OK. Please make that argument over on distutils-sig. It's not me you have to convince 😄
We can defer that discussion till the final decision on the PEP is made clear. |
Yep. Will do. :) |
It seems to me that distutils-sig has reached a consensus here. If we decide that pip should keep status quo as its behavior going forward, we should go ahead and remove the warning that's currently on pip's master when this edge-case occurs. I'm not keen on holding off 18.0 for this though, but am willing to do so it someone else from @pypa/pip-committers wants us to. Note that we might end up having to discuss a bit here and the current plan is to make the release on Monday. |
What's the delta between the current behavior and the distutils-sig behavior? Is it just the warning? |
Yes. |
Assuming we decide to isolate with (setuptools, wheel) when the build-system table is not specified. |
If the only change is the warning, then we should probably remove the warning, because it's not going to break in the future right? So the warning is just going to spread FUD. |
Whoops. I missed a detail - pip currently treats missing build-system table the same as missing requires key in a given build-system table but the discussion is inclined to rejecting the latter. I'll make a PR to remove the warning though. It doesn't need to be there now. 18.0 will behave like 10.0 does. I put in some minutes earlier drafting a PR for the 2b proposal (that's how I noticed this). I'll file it later -- let's not make that change in 18.0. We can discuss 2a vs 2b after 18.0 and make that change. |
Sorry, now I'm confused. Ignoring the warning, pip currently handles a missing If we're now disabling isolation on master if I agree that while the PEP will expect tools to error if |
I messed up in the above comment (it got posted prematurely - I didn't realize that it was posted). I'll rectify in a bit. |
Coming back around to this finally... About the "drafting a PR for the 2b proposal", I'd seen that pip stopped isolating in this edge case when #5512 was merged (by me); I subsequently realized that I had rectified that error in #5585. Sorry for the confusion. More concretely: #5626 is what I expect will be a part of pip 18.0 -- it simply removes the warning that was added in #5512. Would appreciate reviews on that. #5627 implements the error when |
Closing since I think we're done here. If someone wants to discuss changing pip's behavior in this edge case, please file a new issue. :) |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
pip/src/pip/_internal/req/req_install.py
Line 450 in 018f03a
If other @pypa/pip-committers think it's a good idea, we should not isolate if there's no
build-system.requires
in thepyproject.toml
file.This isn't something I noticed earlier but maybe this would have eased transitions (prevented issues like the one with pandas). I still think it would be a good idea to do this.
The text was updated successfully, but these errors were encountered: