Fallback to pep517 if setup.py is present and setuptools cannot be imported by hrnciar · Pull Request #10717 · 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

Conversation33 Commits5 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 }})

This is WIP PR without any documentation or tests. I'd like to first know your opinion whether the idea is acceptable. Thank you.

I'm inclined to agree with the suggestion @uranusjr made on Discourse - if there's no setup.py and setuptools cannot be imported, the only possible thing that will work is to use PEP 517, so why not do that? Which means this PR can simply be "use PEP 517 if setuptools cannot be imported".

I'd argue that when the user explicitly used --no-pep517 and setuptools is not installed, that deserves an error.

That combined basically means something like:

    ... elif not has_setuptools:
        if use_pep517 is not None and not use_pep517:
            raise InstallationError(
                "Disabling PEP 517 processing is invalid: "
                "setuptools not installed"
            )
        use_pep517 = True

The “can’t do --no-use-pep517 because setuptools is not found” error can even be raised as early as command argument parsing, instead of half-way into package installation.

elif use_pep517 is None:
use_pep517 = has_pyproject
use_pep517 = has_pyproject or not importlib.util.find_spec("setuptools")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the setuptools check can be technically made much sooner in this function, but this way, it is avoided when not necessary. Not sure which approach is better.

FWIW, I was hoping to go down a different avenue here: dc217bb

That presents a clearer error message instead of opting these projects into PEP 517.

At least to me, opting to PEP 517 when setuptools is not installed sounds like a more user-friendly thing to do than erroring out (and gradually downloading a lesser and lesser version of the package only to error again and again).

Yea... #10655 is annoying.

It seems to me that there's reasonable agreement that we should stop backtracking on build failures, which combined with better presentation of build failures (I'll file this as a PR once #10703 merges) could be a more "complete" solution here; rather than needing to change this one special case?

Even without backtracking, I still think that using PEP 517 when setuptools is not present is the nicer thing to do, but I won't fight you on that. A better error report and no backtracking makes this good enough for me.

Thinking about this a bit more, I think we should do all three things: this PR, #10722 and #10723. They're all better behaviours, and I think it's totally reasonable for us to do all of them.

I was trying to solve the CI issues, but I ran into some test failures. I don't think they are caused by this PR, because I am getting the same failures when I run the tests on the main branch locally. If that's the case, I think PR is ready for review.

hrnciar marked this pull request as ready for review