Skip to content
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

Fix editable-mode logic when pyproject.toml present #6370

Merged
merged 2 commits into from Apr 6, 2019

Conversation

Projects
None yet
7 participants
@cjerdonek
Copy link
Member

commented Mar 31, 2019

This is the second of two PR's to address installing in editable mode when a pyproject.toml file is present. The first part was PR #6331, which has already been merged.

Whereas PR #6331 addressed the case when PEP 517 requires handling the source tree as pyproject.toml-style, this PR addresses the case where it is optional (i.e. PEP 517 doesn't mandate it).

Specifically, this PR does four things:

  1. Raises an informative InstallationError if editable mode was requested, a pyproject.toml file is present but PEP 517 processing is optional, and no other PEP 517-related options were passed. Among other things, the error message tells the user they can pass --no-use-pep517 to bypass pyproject.toml-style processing and use editable mode. This behavior was discussed and agreed to on the comment thread for PR #6331. The error currently looks like this--

    Error installing 'my-package': editable mode is not supported for pyproject.toml-style projects. pip is processing this project as pyproject.toml-style because it has a pyproject.toml file. Since the project has a setup.py and the pyproject.toml has no "build-backend" key for the "build_system" value, you may pass --no-use-pep517 to opt out of pyproject.toml-style processing. See PEP 517 for details on pyproject.toml-style projects.

  2. Gets pip install -e working again for the PEP 517-is-optional case (provided --no-use-pep517 was passed). Previously, this case was broken because when PEP 517 processing was turned on by default for pip, the build-system.requires section was ignored in the non-PEP 517 case (in the IsSDist.prep_for_dist() method).

  3. Adds a lot more unit test coverage of resolve_pyproject_toml() (the pure function responsible for interpreting the various flags in relation to the contents of pyproject.toml). It also fixes test_constraints_local_editable_install_pep518() by passing --no-use-pep517 to the pip install -e invocation in the test, which it should have been doing before.

  4. Fixes some exception handling that occurs in IsSDist.prep_for_dist() when conflicts are found while processing build dependencies. The prior code constructed an exception message using a variable (conflicting) that hadn't been defined yet. So I don't think that code path was working before.

@cjerdonek cjerdonek force-pushed the cjerdonek:editable-and-pep-517-optional branch from 6eb4b59 to 414c359 Mar 31, 2019

@cjerdonek cjerdonek force-pushed the cjerdonek:editable-and-pep-517-optional branch from 414c359 to d021deb Mar 31, 2019

@cjerdonek cjerdonek marked this pull request as ready for review Mar 31, 2019

@cjerdonek cjerdonek changed the title Fix editable handling when pyproject.toml present Fix editable-mode logic when pyproject.toml present Mar 31, 2019

@cjerdonek cjerdonek force-pushed the cjerdonek:editable-and-pep-517-optional branch from d021deb to 233a023 Mar 31, 2019

@cjerdonek cjerdonek force-pushed the cjerdonek:editable-and-pep-517-optional branch from 233a023 to 71f506e Mar 31, 2019

@cjerdonek

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

Someone just reported as a new issue (#6375) the issue that this PR is meant to address.

@cjerdonek

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

@pfmoore I'm hoping you can take a look at this PR at some point. I think it would be good to try to get it into the next release in some form so that we can head off people unwittingly trying to combine editable mode with PEP 517 behavior before it gets too ingrained and harder to change. I'm worried some people may already be doing this -- see e.g. the just-filed #6375, as well as the Poetry issue #6314 from before. This PR will also serve another important function by helping to educate people through the error message.

Finally, the PR also unit tests pretty much all of the logical interactions between the various flags and error messages so we can encode that behavior and prevent regressions.

@con-f-use

This comment has been minimized.

Copy link

commented Apr 4, 2019

If I understand correctly, then accepting the proposed solution here means:

  1. Calling pip install --no-use-pep517 --editable . with a pyproject.toml present that does not specify a back-end, causes
    • ...build dependencies specified by requires=[...] in the pyproject.toml will be installed by pip (which btw. is not case yet, see #6375)
    • ...pip will then proceed installation not treating the project as specified by PEP517 but rather in the old way.
  2. Calling pip install --editable . with a pyproject.toml present that does not specify a back-end, on the other hand, will result in
    • ...display of editable mode is not supported for pyproject.toml-style projects error
    • ...pip exiting with an error status code

I might be wrong, but to me that means, the only canonical way to specify build dependencies in such a project is the --no-use-pep517 option (again, which currently does not install the build dependencies due to what might be a bug or a misunderstanding on my part).

Regardless, even if it did install the specified build dependencies, some other tools like Pipenv, have no way to specify this option on a by-requirement bases. What I mean is that in a Pipenv Pipfile, there is no way to do something like:

# Pipfile
[packages]
i_require_gitpython_to_build = { path = ".", editable = true, requires=["gitpython"] }
# or alternatively:
i_require_gitpython_to_build = { path = ".", editable = true, extra_pip_options=["--no-use-pep517"] }

Which effectively means, one cannot use those tools to develop projects with old-style setup.py scripts that have build dependencies.

I'm not at all sure if everything I wrote is correct and if the following is a good idea, but maybe, case 1.) should just display a warning instead of failing and the proceed with installing the build dependencies and not processing according to PEP517?


Btw. education on PEP517/518 and the new canonical way of building, developing and installing python package is, from my user perspective, a bit scarce at the moment. So every little bit as soon as possible is appreciated. Just as an example, it took me a long time to find out, that setup-tools does not have its own section in a pyproject.toml akin to [tools.flit.metadata], i.e. it does not parse:

[tool.setuptools.metadata]
module = "mymodule"
author = "confus"
author-email = "someone@somedomain.com"

And other little things like PEP517/518, pyproject.toml, flit, hatch and poetry just being a small footnote in the official python packaging guide or a user-friendly comprehensive official documentation on setuptools sorely missing. But that's offtopic, sorry.

@pganssle pganssle referenced this pull request Apr 4, 2019

Merged

Add support for setup.cfg-only projects #1675

2 of 2 tasks complete
@cjerdonek

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@con-f-use I believe your summary is correct. A few comments:

  • It's a bug that the dependencies in pyproject.toml aren't being installed when editable mode is used. (That's one of the issues this PR is meant to fix.)
  • It looks like pipenv may need an update then. (Poetry also needed its own update, for a similar reason. See #6314.) Would you be able to let them know?

I'm not at all sure if everything I wrote is correct and if the following is a good idea, but maybe, case 1.) should just display a warning instead of failing and the proceed with installing the build dependencies and not processing according to PEP517?

The issue you're raising here was discussed in "part 1" of this PR. (I was the one that brought it up there.) See e.g. this comment: #6331 (comment)

Basically, the issue is that the presence of pyproject.toml was meant to be the way for users to opt in to the new PEP 517 behavior. So allowing pyproject.toml to result in the old behavior would defeat / undermine that purpose. For example, if and when we add support for editable mode to PEP 517, we would then have a backwards compatibility issue if we were to let invocations happen today. This is the point that @pfmoore was making in his comment I linked to above (and that I agree with, for the same reasons). Yes, it will be a harder pill for users to swallow today (but it's still very early in PEP 517's adoption). But that pill will become a much bigger problem later on if we permit the old behavior to happen with pyproject.toml (without an explicit flag from the user like --no-use-pep517).

One reason I want this PR to land sooner rather than later is to head off this problem earlier, because the longer it's postponed, the more users will be negatively affected by waiting.

@cjerdonek

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

PS - yes, I agree there needs to be better documentation. Having said that, I do think the error messages in this PR and the previous will help to educate people and point them to where to find more information on this topic. That was a deliberate goal on my part and one of the reasons the error messages are a bit longer compared with other error messages by pip.

@pfmoore

pfmoore approved these changes Apr 5, 2019

@con-f-use

This comment has been minimized.

Copy link

commented Apr 5, 2019

Again, thanks for you patience.

* It looks like pipenv may need an update then. (Poetry also needed its own update, for a similar reason. See #6314.) Would you be able to let them know?

Already kinda did yesterday. Maybe a gentle nudge from someone more known in the packaging work wold be additional motivation, tough ;-)

@cjerdonek cjerdonek merged commit 78744e8 into pypa:master Apr 6, 2019

29 checks passed

Linux Build #20190331.7 succeeded
Details
Linux (Package) Package succeeded
Details
Linux (Test Primary Python27) Test Primary Python27 succeeded
Details
Linux (Test Primary Python36) Test Primary Python36 succeeded
Details
Linux (Test Secondary Python34) Test Secondary Python34 succeeded
Details
Linux (Test Secondary Python35) Test Secondary Python35 succeeded
Details
Linux (Test Secondary Python37) Test Secondary Python37 succeeded
Details
Windows Build #20190331.7 succeeded
Details
Windows (Package) Package succeeded
Details
Windows (Test Primary Python27-x64) Test Primary Python27-x64 succeeded
Details
Windows (Test Primary Python36-x64) Test Primary Python36-x64 succeeded
Details
Windows (Test Secondary Python27-x86) Test Secondary Python27-x86 succeeded
Details
Windows (Test Secondary Python34-x64) Test Secondary Python34-x64 succeeded
Details
Windows (Test Secondary Python34-x86) Test Secondary Python34-x86 succeeded
Details
Windows (Test Secondary Python35-x64) Test Secondary Python35-x64 succeeded
Details
Windows (Test Secondary Python35-x86) Test Secondary Python35-x86 succeeded
Details
Windows (Test Secondary Python36-x86) Test Secondary Python36-x86 succeeded
Details
Windows (Test Secondary Python37-x64) Test Secondary Python37-x64 succeeded
Details
Windows (Test Secondary Python37-x86) Test Secondary Python37-x86 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
macOS Build #20190331.7 succeeded
Details
macOS (Package) Package succeeded
Details
macOS (Test Primary Python27) Test Primary Python27 succeeded
Details
macOS (Test Primary Python36) Test Primary Python36 succeeded
Details
macOS (Test Secondary Python34) Test Secondary Python34 succeeded
Details
macOS (Test Secondary Python35) Test Secondary Python35 succeeded
Details
macOS (Test Secondary Python37) Test Secondary Python37 succeeded
Details
news-file/pr News files updated and/or change is trivial.
Details
@cjerdonek

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2019

Thanks, @pfmoore.

@cjerdonek cjerdonek deleted the cjerdonek:editable-and-pep-517-optional branch Apr 6, 2019

@con-f-use

This comment has been minimized.

Copy link

commented Apr 8, 2019

@con-f-use original post:

I'm not at all sure if everything I wrote is correct and if the following is a good idea, but maybe, case 1.) should just display a warning instead of failing and the proceed with installing the build dependencies and not processing according to PEP517?

@cjerdonek response:

Basically, the issue is that the presence of pyproject.toml was meant to be the way for users to opt in to the new PEP 517 behavior. So allowing pyproject.toml to result in the old behavior would defeat / undermine that purpose. For example, if and when we add support for editable mode to PEP 517, we would then have a backwards compatibility issue if we were to let invocations happen today. This is the point that @pfmoore was making in his comment I linked to above (and that I agree with, for the same reasons). Yes, it will be a harder pill for users to swallow today (but it's still very early in PEP 517's adoption). But that pill will become a much bigger problem later on if we permit the old behavior to happen with pyproject.toml (without an explicit flag from the user like --no-use-pep517).

Another thing that crossed my mind is to have something like this:

# pyproject.toml
[build-system]
requires = ['some_requirement']

[tool.pip]
extra_options = ['--no-use-pep517']
# or:
no_pep517 = true

Not sure if it's a good idea either, but it would not break a Pipenv installation in the way I outlined in #6370 (comment) and would also not break backwards compatibility. Maybe make it more concrete with pip_no_pep517 = true so not open the whole can of worms that is exposing pip options.

@Evpok

This comment has been minimized.

Copy link

commented Apr 24, 2019

This breaks editable installs for people who use pyproject.toml for unrelated configuration (eg black). I think I understand the rationale, but going through a gentler deprecation path would have been nice.

@tom-dalton-fanduel

This comment has been minimized.

Copy link

commented Apr 24, 2019

Echoing the above, a bunch of our builds have started failing for the same reason - we use the new toml file purely for 2 lines of black configuration. We have a project structure like:

setup.py  # Contains the actual project/package requirements
pyproject.toml  # Contains only 2 lines of black configuration
requirements/base.txt  # Contains only `-e .`
requirements/testing.txt  # Contains `-r base.txt` and then other stuff like pytest etc.

This setup allows us to

  • Specify the project requirements exactly once (in the setup.py)
  • Use a command like pip install -r requirements/testing.txt to install both the project requirements along with other stuff needed to run the tests.
  • Build the package with python setup.py sdist

It's not immediately clear from the docs I've seen so far what the minimal change we need to make is - I'd guessing in the short term are options are to

  • Downgrade pip, or
  • Add the --no-use-pep517 to our build tool config

I'd echo the author above that a deprecation warning would have been nice here, but mostly I'd like to understand if there's a small change/addition we can make to the pyproject.toml file to resume previous behaviour. The examples I've seen so far use poetry or flit as example build tools, I'm surprised there's been no example using 'historical' tooling (e.g. pip/setuptools) so I'm wondering if I'm missing something here.

@con-f-use

This comment has been minimized.

Copy link

commented Apr 24, 2019

Just making unqualified comments here, but doesn't black have a specific config file other than pyproject.toml?

Btw. what was the reason again to not automatically fall back to the old setup.py build method, when there is a pyproject.toml with no build-backend and backend-path specified? I.e. implying --no-use-pep517 in thoses cases or at least try again with the old way (no virtualenv) before failing?
I feel this was discussed somewhere but can't find it. Not specifying a backend, not even build-backend='setuptools', basically screams "old way" to me.

@tom-dalton-fanduel

This comment has been minimized.

Copy link

commented Apr 24, 2019

Unfortunately (for me at least) black doesn't offer anything other than pyproject.toml. Opinions seem a bit split on it, personally I'm disappointed it doesn't offer setup.cfg as an option but I can understand the maintainer wanting to keep it simple. Ref python/black#683

@cjerdonek

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

I think I understand the rationale, but going through a gentler deprecation path would have been nice.

A little background on this: It was a bug / oversight that this was ever working. (It shouldn't have ever been working in the first place.) Yes, we know it's going to be an inconvenience for some that started using this. But PEP 517 is still a very new feature, and it would create much bigger problems further down the road if people started relying on this behavior more, which is why we wanted to stop it earlier while adoption is still small.

Btw. what was the reason again to not automatically fall back to setup.py when there is a pyproject.toml with no build-backend and backend-path specified, even without `--no-use-pep517? I feel this was discussed somewhere but can't find it.

Here are three comments with more detailed background / rationale:

The simplest work-around for people should be to add --no-use-pep517 to any invocation that requires editable mode (which I believe any error message you're seeing should say). Other options would be not to use editable mode, or not use a pyproject.toml file.

@gaborbernat

This comment has been minimized.

Copy link

commented Apr 24, 2019

In some cases editable install is desirable (Cython code coverage, development of a library), so the later two options are often no-go from the get-go. Will --no-use-pep517 work even when both build-backend and requires section is satisfied? Does this mean that the flag is now supported for the foreseeable future?

@tom-dalton-fanduel

This comment has been minimized.

Copy link

commented Apr 24, 2019

Hmm. I might be doing something wrong but the new flag doesn't seem to work for me:

pip install --no-use-pep517 -q --upgrade --requirement /var/lib/jenkins/workspace/EC2_SQS_Queue_Deletion/sqs-deletion/src/requirements/development.txt
ERROR: Error installing 'file:///var/lib/jenkins/workspace/EC2_SQS_Queue_Deletion/sqs-deletion (from -r /var/lib/jenkins/workspace/EC2_SQS_Queue_Deletion/sqs-deletion/src/requirements/_base.txt (line 1))':
  editable mode is not supported for pyproject.toml-style projects. pip is processing this project as pyproject.toml-style because it has a pyproject.toml file.
  Since the project has a setup.py and the pyproject.toml has no "build-backend" key for the "build_system" value, you may pass --no-use-pep517 to opt out of pyproject.toml-style processing.
  See PEP 517 for details on pyproject.toml-style projects.

make: *** [/var/lib/jenkins/workspace/EC2_SQS_Queue_Deletion/sqs-deletion/_build/requirements.txt] Error 1```
@tom-dalton-fanduel

This comment has been minimized.

Copy link

commented Apr 24, 2019

I'm going to open a new issue if only to prevent cluttering up this merged PR.

@tom-dalton-fanduel

This comment has been minimized.

Copy link

commented Apr 24, 2019

:) I'll add my info to the bug above

@con-f-use

This comment has been minimized.

Copy link

commented Apr 24, 2019

@gaborbernat Yes, mixed caeses were my concern, too, like:

echo "
-e pep517_project/
non-pep517_project/
" | pip install --no-use-pep517 -r /dev/stdin

would this work:

echo "
-e --no-use-pep517 pep517_project/
non-pep517_project/
" | pip install  -r /dev/stdin

I mean Pipenv and similar tools still have now way to pass options to pip on a per-package basis, but it be nice.

@pablogsal

This comment has been minimized.

Copy link

commented Apr 24, 2019

The simplest work-around for people should be to add --no-use-pep517 to any invocation that requires editable mode (which I believe any error message you're seeing should say)

That fails with:

ERROR: Disabling PEP 517 processing is invalid: project specifies a build backend of setuptools.build_meta in pyproject.toml

This seems to imply that PIP refuses to opt-out of PEP 517 if there is a build-backend in the pyproject.toml even if the environment allows to just run setup.py directly.

@cjerdonek

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

Will --no-use-pep517 work even when both build-backend and requires section is satisfied?

It will only work when PEP 517 doesn't mandate that pyproject.toml-style be used. If the "build-backend" key is present, the PEP says pyproject.toml-style must be used, so the flag won't work in that case.

The flag for the legacy behavior will eventually go away (that's the plan). Here's the tracking issue: #6334

@gaborbernat

This comment has been minimized.

Copy link

commented Apr 24, 2019

So I get now pip is PEP compliant, but this now breaks peoples workflow without any sane workaround, I consider it as a PEP oversight. PEP-517/8 on purpose did not want to cover editable installs, considering something to be addressed down the line. As such I would argue PEP-517/PEP-518 no longer applies when editable install mode is enabled. It's the build frontend dependent on how this case is handled. In such a case, pip should fallback to the old system, until we define editable installs inside a PEP. @pfmoore want to weigh in?

@tom-dalton-fanduel

This comment has been minimized.

Copy link

commented Apr 24, 2019

Will --no-use-pep517 work even when both build-backend and requires section is satisfied?

It will only work when PEP 517 doesn't mandate that pyproject.toml-style be used. If the "build-backend" key is present, the PEP says pyproject.toml-style must be used, so the flag won't work in that case.

I have no build-backend section in my pyproject.toml file but the --no-use-pep517 flag still doesnt appear to work as expected. See #6433 for details.

@tom-dalton-fanduel

This comment has been minimized.

Copy link

commented Apr 24, 2019

Raised #6433 for further discussion

jsignell added a commit to pyviz/pyctdev that referenced this pull request Apr 25, 2019

jsignell added a commit to pyviz/pyctdev that referenced this pull request Apr 25, 2019

Use python setup.py rather than editable pip (#11)
* Use python setup.py rather than editable pip

`pip install -e . --no-deps` started breaking with pypa/pip#6370 as seen in https://travis-ci.org/pyviz/panel/jobs/524005537#L1438

* Reverting conda  pin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.