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 PEP 517 builds for packages without setup.py #6606

Merged
merged 11 commits into from Oct 17, 2019

Conversation

@seppeljordan
Copy link
Contributor

seppeljordan commented Jun 14, 2019

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.

@seppeljordan seppeljordan force-pushed the seppeljordan:issue-6222 branch 5 times, most recently to 1966934 Jun 14, 2019
@pfmoore

This comment has been minimized.

Copy link
Member

pfmoore commented Jun 14, 2019

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

This comment has been minimized.

Copy link

blaiseli commented Jul 3, 2019

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

This comment has been minimized.

Copy link
Contributor Author

seppeljordan commented Jul 14, 2019

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

This comment has been minimized.

Copy link
Contributor

BrownTruck commented Jul 26, 2019

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

This comment has been minimized.

Copy link
Contributor Author

seppeljordan commented Sep 9, 2019

Hi, could somebody please review this?

@seppeljordan

This comment has been minimized.

Copy link
Contributor Author

seppeljordan commented Sep 11, 2019

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

Copy link
Member

pradyunsg left a comment

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

This comment has been minimized.

Copy link
Contributor Author

seppeljordan commented Sep 14, 2019

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

@pradyunsg

This comment has been minimized.

Copy link
Member

pradyunsg commented Sep 14, 2019

Awesome! Thank you!

@seppeljordan seppeljordan force-pushed the seppeljordan:issue-6222 branch from a04dd3c to d0f6cf7 Sep 22, 2019
@seppeljordan

This comment has been minimized.

Copy link
Contributor Author

seppeljordan commented Sep 22, 2019

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

This comment has been minimized.

Copy link
Member

pradyunsg commented Sep 22, 2019

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 seppeljordan force-pushed the seppeljordan:issue-6222 branch from d0f6cf7 to f44da3b Sep 22, 2019
@seppeljordan

This comment has been minimized.

Copy link
Contributor Author

seppeljordan commented Sep 22, 2019

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 seppeljordan force-pushed the seppeljordan:issue-6222 branch from f44da3b to e9ea396 Sep 22, 2019
@seppeljordan seppeljordan force-pushed the seppeljordan:issue-6222 branch from 12ca306 to 58eb90f Sep 25, 2019
@seppeljordan

This comment has been minimized.

Copy link
Contributor Author

seppeljordan commented Sep 25, 2019

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 seppeljordan force-pushed the seppeljordan:issue-6222 branch from 24d0fe0 to 483daab Oct 12, 2019
@seppeljordan

This comment has been minimized.

Copy link
Contributor Author

seppeljordan commented Oct 12, 2019

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

This comment has been minimized.

Copy link
Member

chrahunt commented Oct 13, 2019

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.

@pradyunsg pradyunsg changed the title Implement requirement installation for PEP 517 conform packages without setup.py Fix PEP 517 builds for packages without setup.py Oct 17, 2019
@pradyunsg pradyunsg merged commit 2e9f89e into pypa:master Oct 17, 2019
24 checks passed
24 checks passed
🤖 (ubuntu-18.04, docs) 🤖 (ubuntu-18.04, docs)
Details
🤖 (ubuntu-18.04, lint) 🤖 (ubuntu-18.04, lint)
Details
Linux Build #20191012.4 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 Python35) Test Secondary Python35 succeeded
Details
Linux (Test Secondary Python37) Test Secondary Python37 succeeded
Details
Windows Build #20191012.4 succeeded
Details
Windows (Package) Package succeeded
Details
Windows (Test Primary Python27-x86) Test Primary Python27-x86 succeeded
Details
Windows (Test Primary Python37-x64) Test Primary Python37-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-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 #20191012.4 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 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
@seppeljordan

This comment has been minimized.

Copy link
Contributor Author

seppeljordan commented Oct 17, 2019

Thank you @pradyunsg @chrahunt

@pradyunsg

This comment has been minimized.

Copy link
Member

pradyunsg commented Oct 17, 2019

@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. :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.