Fix PEP 517 builds for packages without setup.py

[seppeljordan]

The idea of this patch is that we build a wheel and install that whell when we install a requirement from
source that does not support setup.py.
Fixes #6222.

I really want to see this happen, so please provide feedback in case you find any problems or possible improvements with this PR.

[pfmoore]

I haven't looked at this in any detail (and I'm not likely to have the time to in the next few weeks) but at a minimum, this needs tests.

[blaiseli]

I cloned this version of pip and installed it, and it enabled me to install flit, (and then pep8 which was my goal). With pip 19.1.1, I had ModuleNotFoundError: No module named 'setuptools' upon flit installation, even when directly trying to install flit, without --no-binary.

[seppeljordan]

I would appreciate help with this since I feel like this goes way beyond my knowledge. So if anybody would like to give me a proper code review, that would be great.

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

[seppeljordan]

Hi, could somebody please review this?

[seppeljordan]

BTW: There is now a functional test that would fail on master and passes with this patch. If you see further testing necessary, please let me know. cheers

[pradyunsg]

Thanks for this PR @seppeljordan (and your patience)!

I think we should be handling the wheel building earlier, so that such InstallRequirement objects flow through WheelBuilder for getting built into wheels; instead of waiting until install to build the wheel.

This is because we should have the "build a PEP 517 wheel" logic in only one location within the codebase (currently that's WheelBuilder._build_one_pep517) and the logic that you've provided is similar but not the same as that method, which makes the codebase difficult to mentally model.

If you're still interested in taking this forward, I would suggest that you investigate why these InstallRequirement objects are not being added to pep517_requirements in commands/install.py:InstallCommand.run. Once that understanding is built, we can then change code in a much more targeted manner, so that these do get added to pep517_requirements, and processed like a PEP 517 requirement.

[seppeljordan]

Thank you for the review, I will investigate and report back (hopefully with a patch).

[pradyunsg]

Awesome! Thank you!

[seppeljordan]

Hey, so I looked into the matter and it seems like InstallRequirement.load_pyproject_toml() is never called on detected requirements. So my question now would be, when is the appropriate time to call InstallRequirements.load_pyproject_toml(), since it seems to be allowed only in specific circumstances. src/pip/_internal/commands/install.py has the following section in it:

                legacy_requirements = []
                pep517_requirements = []
                for req in requirement_set.requirements.values():
                    if req.use_pep517:
                        pep517_requirements.append(req)
                    else:
                        legacy_requirements.append(req)

It looks to me like this is where we decide how to process which requirement type. But if I modify the section like so:

                legacy_requirements = []
                pep517_requirements = []
                for req in requirement_set.requirements.values():
                    req.load_pyproject_toml()
                    if req.use_pep517:
                        pep517_requirements.append(req)
                    else:
                        legacy_requirements.append(req)

we get an error. One would think that it is save to call this method here, since it is the last opportunity to do so before the path splits, but in fact you get an error:

Command executed: pip install --no-binary :all: flit

Collecting flit
  Using cached https://files.pythonhosted.org/packages/1f/87/9ea76ab4cdf1fd36710d9688ec36a0053067c47e753b32272f952ff206c5/flit-1.3.tar.gz
flit
(['intreehooks', 'requests', 'docutils', 'pytoml', "zipfile36; python_version in '3.3 3.4 3.5'"], 'intreehooks:loader', [])
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Installing backend dependencies ... done
    Preparing wheel metadata ... done
Requirement already satisfied: requests in ./venv/lib/python3.7/site-packages (from flit) (2.22.0)
Requirement already satisfied: docutils in ./venv/lib/python3.7/site-packages (from flit) (0.15.2)
Collecting pytoml
  Using cached https://files.pythonhosted.org/packages/f4/ba/98ee2054a2d7b8bebd367d442e089489250b6dc2aee558b000e961467212/pytoml-0.1.21.tar.gz
pytoml
None
Requirement already satisfied: chardet<3.1.0,>=3.0.2 in ./venv/lib/python3.7/site-packages (from requests->flit) (3.0.4)
Requirement already satisfied: urllib3!=1.25.0,!=1.25.1,<1.26,>=1.21.1 in ./venv/lib/python3.7/site-packages (from requests->flit) (1.25.5)
Requirement already satisfied: certifi>=2017.4.17 in ./venv/lib/python3.7/site-packages (from requests->flit) (2019.9.11)
Requirement already satisfied: idna<2.9,>=2.5 in ./venv/lib/python3.7/site-packages (from requests->flit) (2.8)
flit
(['intreehooks', 'requests', 'docutils', 'pytoml', "zipfile36; python_version in '3.3 3.4 3.5'"], 'intreehooks:loader', [])
ERROR: Exception:
Traceback (most recent call last):
  File "/home/sebastian/src/pip/src/pip/_internal/cli/base_command.py", line 153, in _main
    status = self.run(options, args)
  File "/home/sebastian/src/pip/src/pip/_internal/commands/install.py", line 402, in run
    req.load_pyproject_toml()
  File "/home/sebastian/src/pip/src/pip/_internal/req/req_install.py", line 526, in load_pyproject_toml
    self.pyproject_toml_path,
  File "/home/sebastian/src/pip/src/pip/_internal/req/req_install.py", line 511, in pyproject_toml_path
    assert self.source_dir, "No source dir for %s" % self
AssertionError: No source dir for requests in ./venv/lib/python3.7/site-packages (from flit)

Don't mind the debug output in between. The point is that we cannot execute load_pyproject_toml here...

Any ideas or thoughts on the subject?

@pradyunsg Could you provide contact to people who might know about this so I can ask them?

[pradyunsg]

That... would be me. :)

So, that method should get called in SourceDistribution.prepare_distribution_metadata, which is called in operations.prepare:_get_prepared_distribution, which should get called from RequirementPreparer.prepare_linked_requirement.

Could you look into why prepare_linked_requirement isn't calling into load_pyproject_toml? Or perhaps load_pyproject_toml isn't doing the right thing?

(this is off-the-top-of-my-head, so let me know if this isn't clear)

[seppeljordan]

Okay. I made some progress. You are right in that load_pyproject_toml is called in SourceDistribution.prepare_distribution_metadata. I think I found a solution, that I force-pushed to this branch. The basic idea is that when building the dependencies we fail to do so. From src/pip/_internal/wheel.py:

    if not check_binary_allowed(req):
        logger.info(
            "Skipping wheel build for %s, due to binaries "
            "being disabled for it.", req.name,
        )
        return None

So the proposed fix is that instead of disabling binaries for all packages when --no-binary :all: is supplied we now allow binaries for all InstallRequirements with bool(req.use_517) == True.

Please let me know if you think that this is a reasonable solution.

[seppeljordan]

Hi, I wrote a functional test. This test resides currently in tests/functional/test_install.py. To carry out the test I had to create a new testing package.

The test itself runs pip install with --no-binary :all: on the test package and checks the output for lines showing that a build was done. Unfortunately I needed to keep the index intact to actually build the package with setuptools.

[seppeljordan]

This whole process is kind of frustrating. I get it, you all have a lot of stuff to do... On the other hand, isn't it kind of embarrassing to have a package manager that can not install from source? I saw that even the release process is effected by this issue.

If this PR was not taken care of during october I will close this it since it causes real frustration every time I see it in my PR overview.

[chrahunt]

Hi @seppeljordan. Thank you for following up! I think that's the best way to get more eyes on this, and I'm sorry it wasn't more clear from our developer docs. I've created #7185 to help us tackle that gap in our processes.

[seppeljordan]

Thank you @pradyunsg @chrahunt

[pradyunsg]

@seppeljordan Really sorry that this got frustrating for you.

I failed to state this as clearly earlier: I'm grateful that you took the time to file this PR initially, explore how to fix this, consider my request to rethink the fix, took the time to explore the test suite and write tests, and overall made an excellent PR experience for me as a maintainer of this project. Thank you!

I've provided some additional context from my perspective below (I would've liked to polish up the following words more, but I have an assignment to submit in an hour, because I'm still in college).

---

The lack of a response in this PR wasn't due to a lack of interest (not from me) but rather the lack of maintainer/developer time on pip.

I'm one of the (two or three?) maintainers who are familiar enough with this part of pip, to be fairly sure of what a fix for this issue looks like. As @chrahunt pointed out, follow ups are likely the best way to get more eyes on a PR, especially mine -- that's how I'd noticed your PR on Sept 13 and taken time to review it on Sept 14.

A bit more context on my personal situation -- I reviewed ~80 PRs in all of Sept, which included this one; and that's when I'd set a goal to reduce the ratio of time I spend reviewing PRs, to make time for some refactoring work. There are ~950 GitHub unread notifications sitting in my Octobox instance right now, and pinging on a PR moves it higher in that queue, which makes it more likely that I actually review the PR. :)

Finally, I suggest anyone who's made excellent OSS contributions but gotten frustrated during the process, to take a look at https://snarky.ca/setting-expectations-for-open-source-participation/. If you know about it already, that's great! If not, please do take a look.

---

I do understand that this has been a frustrating experience. That said, if you'd be willing, I'd really appreciate it if you continue to contribute to pip. The amount of effort that you've put into understanding the codebase and communicating is great to see. :)