Show a nice error if editable mode is attempted with a pyproject.toml source tree by cjerdonek · Pull Request #6331 · pypa/pip (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation18 Commits2 Checks0 Files changed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Fixes #6314.
This PR adds an informative error message if an editable install is attempted with a pyproject.toml-style (PEP 517) source tree. It also adjusts the logic so that, if PEP 517 is optional and editable mode is being used, then it prevents opting in to PEP 517 processing.
This PR has two commits:
- A preliminary refactor to make testing easier, and
- Adding the behavior change with tests.
To make reviewing easier, here are the three possible error messages that this PR can result in:
Error installing 'my-package': editable mode is not supported for pyproject.toml-based projects. This project is being processed as pyproject.toml-based because it has a pyproject.toml and no setup.py. See PEP 517 for the relevant specification.
Error installing 'my-package': editable mode is not supported for pyproject.toml-based projects. This project is being processed as pyproject.toml-based because it has a pyproject.toml with a "build-backend" key in the "build_system" value. See PEP 517 for the relevant specification.
Error installing 'my-package': editable mode is not supported for pyproject.toml-based projects. This project is being processed as pyproject.toml-based because PEP 517 processing was explicitly requested. See PEP 517 for the relevant specification.
One minor nit (I don't have time to do a full review right now). I'd prefer it if the message referred to "PEP 517 based" rather than "pyproject.toml-based". After all, it's PEP 517 that doesn't have a hook for doing editable installs.
But it is only a minor point.
Okay, thanks. Let me explain my thinking and how I arrived at that, and you can let me know what you think.
That word choice came about after reading and reflecting on issue #6256 ("pip's error message for wheel-build failure on PEP517 packages isn't very friendly"), and after reading the PEP more carefully.
Basically, I do really want pip to be more user-friendly, and my thinking was that it would be more user-friendly for us (and without sacrificing accuracy) to lead with the thing that more people will be familiar with (the pyproject.toml file) rather than the technical-sounding PEP number. PEP 517's purpose was to define a new kind of source tree, so I was thinking it should be okay for us to use that name it came up with:
There is an existing, legacy source tree format involving setup.py. ... We'll refer to it as the setup.py-style. Here we define a new style of source tree based around the pyproject.toml file
And further on:
Traditionally these have always contained setup.py-style source trees; we now allow them to also contain pyproject.toml-style source trees.
(After reading this again here, I see now it might be more faithful to the PEP for us to say "pyproject.toml-style" rather than "pyproject.toml-based.")
To your point re: PEP 517, I was careful to include a reference to PEP 517 for people already familiar with it, and for those that want to learn more. The error message also says why the source tree is being considered pyproject.toml-based, so users can understand that it's not just the presence of the file that triggers that behavior.
Lastly, another point is that it won't always be the case that pyproject.toml-style source trees will be governed by only one PEP. But the name will always be the same. So it also seems more "evergreen" for us to be using the English name over the PEP number.
Thanks. I take your point and agree with you (I'd forgotten that PEP 517 had given a name to this type of source tree).
Okay, thanks a lot, @pfmoore.
I just updated the PR in a couple ways. First, I had to scale the PR back slightly to get the tests passing. I noticed something that I'd like your thoughts on, @pfmoore, which I'll spell out for you at the end of this comment.
The second thing I changed was tweaking the error text to use the word "style" rather than "based" (in line with my comments above) to match PEP 517's language more closely. The three possible messages are now:
Error installing 'my-package': editable mode is not supported for pyproject.toml-style projects. This project is being processed as pyproject.toml-style because it has a pyproject.toml file and no setup.py. See PEP 517 for the relevant specification.
Error installing 'my-package': editable mode is not supported for pyproject.toml-style projects. This project is being processed as pyproject.toml-style because it has a pyproject.toml file with a "build-backend" key in the "build_system" value. See PEP 517 for the relevant specification.
Error installing 'my-package': editable mode is not supported for pyproject.toml-style projects. This project is being processed as pyproject.toml-style because PEP 517 processing was explicitly requested. See PEP 517 for the relevant specification.
Okay, here is the thing I'd like your thoughts on. There is a part of pip's pyproject.py that makes it so that if the PEP 517 conditions aren't met, but the project has a pyproject.toml file, then the code opts the project into PEP 517 processing. Here is that code:
| # If we haven't worked out whether to use PEP 517 yet, |
|---|
| # and the user hasn't explicitly stated a preference, |
| # we do so if the project has a pyproject.toml file. |
| elif use_pep517 is None: |
| use_pep517 = has_pyproject |
The possible problem I see is that it does this even if the user is doing an editable install. It's a problem IMO because PEP 517 doesn't support editable installs. When I tried disabling that opt-in logic when editable mode is being used, the following test failed:
| @pytest.mark.network |
|---|
| def test_constraints_local_editable_install_pep518(script, data): |
| to_install = data.src.join("pep518-3.0") |
| script.pip('download', 'setuptools', 'wheel', '-d', data.packages) |
| script.pip( |
| 'install', '--no-index', '-f', data.find_links, '-e', to_install) |
The "pep518-3.0" project in that test has a "build-system" "requires" section:
| [build-system] |
|---|
| requires=["simplewheel==2.0", "setuptools", "wheel"] |
and when the project isn't handled as a PEP 517 project, the code doesn't look at the contents of the pyproject.toml file to find the build requirements (the code reading pyproject.toml just returns None).
So I left that opt-in logic alone for now and only addressed the cases where the PEP 517 processing is required by the PEP. My question for you is -- if PEP 517 processing isn't required but the project has a pyproject.toml file, should pip be opting the project in to PEP 517 processing? I think the answer is no, but the fix is going to be a bit harder, because we'll basically need to revert to using the old code for that case. I think that change should be done in a separate PR because it's a bit harder IMO, and also it's not the case that was causing #6314. Does that seem reasonable to you?
My question for you is -- if PEP 517 processing isn't required but the project has a pyproject.toml file, should pip be opting the project in to PEP 517 processing?
This was a matter of debate at the start of the implementation. I don't currently have time to hunt out the references, but there were two key places where discussion happened - the issue for implementing PEP 517 and the PR itself.
Basically the question was, which of the new (PEP 517) and old (legacy) code paths should we use in cases of doubt. The consensus was that we should use PEP 517 wherever possible, pushing as many projects onto the new code path as we could. The discussion about this was around #5407 (comment). I have a feeling that originally, the code that now says use_pep517 = has_pyproject was use_pep517 = True, using PEP 517 unless it was impossible to do so or the user explicitly opted out, but we backed off slightly to the current version.
Nevertheless, I'm still strongly of the opinion that we should use PEP 517 wherever we can reasonably do so. Ultimately, we want to remove the legacy code path altogether. To do that, we need to use the PEP 517 code path even for projects that haven't opted into it, or we'll be unable to support the long tail of projects that won't ever add a pyproject.toml because they have no need for it.
I'm OK with opting out of PEP 517 if -e is specified, but only then. And even then, I have reservations, as it would be awfully easy to end up in a situation where pip install -e . and pip install . do fundamentally different things (the build requirements issue you found being one of them).
Thinking about this, I've raised #6334 as a placeholder registering the intention to ultimately remove the legacy codepath.
So, to summarise, my answer to your question "should pip be opting the project in to PEP 517 processing?" is yes, we should - and that was always the intended behaviour.
(Of course, there's always the option to change our minds if the evidence says we need to - but I don't believe that's the case right now).
@pfmoore Oops, sorry. I accidentally left out a phrase when I restated my question and meant for my question to focus only on editable mode, so I'll restate:
if PEP 517 processing isn't required but the project has a pyproject.toml file, should pip be opting the project in to PEP 517 processing if editable mode was requested?
It seems to me like we don't want to opt in to PEP 517 in this case for a couple reasons. First, we haven't thought through what it means to use PEP 517 in conjunction with editable mode (and the PEP expressly doesn't support it). And second, if we do add support for it later, I wouldn't want to risk prematurely adding in a new behavior that we need to preserve backwards compatibility with later on. (Basically, even though you've already said you're okay with not opting in in this case, I'm making a case for you to remove the reservations you might have with this option.)
@cjerdonek Ah, that makes more sense. I won't directly answer your question (as I'm getting lost in double negatives, and I don't want to add to the confusion). I'll just say what I think should happen (as I think that will be clearer):
I think that if the standard logic has resulted in pip deciding to process a project using PEP 517, and the user has specified -e, then we should fail saying that PEP 517 doesn't support editable installs (your wording for that seems fine to me). If the user wants editable mode, they can override this explicitly using -e --no-use-pep517.
There is a strong argument that this is not a particularly user-friendly approach, and I agree that it isn't. However, I am more concerned that if we do silently switch to the legacy code path for -e, the result will be potentially different behaviour (e.g., the install requirements issue you found) depending on whether the user does a normal or an editable install, and I think that's a worse problem. We could take a middle ground, and switch but warn, but IMO people tend not to read such warnings, so it won't help a lot (other than letting us say "we told you so"...)
Note - this is just my opinion, and I've been clear that I'm keen on forcing the pace on switching to PEP 517. Also, I'm also not a heavy user of editable installs myself. So I won't be surprised if the consensus is against me on this. And I'm not going to fight in that case. But I wanted to get my reasoning on record.
And second, if we do add support for it later, I wouldn't want to risk prematurely adding in a new behavior that we need to preserve backwards compatibility with later on.
I 100% agree with this - but my response is to implement no behaviour (i.e., fail). I think you're arguing that "use the legacy codepath" doesn't add new behaviour, but I'd view "use the legacy behaviour" as choosing a behaviour (for PEP 517 projects) that we'd have to change later.
I 100% agree with this - but my response is to implement no behaviour (i.e., fail). I think you're arguing that "use the legacy codepath" doesn't add new behaviour, but I'd view "use the legacy behaviour" as choosing a behaviour (for PEP 517 projects) that we'd have to change later.
I was actually going to suggest this (failing outright) as another option for the same reasons you mentioned, but I hesitated because I thought it might be too late given that 19.0 was already released. I do very much agree with you that what you're suggesting is the right approach for a properly forward-compatible approach. I'm totally okay with this approach if you're okay with it (and actually prefer it), provided that we're willing to accept any pushback after 19.1 is released. (It seems likely that some users will start to experience errors in 19.1 in some situations that they weren't experiencing before.)
I think that if the standard logic has resulted in pip deciding to process a project using PEP 517, and the user has specified -e, then we should fail saying that PEP 517 doesn't support editable installs (your wording for that seems fine to me). If the user wants editable mode, they can override this explicitly using -e --no-use-pep517.
One clarification here. There are actually two cases I think we should consider separately:
- when the PEP 517 behavior is mandated by the PEP (e.g.
pyproject.tomlis present with nosetup.py) - when the PEP 517 behavior isn't mandated by the PEP but pip is choosing to opt in (e.g.
pyproject.tomlis present along with asetup.pyand the user hasn't explicitly requested anything)
For (2), I agree with you that we should let the user override an editable failure with --no-use-pep517. However, for (1), I thought you had said above that we shouldn't permit the user to opt out of PEP 517 when the PEP mandates it -- that that was never the intent of the --no-use-pep517 option. Here is the comment of yours that I'm referring it. I'm happy to defer to you on what we should do here, but just want to clarify which you meant.
If the PEP mandates PEP 517 behaviour, then I think --no-use-pep517 will fail (and if it doesn't then it should). So we are in agreement, I just wasn't explicit that I expect --no-use-pep517 to fail in those cases.
Great! I think we're in agreement, too. I'll try doing the second half of what we discussed (the so-called optional case) in a separate PR after this one is done.
@pfmoore Would you be able to review this PR at some point? I'd like to be able to start working on "part 2" of this (well enough in advance of the next release, etc), but I was hoping this could be merged before then.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have time to do more than desk-check the PR via the github UI, I'm afraid, but it looks generally OK. One concern I have is that when we add the new PEP 517 backend-path support (see pypa/pyproject-hooks#46) it's going to be harder to thread the new backend-path key through the sequence of calls you've introduced here (resolve_pyproject_toml needs to take an extra parameter, which it does nothing with except pass it on, which is a code smell to me). And if we ever add further keys needed for creating the hook caller, this could get worse.
I'm OK with the comment "let's leave that for whoever deals with integrating that PR" (except for the fact that the obvious candidate may well be me, and I really don't want to have to deal with it ;-)) but I think it's worth being explicit that this change, as it stands, makes it harder to pass data from pyproject.toml to the backend. See https://github.com/pypa/pip/blob/master/src/pip/_internal/req/req_install.py#L488-L592 for how it's done now.
Thanks, @pfmoore. To provide some background on the rationale, the reason I broke out resolve_pyproject_toml() is that load_pyproject_toml() is a very complicated function with a large number of logical combinations to test. Even after breaking it out, resolve_pyproject_toml() is still a 138-line function.
The advantage of breaking out this particular function is that it makes it a lot easier to test the various code paths. We can test the logic independent of the file system; testing new combinations is just a matter of adding new tuples to the parametrized list of test cases. (In particular, we don't need to fiddle with writing a setup.py and/or a pyproject.toml to test each combination.) This will be very helpful in preventing regressions going forward because it makes it easier to track and maintain test coverage over all of those various cases. Also, each of the parameters that appears has various if clauses associated with it, so I think it's helpful to surface those explicitly so we can be clear on what the logic depends on.
Regarding backend-path, if it's true that resolve_pyproject_toml() wouldn't need to look at it, then it's possible we wouldn't to pass that as an argument.
Also, something I want to emphasize is that breaking out resolve_pyproject_toml() is what let me test each code path / error message that I introduced in this PR. I know beforehand and without that change there were portions of load_pyproject_toml() that were untested. I think it's good for us to do what we can to make the code easier to test and make sure more of the code is tested.
As I said, I mostly wanted to be sure that the point had been considered. Based on your comments, I'm happy to approve.
Okay, thanks a lot, @pfmoore. I'll merge and work on "part 2" shortly.
FYI, I finished working on the follow-on "part 2" of this PR. It's posted here: #6370
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.
Outdated issues that have been locked by automation
label
2 participants
