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

Issue #6163: Postpone using PEP 517 by default #6229

Conversation

ncoghlan
Copy link
Member

@ncoghlan ncoghlan commented Feb 1, 2019

  • Temporarily changes the mode selection logic to
    only implicitly use PEP 517 if the pyproject.toml
    build-system table is present

  • Adds sys.path test cases for 3 pyproject.toml scenarios:

    • no build-system section
    • build-system section without build-backend
    • build-system section with build-backend

* Temporarily changes the mode selection logic to
  only implicitly use PEP 517 if the pyproject.toml
  build-system.build-backend key is set

* Adds sys.path test cases for 3 pyproject.toml scenarios:
  - no build-system section
  - build-system section without build-backend
  - build-system section with build-backend
@ncoghlan
Copy link
Member Author

ncoghlan commented Feb 1, 2019

Hmm, I just realised "has a build-system section" is going to be a better opt-in than "has a build-backend entry", as otherwise build-system.requires may behave badly.

@pfmoore
Copy link
Member

pfmoore commented Feb 1, 2019

I'm getting confused as to the various proposals going round here. There's this, the one about switching to setuptools.build_meta_legacy, and another one (I think). Could you possibly summarise somewhere, which are alternatives, which would go in together, and what your preferred approach is? Also, which of the reported issues would be addressed by each approach.

@ncoghlan
Copy link
Member Author

ncoghlan commented Feb 1, 2019

@pfmoore Yeah, currently writing that summary on #6163 (then I'm going to close the other PRs)

@pganssle
Copy link
Member

pganssle commented Feb 1, 2019

Nick is summarizing, but I think this one is orthogonal to the other proposals, right? This is the "postpone the PEP 517 opt-in until 19.1 to buy us time" PR.

I would like to get the integration-tests repo up and going well before the 19.1 release, so we can try to more extensively test the interplay between setuptools and pip well before then.

@ncoghlan
Copy link
Member Author

ncoghlan commented Feb 1, 2019

I narrowed the change in defaults even further such that even an empty [build-system] table is still enough to invoke the PEP 517/518 execution path.

There's one test that I marked as a strict xfail, which is the one that specifically tests that a blank pyproject.toml file runs in PEP 517/518 mode. We don't want to change that test - we just want to disable it until the workaround for #6163 is removed, and then ensure it gets re-enabled once it is passing again.

@pganssle
Copy link
Member

pganssle commented Feb 1, 2019

I narrowed the change in defaults even further such that even an empty [build-system] table is still enough to invoke the PEP 517/518 execution path.

Why? I think we should make the opt-in condition that build-system.build-backend is specified. This change will certainly reduce the scope of the problem, but I think we can be patient about the opt-in behavior, considering the plan is to extensively integration test this in 19.1. Doing a weak opt-in will just annoy people and confuse them about when pep 517 applies.

@ncoghlan
Copy link
Member Author

ncoghlan commented Feb 2, 2019

@pganssle If [build-system] exists, then the project developers aren't just adding settings to pyproject.toml for an unrelated project that is using the tools table to store its settings - they're actually trying to use the PEP 518/517 build system.

The pip test suite already had some test cases for that: one with build-system.requires, one with an empty build-system, and one with a blank pyproject.toml file.

All 3 initially failed with this change, so I got CI passing again by having the first two continue to opt-in to build isolation, while marking the 3rd test as an expected failure (which will start passing again once the default behaviour is able to be changed back to running an isolated build with the setuptools.build_meta_legacy backend).

@ncoghlan
Copy link
Member Author

ncoghlan commented Feb 2, 2019

Details on the failures reported by Travis:

  1. On Python 3.8-dev, the pyparsing deprecation warning for an invalid \w escape sequence has been updated to a full SyntaxWarning: https://travis-ci.org/pypa/pip/jobs/487508224
  2. The pyp3.5-6.0 download failed: https://travis-ci.org/pypa/pip/jobs/487508213

@pganssle
Copy link
Member

pganssle commented Feb 2, 2019

Eh, I don't think this is worth doing at all at this point. Probably we missed our window already and most people have permanently hard-coded --no-use-pep517 into their installation workflows by now.

This version of the "fix" will still break a bunch of stuff, including setuptools itself. Might as well just live with the fact that pip 19 broke backwards compatibility.

@ncoghlan
Copy link
Member Author

ncoghlan commented Feb 2, 2019

@pganssle Adding the "How are imports from setup.py handled?" test cases is critical if anyone is ever going to be able to migrate such projects to PEP 517.

@pganssle
Copy link
Member

pganssle commented Feb 2, 2019

Adding the "How are imports from setup.py handled?" test cases is critical if anyone is ever going to be able to migrate such projects to PEP 517

I meant the postponement of the pep 517 default. The other stuff is just pip's unit tests, and I don't really know or care about that.

@ncoghlan
Copy link
Member Author

ncoghlan commented Feb 2, 2019

There's still going to be a window of weeks or months where pip 19 will define the behaviour of interactive installs (as opposed to automated CI workflows).

@ncoghlan
Copy link
Member Author

ncoghlan commented Feb 2, 2019

@pfmoore @pradyunsg What are the consequences of allowing build-system.requires without opting the affected project in to the PEP 517 backend? Is there a way to request the use of build isolation, but still execute setup.py directly in that environment?

(The way I've structured this PR is based on the assumption that that isn't an available option)

Otherwise I'm going to have to resurrect PR #6210, as that currently seems to be the only proposed workaround for the compatibility break that actually works, since it retains the build isolation, while adding back the source directory to sys.path[0], without having to wait for setuptools.build_meta_legacy to be available.

@pganssle
Copy link
Member

pganssle commented Feb 2, 2019

What are the consequences of allowing build-system.requires without opting the affected project in to the PEP 517 backend? Is there a way to request the use of build isolation, but still execute setup.py directly in that environment?

This is precisely what pip 18 does, which is why there are a lot of projects out there that have build-system.requires but don't have build-backend, and why a bunch of projects would break if you opted them in to PEP 517. It's also why I suggested it in the first place.

@ncoghlan
Copy link
Member Author

ncoghlan commented Feb 2, 2019

Ah, cool - I'd missed that pip 18 was already behaving that way. In that case, I'll rework this to mark all three test cases as expected failures, and go back to the original simple default.

@ncoghlan
Copy link
Member Author

ncoghlan commented Feb 2, 2019

Ah, no - there were a lot more than 3 failures in the initial draft where even build-system.requires didn't trigger the use of a PEP 517 backend.

I've pushed that broken version now, so folks can see the test_install failures for themselves in the CI results.

By contrast, the original "add the source directory back to sys.path[0] when implicitly falling back to setuptools.build_meta" workaround in #6210 didn't have any weird side effects on test_install - it just worked as expected.

@ncoghlan
Copy link
Member Author

ncoghlan commented Feb 2, 2019

Reviewing the failures, I believe the issue is that when the PEP 517 support was added, the original PEP-518-only support was removed. This means that if PEP 517 gets turned off, then build-system.requires doesn't get processed either.

ncoghlan added a commit to ncoghlan/pip that referenced this pull request Feb 2, 2019
@ncoghlan
Copy link
Member Author

ncoghlan commented Feb 2, 2019

Closing this one as it turned out that it unfortunately doesn't resolve the original issue - disabling PEP 517 means affected projects that are relying on both PEP 518 and local imports from setup.py still don't work, just for a different reason.

Instead, we'll go back to the original plan, with #6210 as an interim workaround for pip 19.0.2 that should get most install scripts working again, and then the longer term solution with setuptoools.build_meta_legacy in #6212.

@cjerdonek cjerdonek added the C: PEP 517 impact Affected by PEP 517 processing label Feb 2, 2019
@pradyunsg pradyunsg closed this Feb 3, 2019
@pradyunsg
Copy link
Member

@ncoghlan I think you missed clicking the close button so I did that in my end. :)

@lock
Copy link

lock bot commented May 30, 2019

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.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: PEP 517 impact Affected by PEP 517 processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants