-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Install build requirements before calling setup.py #4799
Changes from 13 commits
d0c42d3
6a99774
615de6a
08bec25
958a936
27cb7f3
8dad780
d3d13ea
3b88968
74c086b
5bb4e76
b214425
12202ee
5c77300
8810500
1601ebb
14ae49d
5e16d51
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,3 +36,4 @@ coverage.xml | |
|
||
# Scratch Pad for experiments | ||
.scratch/ | ||
.vscode | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fix situation where build requirements are installed before calling setup.py. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need a news entry since this bug never actually made it to a release. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,3 +62,17 @@ def __exit__(self, exc_type, exc_val, exc_tb): | |
os.environ.pop('PYTHONPATH', None) | ||
else: | ||
os.environ['PYTHONPATH'] = self.save_pythonpath | ||
|
||
def cleanup(self): | ||
self._temp_dir.cleanup() | ||
|
||
|
||
class NoOpBuildEnvironment(BuildEnvironment): | ||
def __enter__(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: I should have overridden the initialization method here as well. |
||
pass | ||
|
||
def __exit__(self, exc_type, exc_val, exc_tb): | ||
pass | ||
|
||
def cleanup(self): | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,15 @@ | ||
"""Prepares a distribution for installation | ||
""" | ||
|
||
import itertools | ||
import logging | ||
import os | ||
import sys | ||
from copy import copy | ||
|
||
from pip._vendor import pkg_resources, requests | ||
|
||
from pip._internal.build_env import NoOpBuildEnvironment | ||
from pip._internal.compat import expanduser | ||
from pip._internal.download import ( | ||
is_dir_url, is_file_url, is_vcs_url, unpack_url, url_to_path, | ||
|
@@ -14,9 +18,14 @@ | |
DirectoryUrlHashUnsupported, HashUnpinned, InstallationError, | ||
PreviousBuildDirError, VcsHashUnsupported, | ||
) | ||
from pip._internal.index import FormatControl | ||
from pip._internal.req.req_install import InstallRequirement | ||
from pip._internal.utils.hashes import MissingHashes | ||
from pip._internal.utils.logging import indent_log | ||
from pip._internal.utils.misc import display_path, normalize_path | ||
from pip._internal.utils.misc import ( | ||
call_subprocess, display_path, normalize_path, | ||
) | ||
from pip._internal.utils.ui import open_spinner | ||
from pip._internal.vcs import vcs | ||
|
||
logger = logging.getLogger(__name__) | ||
|
@@ -38,6 +47,23 @@ def make_abstract_dist(req): | |
return IsSDist(req) | ||
|
||
|
||
def _install_build_reqs(finder, prefix, build_requirements): | ||
finder = copy(finder) | ||
finder.format_control = FormatControl(set(), set([":all:"])) | ||
urls = [ | ||
finder.find_requirement( | ||
InstallRequirement.from_line(r), upgrade=False).url | ||
for r in build_requirements | ||
] | ||
args = [ | ||
sys.executable, '-m', 'pip', 'install', '--ignore-installed', | ||
'--prefix', prefix | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: trailing comma |
||
] + list(urls) | ||
|
||
with open_spinner("Installing build dependencies") as spinner: | ||
call_subprocess(args, show_stdout=False, spinner=spinner) | ||
|
||
|
||
class DistAbstraction(object): | ||
"""Abstracts out the wheel vs non-wheel Resolver.resolve() logic. | ||
|
||
|
@@ -64,7 +90,7 @@ def dist(self, finder): | |
"""Return a setuptools Dist object.""" | ||
raise NotImplementedError(self.dist) | ||
|
||
def prep_for_dist(self): | ||
def prep_for_dist(self, finder): | ||
"""Ensure that we can get a Dist for this requirement.""" | ||
raise NotImplementedError(self.dist) | ||
|
||
|
@@ -75,7 +101,7 @@ def dist(self, finder): | |
return list(pkg_resources.find_distributions( | ||
self.req.source_dir))[0] | ||
|
||
def prep_for_dist(self): | ||
def prep_for_dist(self, finder): | ||
# FIXME:https://github.com/pypa/pip/issues/1112 | ||
pass | ||
|
||
|
@@ -91,17 +117,35 @@ def dist(self, finder): | |
) | ||
return dist | ||
|
||
def prep_for_dist(self): | ||
self.req.run_egg_info() | ||
self.req.assert_source_matches_version() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Specifically here. |
||
def prep_for_dist(self, finder): | ||
# Before calling "setup.py egg_info", we need to set-up the build | ||
# environment. | ||
|
||
build_requirements, isolate = self.req.get_pep_518_info() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A comment here saying that this is for installing stuff into a PEP 518 environment would be nice. :) |
||
|
||
if 'setuptools' not in build_requirements: | ||
logger.warning( | ||
"This version of pip does not implement PEP 516, so " | ||
"it cannot build a wheel without setuptools. You may need to " | ||
"upgrade to a newer version of pip.") | ||
|
||
if not isolate: | ||
self.req.build_env = NoOpBuildEnvironment(no_clean=False) | ||
|
||
with self.req.build_env as prefix: | ||
if isolate: | ||
_install_build_reqs(finder, prefix, build_requirements) | ||
|
||
self.req.run_egg_info() | ||
self.req.assert_source_matches_version() | ||
|
||
|
||
class Installed(DistAbstraction): | ||
|
||
def dist(self, finder): | ||
return self.req.satisfied_by | ||
|
||
def prep_for_dist(self): | ||
def prep_for_dist(self, finder): | ||
pass | ||
|
||
|
||
|
@@ -259,14 +303,14 @@ def prepare_linked_requirement(self, req, session, finder, | |
(req, exc, req.link) | ||
) | ||
abstract_dist = make_abstract_dist(req) | ||
abstract_dist.prep_for_dist() | ||
abstract_dist.prep_for_dist(finder) | ||
if self._download_should_save: | ||
# Make a .zip of the source_dir we already created. | ||
if req.link.scheme in vcs.all_schemes: | ||
req.archive(self.download_dir) | ||
return abstract_dist | ||
|
||
def prepare_editable_requirement(self, req, require_hashes): | ||
def prepare_editable_requirement(self, req, require_hashes, finder): | ||
"""Prepare an editable requirement | ||
""" | ||
assert req.editable, "cannot prepare a non-editable req as editable" | ||
|
@@ -284,7 +328,7 @@ def prepare_editable_requirement(self, req, require_hashes): | |
req.update_editable(not self._download_should_save) | ||
|
||
abstract_dist = make_abstract_dist(req) | ||
abstract_dist.prep_for_dist() | ||
abstract_dist.prep_for_dist(finder) | ||
|
||
if self._download_should_save: | ||
req.archive(self.download_dir) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
#!/usr/bin/env python | ||
from setuptools import find_packages, setup | ||
|
||
import simple # Test gh-4647 regression | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you make a separate test please? A single test should only try to test one thing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test was incorrect when it was initially written. There is no need to have a broken test and a correct test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah right. I think the comment can be changed to "ensure dependency is installed". |
||
|
||
setup(name='pep518', | ||
version='3.0', | ||
packages=find_packages() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change.