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

PEP 517 implementation #5743

Merged
merged 30 commits into from
Nov 15, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
744b8cf
improve build environment
benoit-pierre Oct 25, 2018
de4d503
Phase 1 - build wheels using PEP 517 hook
pfmoore Aug 16, 2018
14f35f9
Experimental fix to pep517 to use pip's subprocess caller
pfmoore Aug 17, 2018
8fbf78d
Phase 2 - generate metadata using PEP 517 hook
pfmoore Aug 20, 2018
4de2915
Update required setuptools version for PEP 517
pfmoore Aug 24, 2018
b9e92a7
With build isolation, we shouldn't check if wheel is installed to dec…
pfmoore Aug 27, 2018
b62284a
Build PEP 517 and legacy wheels separately
pfmoore Aug 27, 2018
48e9cb6
Address test failures
pfmoore Aug 28, 2018
ab3e216
Add a news file
pfmoore Aug 28, 2018
a82b7ce
Fix test_pep518_with_user_pip which was getting errors due to irrelev…
pfmoore Aug 28, 2018
4281bf8
Correct an out of date comment
pfmoore Aug 28, 2018
c8d8e37
Fix copy and paste error
pfmoore Sep 7, 2018
c0ed438
Fix for test_install_no_binary_disables_building_wheels
pfmoore Sep 7, 2018
41b07c9
Include backend-provided requirements in build environment
pfmoore Oct 9, 2018
9d2b178
Add --[no-]use-pep517 command line flag
pfmoore Oct 9, 2018
f40491b
Vendor the new version of pep517
pfmoore Oct 9, 2018
83979fe
Actually use the new --[no-]use-pep517 option
pfmoore Oct 9, 2018
f10be25
PEP 517 tests
pfmoore Oct 10, 2018
3c94d81
Support --python-tag for PEP 517 builds
pfmoore Oct 16, 2018
a4c7d7d
Add documentation
pfmoore Oct 19, 2018
f805ac1
Properly wrap all hook calls in our subprocess runner
pfmoore Oct 19, 2018
689f97c
fix failing tests
benoit-pierre Oct 26, 2018
817ef1a
add a couple of additional PEP 517 tests
benoit-pierre Oct 26, 2018
4ca38e0
Merge with master
pfmoore Nov 11, 2018
cf4d84e
Address doc review comments
pfmoore Nov 14, 2018
e8f7aa1
Pass use_pep517 option to resolver
pfmoore Nov 14, 2018
3a0f9b1
Remove unneeded TODO
pfmoore Nov 14, 2018
6b7473d
Pass --use-pep517 option to the resolver in the pip wheel command
pfmoore Nov 14, 2018
85e4f8e
Fix some remaining TODO comments
pfmoore Nov 14, 2018
f06a0cb
Move setup.py egg_info logging into run_egg_info
pfmoore Nov 14, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Phase 2 - generate metadata using PEP 517 hook
  • Loading branch information
pfmoore authored and benoit-pierre committed Oct 29, 2018
commit 8fbf78d407543753da938874dcd0b836eb8f47fb
18 changes: 18 additions & 0 deletions src/pip/_internal/commands/install.py
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,24 @@ def run(self, options, args):
session=session, autobuilding=True
)

# If we're using PEP 517, we cannot do a direct install
# so we fail here.
# TODO: Technically, if it's a setuptools-based project
# we could fall back to setup.py install even if we've
# been assuming PEP 517 to this point, but that would be
# complicated to achieve, as none of the legacy setup has
# been done. Better to get the user to specify
# --no-use-pep517.
failed_builds = [
r for r in requirement_set.requirements.values()
if r.use_pep517 and not r.is_wheel
]

if failed_builds:
raise InstallationError(
"Could not build wheels for {}".format(
failed_builds))

to_install = resolver.get_installation_order(
requirement_set
)
Expand Down
2 changes: 1 addition & 1 deletion src/pip/_internal/operations/prepare.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ def _raise_conflicts(conflicting_with, conflicting_reqs):
" and ".join(map(repr, sorted(missing)))
)

self.req.run_egg_info()
self.req.prepare_metadata()
self.req.assert_source_matches_version()


Expand Down
115 changes: 86 additions & 29 deletions src/pip/_internal/req/req_install.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ def __init__(self, req, comes_from, source_dir=None, editable=False,
self.isolated = isolated
self.build_env = NoOpBuildEnvironment()

# For PEP 517, the directory where we request the project metadata
# gets stored. We need this to pass to build_wheel, so the backend
# can ensure that the wheel matches the metadata (see the PEP for
# details).
self.metadata_directory = None

# The static build requirements (from pyproject.toml)
self.pyproject_requires = None

Expand Down Expand Up @@ -311,6 +317,14 @@ def _correct_build_location(self):
self.source_dir = os.path.normpath(os.path.abspath(new_location))
self._egg_info_path = None

# Correct the metadata directory, if it exists
if self.metadata_directory:
old_meta = self.metadata_directory
rel = os.path.relpath(old_meta, start=old_location)
new_meta = os.path.join(new_location, rel)
new_meta = os.path.normpath(os.path.abspath(new_meta))
self.metadata_directory = new_meta

def remove_temporary_source(self):
"""Remove the source files from this requirement, if they are marked
for deletion"""
Expand Down Expand Up @@ -437,7 +451,12 @@ def load_pyproject_toml(self):
self.pyproject_requires = requires
self.pep517_backend = Pep517HookCaller(self.setup_py_dir, backend)

def run_egg_info(self):
def prepare_metadata(self):
"""Ensure that project metadata is available.

Under PEP 517, call the backend hook to prepare the metadata.
Under legacy processing, call setup.py egg-info.
"""
assert self.source_dir
if self.name:
logger.debug(
Expand All @@ -451,26 +470,10 @@ def run_egg_info(self):
)

with indent_log():
script = SETUPTOOLS_SHIM % self.setup_py
base_cmd = [sys.executable, '-c', script]
if self.isolated:
base_cmd += ["--no-user-cfg"]
egg_info_cmd = base_cmd + ['egg_info']
# We can't put the .egg-info files at the root, because then the
# source code will be mistaken for an installed egg, causing
# problems
if self.editable:
egg_base_option = []
if self.use_pep517:
self.prepare_pep517_metadata()
else:
egg_info_dir = os.path.join(self.setup_py_dir, 'pip-egg-info')
ensure_dir(egg_info_dir)
egg_base_option = ['--egg-base', 'pip-egg-info']
with self.build_env:
call_subprocess(
egg_info_cmd + egg_base_option,
cwd=self.setup_py_dir,
show_stdout=False,
command_desc='python setup.py egg_info')
self.run_egg_info()

if not self.req:
if isinstance(parse_version(self.metadata["Version"]), Version):
Expand All @@ -489,13 +492,56 @@ def run_egg_info(self):
metadata_name = canonicalize_name(self.metadata["Name"])
if canonicalize_name(self.req.name) != metadata_name:
logger.warning(
'Running setup.py (path:%s) egg_info for package %s '
'Generating metadata for package %s '
'produced metadata for project name %s. Fix your '
'#egg=%s fragments.',
self.setup_py, self.name, metadata_name, self.name
self.name, metadata_name, self.name
)
self.req = Requirement(metadata_name)

def prepare_pep517_metadata(self):
assert self.pep517_backend is not None

metadata_dir = os.path.join(
self.setup_py_dir,
'pip-wheel-metadata'
Copy link
Member

Choose a reason for hiding this comment

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

Having played around a bit, perhaps we should use a different name here: pip-wheel-metadata stands out... Perhaps a . at the start and perhaps even a sub-folder .pip-metadata/wheel would be a good idea?

Idk. It's a little subjective but I expect this to be something that shows up in development constantly so we should try to optimize the ergonomics here.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps build/pip-wheel-metadata?

Copy link
Member

Choose a reason for hiding this comment

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

I like how build/pip-wheel-metadata would look here.

)
ensure_dir(metadata_dir)

with self.build_env:
# Note that Pep517HookCaller implements a fallback for
# prepare_metadata_for_build_wheel, so we don't have to
# consider the possibility that this hook doesn't exist.
backend = self.pep517_backend
distinfo_dir = backend.prepare_metadata_for_build_wheel(
metadata_dir
)

self.metadata_directory = os.path.join(metadata_dir, distinfo_dir)

def run_egg_info(self):
script = SETUPTOOLS_SHIM % self.setup_py
base_cmd = [sys.executable, '-c', script]
if self.isolated:
base_cmd += ["--no-user-cfg"]
egg_info_cmd = base_cmd + ['egg_info']
# We can't put the .egg-info files at the root, because then the
# source code will be mistaken for an installed egg, causing
# problems
if self.editable:
egg_base_option = []
else:
egg_info_dir = os.path.join(self.setup_py_dir, 'pip-egg-info')
ensure_dir(egg_info_dir)
egg_base_option = ['--egg-base', 'pip-egg-info']
with self.build_env:
call_subprocess(
egg_info_cmd + egg_base_option,
cwd=self.setup_py_dir,
show_stdout=False,
command_desc='python setup.py egg_info')


@property
def egg_info_path(self):
if self._egg_info_path is None:
Expand Down Expand Up @@ -556,13 +602,23 @@ def metadata(self):
return self._metadata

def get_dist(self):
"""Return a pkg_resources.Distribution built from self.egg_info_path"""
egg_info = self.egg_info_path.rstrip(os.path.sep)
base_dir = os.path.dirname(egg_info)
metadata = pkg_resources.PathMetadata(base_dir, egg_info)
dist_name = os.path.splitext(os.path.basename(egg_info))[0]
return pkg_resources.Distribution(
os.path.dirname(egg_info),
"""Return a pkg_resources.Distribution for this requirement"""
if self.metadata_directory:
Copy link
Member

Choose a reason for hiding this comment

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

Break this up into 2 separate functions that get called by this one?

Copy link
Member

Choose a reason for hiding this comment

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

I think this can be simplified to:

    def get_dist(self):
        """Return a pkg_resources.Distribution for this requirement"""
        if self.metadata_directory:
            metadata_path = self.metadata_directory
        else:
            metadata_path = self.egg_info_path
        metadata_path = metadata_path.rstrip(os.path.sep)
        metadata = pkg_resources.PathMetadata(
            os.path.dirname(metadata_path), metadata_path)
        return pkg_resources.Distribution.from_filename(
            metadata_path, metadata=metadata)

Copy link
Member

Choose a reason for hiding this comment

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

Or even better:

    def get_dist(self):
        """Return a pkg_resources.Distribution for this requirement"""
        if self.metadata_directory:
            metadata_path = self.metadata_directory
        else:
            metadata_path = self.egg_info_path
        metadata_path = metadata_path.rstrip(os.path.sep)
        return next(pkg_resources.distributions_from_metadata(metadata_path))

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm OK with this - but don't really know the pkg_resources functions well enough to judge the impact. I'll wait for @pradyunsg to do his review, and then I'm happy to just replace the existing get_dist with this one.

On the other hand, it would also be just as easy to put in what we have, and then follow up with a PR simplifying get_dist (in the interest of smaller more focused PRs being better). But as long as making the change doesn't trigger a new wave of weird CI failures, it doesn't really make much difference - this PR is already way too complicated, one more commit won't make much difference 😉

Copy link
Member

Choose a reason for hiding this comment

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

I'm on board for making more follow up PRs for cleanups. This PR is already a little unwieldy in size and it's taken me a lot longer than I estimated to even try it out.

I've bumped existing notes just so that I don't have to scroll too high when taking a pass later to see what places we've already identified for improvements/refactors.

base_dir, distinfo = os.path.split(self.metadata_directory)
metadata = pkg_resources.PathMetadata(
base_dir, self.metadata_directory
)
dist_name = os.path.splitext(distinfo)[0]
typ = pkg_resources.DistInfoDistribution
else:
egg_info = self.egg_info_path.rstrip(os.path.sep)
base_dir = os.path.dirname(egg_info)
metadata = pkg_resources.PathMetadata(base_dir, egg_info)
dist_name = os.path.splitext(os.path.basename(egg_info))[0]
typ = pkg_resources.Distribution

return typ(
base_dir,
project_name=dist_name,
metadata=metadata,
)
Expand Down Expand Up @@ -693,6 +749,7 @@ def _clean_zip_name(self, name, prefix): # only used by archive.

# TODO: Investigate if this should be kept in InstallRequirement
# Seems to be used only when VCS + downloads
# TODO: Consider PEP 517 implications
def archive(self, build_dir):
assert self.source_dir
create_archive = True
Expand Down
5 changes: 2 additions & 3 deletions src/pip/_internal/wheel.py
Original file line number Diff line number Diff line change
Expand Up @@ -702,7 +702,7 @@ def _build_one_pep517(self, req, tempd, python_tag=None):
spin_message = 'Running PEP 517 build_wheel for %s' % (req.name,)
with open_spinner(spin_message) as spinner:
logger.debug('Destination directory: %s', tempd)
# assert req.metadata_directory is not None
assert req.metadata_directory is not None
try:
def runner(cmd, cwd=None, extra_environ=None):
call_subprocess(
Expand All @@ -715,8 +715,7 @@ def runner(cmd, cwd=None, extra_environ=None):
with req.pep517_backend.subprocess_runner(runner):
req.pep517_backend.build_wheel(
tempd,
# metadata_directory=req.metadata_directory
metadata_directory=None
metadata_directory=req.metadata_directory
)
return True
except Exception:
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/test_req.py
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,7 @@ def test_mismatched_versions(caplog, tmpdir):
shutil.copytree(original_source, source_dir)
req = InstallRequirement(req=Requirement('simplewheel==2.0'),
comes_from=None, source_dir=source_dir)
req.run_egg_info()
req.prepare_metadata()
req.assert_source_matches_version()
assert caplog.records[-1].message == (
'Requested simplewheel==2.0, '
Expand Down