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.

Sign up for GitHub

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

Jump to bottom

[Feature] Pure python package detection #628

Merged
merged 5 commits into from
Jul 16, 2019
Merged

[Feature] Pure python package detection #628

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8ffa7f0
feat(rez-pip): add pure python package detector and streamline variants
lambdaclan May 17, 2019
09e44ae
fix(rez-pip): revert to original run pip command function
lambdaclan Jul 8, 2019
2bb1726
refactor(rez-pip): remove variant purity type debugging
lambdaclan Jul 8, 2019
f07d216
refactor(rez-pip): add comments for json metadata and purelib/platlib
lambdaclan Jul 8, 2019
186694b
refactor(rez-pip) remove variant types debugging variable
lambdaclan Jul 15, 2019
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
40 changes: 32 additions & 8 deletions src/rez/pip.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
from tempfile import mkdtemp
from StringIO import StringIO
from pipes import quote
from email.parser import Parser
import subprocess
import pkg_resources
import os.path
import shutil
import sys
Expand Down Expand Up @@ -200,10 +202,8 @@ def pip_to_rez_package_name(distribution):

def run_pip_command(command_args, pip_version=None, python_version=None):
"""Run a pip command.

Args:
command_args (list of str): Args to pip.

Returns:
`subprocess.Popen`: Pip process.
"""
Expand Down Expand Up @@ -371,6 +371,22 @@ def pip_install_package(source_name, pip_version=None, python_version=None,
_cmd(context=context, command=cmd)
_system = System()

def pure_python_package(installed_dist):

true_table = {
"true": True,
"false": False
}

packages = pkg_resources.find_distributions(destpath)
dist = next((package for package in packages if package.key == installed_dist.key), None)
wheel_data = dist.get_metadata('WHEEL')
# see https://www.python.org/dev/peps/pep-0566/#json-compatible-metadata
wheel_data = Parser().parsestr(wheel_data)
Copy link
Member

Choose a reason for hiding this comment

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

A little comment here that points to the documentation about that would be great (like https://www.python.org/dev/peps/pep-0566/#json-compatible-metadata).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense I will add it.


# see https://www.python.org/dev/peps/pep-0427/#what-s-the-deal-with-purelib-vs-platlib
return true_table[wheel_data["Root-Is-Purelib"]]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a little link to https://www.python.org/dev/peps/pep-0427/#what-s-the-deal-with-purelib-vs-platlib or something else that explains a little bit more what's the deal with Root-Is-Purelib?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense I will add it.


# Collect resulting python packages using distlib
distribution_path = DistributionPath([destpath])
distributions = [d for d in distribution_path.get_distributions()]
Expand Down Expand Up @@ -446,18 +462,26 @@ def make_root(variant, path):
shutil.copystat(source_file, destination_file)

# determine variant requirements
# TODO detect if platform/arch/os necessary, no if pure python
variant_reqs = []
variant_reqs.append("platform-%s" % _system.platform)
variant_reqs.append("arch-%s" % _system.arch)
variant_reqs.append("os-%s" % _system.os)

pure = pure_python_package(distribution)
if not pure:
variant_reqs.append("platform-%s" % _system.platform)
variant_reqs.append("arch-%s" % _system.arch)

if context is None:
# since we had to use system pip, we have to assume system python version
py_ver = '.'.join(map(str, sys.version_info[:2]))
if pure:
py_ver = '.'.join(map(str, sys.version_info[:1]))
else:
py_ver = '.'.join(map(str, sys.version_info[:2]))
else:
python_variant = context.get_resolved_package("python")
py_ver = python_variant.version.trim(2)

if pure:
py_ver = python_variant.version.trim(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking that we shouldn't do this; or, that we do it but only if we know it's safe to do so.

"Safe" depends on whether there are markers in the requirements. For eg, if the package requires "foo-1.0 ; python-3.1.*", then the only way we can properly express the package's requirements in rez is if we variant on pthon-MAJOR.MINOR; otherwise we cannot specify foo as a requirement for just that case.

I also suspect that the complexity in determining this from the markers isn't worth it, and that we should probably just stick to varianting on python-MAJOR.MINOR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello Allan, after discussing the issue with Thorsten we have reached an agreement that we can live without this particular change, trimming the minor version when dealing with pure packages. What is most important for us is the removal of the os as that adds a significant number of rebuilds when dealing with Windows versions (python x plat x arch x os) removing just one of these segments will make our work much much easier.

Also. as you mention above it makes sense to keep the minor version since it might be a hard requirement for some packages that will cause other issues. I would like to explore the markers solution down the line so I will take a look and see if there is a way to achieve it, getting rid of the minor version will still be a big improvement if it is reliable.

For now I will remove the minor version trimming so that we can continue with the review before the merge.

else:
py_ver = python_variant.version.trim(2)

variant_reqs.append("python-%s" % py_ver)

Expand Down