Issue #6163: Temporary workaround for legacy setup.py files

[ncoghlan]

Temporary resolution of #6163 while the longer term fix in setuptools
is still undergoing design review.

[ncoghlan]

The new test cases are now passing (explicit backend) and failing (implicit backend) as expected.

Next step will be to tweak the handling of the implicit backend case to make that test pass without breaking anything else.

[ncoghlan]

@pradyunsg @pfmoore Something that does bother me a bit with the new tests is that the simplest way I found to get them to work was to let them have open access to PyPI in order to grab setuptools and wheel as build dependencies. Are there existing wheels for those stashed somewhere in the source tree that could be used instead?

[ncoghlan]

Latest push should get the new test passing by adding the source directory to sys.path in the implicit case. It's a pretty gross hack, since it tunnels "this was implicit" through to a patched version of Pep517HookCaller as a magic prefix on the build backend name, and the hook caller then passes the directory to insert as sys.path[0] through to the in-process wrapper via a new environment variable.

I think it may still be worth doing though, as the setuptools project will need to live with the resolution of pypa/setuptools#1652 indefinitely, whereas pip will only need to live with this interim fix until a setuptools release with the long term resolution is available (and the test cases added in this PR will make sure that the new legacy backend is working as desired).

[ncoghlan]

Noting what would be required to remove the workaround once the real fix in setuptools is available:

1. Keep the new test cases
2. Revert all the unmanaged local patches to the vendored pep517 code
3. Change the two references to pip._implicit.setuptools.build_meta to instead be setuptools.build_meta_legacy
4. Change the minimum setuptools version checks to whichever release provides build_meta_legacy

[pfmoore]

Could those reversion notes be added as a comment in the code somewhere, please?

(My reaction to this fix is "ewww" and I still don't think it'll be significantly faster to fix this here rather than in setuptools, but that's really down to @pradyunsg as RM to decide how fast he wants to release another version).

Also, can these tests be run against the in-progress setuptools fix? That would confirm if the non-test changes here are needed at all. (Note - I don't actually know how to run tests against a backend in an unreleased version of setuptools. That's a major issue I had with developing the PEP 517 code, and one that I never actually solved, but IMO we need a mechanism to do this, otherwise we'll never be able to properly co-ordinate changes between pip and the setuptools backend).

[ncoghlan]

@pfmoore I restructured all the changes to better mark the "This code is only here to work around issue #6163" sections, and added a block comment with the gory details to the code that sets the fallback build backend.

Unfortunately, testing against pre-release setuptools runs up against the problem that build dependencies are restricted to pre-built artifacts, so simply declaring a direct VCS URL references doesn't work.

However, I think resolving that ties in to my question above about having a way to run the new tests without needing to give them general access to PyPI: if I fix the test case to use the common_wheels directory rather than giving it access to PyPI, then it means that testing against pre-releases becomes possible by changing the entries in https://github.com/pypa/pip/blob/master/tools/tests-common_wheels-requirements.txt to be VCS URL references for the relevant branch.

[ncoghlan]

I tried my idea of modifying tests-common_wheels-requirements.txt locally, and in this particular case that runs into a bootstrapping problem: because tox is set up to use the pip-under-test to do the initial installation, that fails because the current setuptools doesn't have the new build backend available yet.

I then tried doing python3 -m pip wheel -w /tmp/setuptools_build_meta_legacy/ git+https://github.com/pganssle/setuptools@build_meta_legacy#egg=setuptools (with the intent of then specifying file:///tmp/setuptools_build_meta_legacy/[filename] in the test requirements), but setuptools has its own unique bootstrapping requirements that keep that from working.

The approach I finally got to work was to clone @pganssle's branch locally, run the bootstrap.py script, and then run pip wheel against that: python3 -m pip wheel --no-cache -w /tmp/setuptools_build_meta_legacy/ ./setuptools

That approach finally worked, but the actual tests hit the issue I noted in pypa/setuptools#1652 (comment), where the correct entry to add is an absolute path, not the current directory.

Amending the code with my suggested fix locally, rebuilding the test wheel, and re-running the PEP 517 functional tests got me a passing build again :)

[ncoghlan]

I posted #6212 to show what I'd expect the end state to look like after setuptools.build_meta_legacy has been released (minus the changes that are in there solely to enable local testing against a setuptools pre-release).

I still don't think it makes sense to put pressure on the setuptools project to resolve pypa/setuptools#1652 too quickly, though. As PaulG notes in #6163 (comment), the change in this PR is entirely internal to pip and can be removed (other than the new tests) once the proper fix is available, while the change on the setuptools side adds a new public API that they'll then need to support indefinitely, which isn't a decision that should be made in haste.

[ncoghlan]

Checking that this branch resolves the originally reported issue with PyInstaller:

[ncoghlan@localhost pip]$ python3 src/pip install --user PyInstaller==3.4
Collecting PyInstaller==3.4
  Using cached https://files.pythonhosted.org/packages/03/32/0e0de593f129bf1d1e77eed562496d154ef4460fd5cecfd78612ef39a0cc/PyInstaller-3.4.tar.gz
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
    Preparing wheel metadata ... done
Collecting altgraph (from PyInstaller==3.4)
  Downloading https://files.pythonhosted.org/packages/0a/cc/646187eac4b797069e2e6b736f14cdef85dbe405c9bfc7803ef36e4f62ef/altgraph-0.16.1-py2.py3-none-any.whl
Collecting pefile>=2017.8.1 (from PyInstaller==3.4)
  Downloading https://files.pythonhosted.org/packages/ed/cc/157f20038a80b6a9988abc06c11a4959be8305a0d33b6d21a134127092d4/pefile-2018.8.8.tar.gz (62kB)
    100% |████████████████████████████████| 71kB 3.4MB/s 
Collecting macholib>=1.8 (from PyInstaller==3.4)
  Downloading https://files.pythonhosted.org/packages/41/f1/6d23e1c79d68e41eb592338d90a33af813f98f2b04458aaf0b86908da2d8/macholib-1.11-py2.py3-none-any.whl
Requirement already satisfied: setuptools in /usr/local/lib/python3.7/site-packages (from PyInstaller==3.4) (39.0.1)
Collecting future (from pefile>=2017.8.1->PyInstaller==3.4)
  Downloading https://files.pythonhosted.org/packages/90/52/e20466b85000a181e1e144fd8305caf2cf475e2f9674e797b222f8105f5f/future-0.17.1.tar.gz (829kB)
    100% |████████████████████████████████| 829kB 5.5MB/s 
Building wheels for collected packages: PyInstaller
  Building wheel for PyInstaller (PEP 517) ... done
  Stored in directory: /home/ncoghlan/.cache/pip/wheels/09/ee/0a/94ef5d39074625f49e2e9cf97ac30cb7a87e1a7458ed195b8d
Successfully built PyInstaller
Installing collected packages: altgraph, future, pefile, macholib, PyInstaller
  Running setup.py install for future ... done
  Running setup.py install for pefile ... done
Successfully installed PyInstaller-3.4 altgraph-0.16.1 future-0.17.1 macholib-1.11 pefile-2018.8.8

[pganssle]

I like this "solution" better than rushing out a change to the public API in setuptools, but my vote is for just pushing the "Mere existence of pyproject.toml triggers PEP 517" change into the next version of pip, to give us more time to thoroughly test this.

Maybe this is the only change that needs to happen, but I don't think we're actually gaining anything from opting people in to PEP 517 right now, and if the build_meta_legacy stuff turns out to be trickier than it seems, we may end up accumulating interim compatibility hacks in pip for no reason.

The old code path is still going to exist for quite some time, so it's not a big hardship to slow the roll out of the new code path a little.

[pfmoore]

@pganssle If we make PEP 517 opt-in, how do you propose we get users to test whether it works with their configurations? The reason we made it opt-out was precisely this - unless we switch it on, we'll get no user feedback and we're simply delaying the point at which we find out any issues. (I'm not trying to block switching to opt-in if it's genuinely the best option, but I truly don't see how it helps us find out if the new code is any good, in any meaningful way).

[pganssle]

If we make PEP 517 opt-in, how do you propose we get users to test whether it works with their configurations?

I'm suggesting a temporary reversion and we'll manually test the major problems we've seen. If we push the PEP 517-as-default rollout to 19.1 or 19.2, we would also have time to set up the pre-release integration testing repo to do some more thorough testing before it hits users.

[ncoghlan]

@pfmoore I think there's enough "It broke X, Y, and Z" feedback from the past few days that can provide fodder for the design of new pip test cases that doing a 19.0.2 release that flipped the --use-pep517 default to "off" would be a viable way of getting things back to normal for most users, before taking another, more informed, run at switching back to opt-out behaviour in 19.1.

[pfmoore]

@ncoghlan You're probably right. I'm happy to go with that, although I worry that no-one will step up and actually write those extra tests. But if that means we simply don't progress with PEP 517 support as fast as I'd like, then I guess that's just a reality that we have to accept.

[ncoghlan]

@pfmoore As long as the resolutions of the reported bugs are gated on adding relevant tests to the test suite, it will hopefully be OK :)

[ncoghlan]

Closing in favour of #6229 (see #6163 (comment) for more details)

[ncoghlan]

Reopening, as the CI results on #6229 aren't promising - in the current code base, --no-use-pep517 also turns off build-backend.requires processing, which means any project that defines build-system.requires has to be installed via a PEP 517 backend, or the build dependencies won't get installed.

[pradyunsg]

@ncoghlan As for this PR implementation wise, patches to pip's vendored libraries have to stored in tasks/vendoring/patches. The rest looks fine to me.

[cjerdonek]

FYI, the MacOS failure is from a flaky test. See: #6043 (comment)

[pradyunsg]

I am okay with merging this -- as a temporary fix, if setuptools can't get a fix out in time -- but still prefer a setuptools-only fix.

[pganssle]

I am okay with merging this -- as a temporary fix, if setuptools can't get a fix out in time

I'm not sure what "in time" means here, is there a deadline? I think the time for "quick let's work around it so it doesn't cause a disruption" was some time last week. At this point it's less of a critical problem and more of an ongoing annoyance.

Assuming one of the other setuptools maintainers signs off on the build_meta_legacy direction in the next few days, will we be ready to cut a pip release taking advantage of it?

[pfmoore]

"in time" here means "before pip does its next release", plus a little bit of time to change the default backend in pip. IMO, I'd rather delay a pip release until the setuptools release is done - at this point I don't see much value in a "quick fix" pip that will only be around for a short while anyway.

If we'd released a new version of pip with this fix in it within days, that would have been different. But at this point I agree it's more of an ongoing problem now than a critical failure.

[pradyunsg]

Assuming one of the other setuptools maintainers signs off on the build_meta_legacy direction in the next few days, will we be ready to cut a pip release taking advantage of it?

Yes. I am going to cut a pip release, hopefully containing one of the fixes for this issue, in the coming week -- if setuptools releases the backend by early next week (~Feb 7/8), that'd be perfect.

edit: I can't seem to come up with a language that doesn't make this sound like a threat -- this isn't a threat/ultimatum etc. There's a bugfix release that I didn't cut yet and fixing this along with the other fixes, in that would be perfect.

[pradyunsg]

If we'd released a new version of pip with this fix in it within days, that would have been different. But at this point I agree it's more of an ongoing problem now than a critical failure.

Agreed -- I messed up my timeline a bit and the second bugfix for 19.0 isn't out yet (:() so if we can fit either a setuptools + legacy backend or this fix in pip 19.0.2, I'd prefer that to the alternative.

[ncoghlan]

As I've noted previously: my main concern isn't with professional build pipelines that can be fixed by pinning pip to a particular version (those folks are getting what they're paying for), it's with the interactive experience of folks that are currently finding that the installation tutorial on packaging.python.org doesn't work for projects that they're trying to use.

So I'd prefer to see this merged (thus ensuring that 19.0.2 will definitely ship with a resolution for #6163), and then I'll rework #6212 atop that foundation in anticipation of a proper setuptools release being available in time.

Declining to merge this one until the last minute because a setuptools release might be available by then seems like asking for unnecessary trouble.

[pradyunsg]

I missed the comment here @ncoghlan. Whoops! :(

None the less, we have a setuptools release now, so it'd probably make sense to directly complete #6212 for #6163.

[ncoghlan]

Aye, I actually meant to close this when I last updated #6212 - fixing that oversight now :)

[lock]

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.