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 issue 826 - correct python and pip fallback #878

Merged
merged 14 commits into from
Apr 30, 2020
53 changes: 53 additions & 0 deletions .github/workflows/installation.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
name: Installation
on: [push]

jobs:
linux-vm:
name: ${{ matrix.python-version }} - ${{ matrix.method }}
runs-on: ubuntu-latest
strategy:
matrix:
python-version:
- '2.7'
- '3.6'
- '3.7'
method:
- 'python ./install.py'

Choose a reason for hiding this comment

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

We technically already test this type of install quite a lot for all platforms and all python versions. I think it would make sense to test this here, but only if we test install.py more in depth, like make sure it installs the rez scripts in the rez folder correctly, and on all platforms. We should also test https://github.com/nerdvegas/rez/blob/master/install.py#L81.

Without testing these things, I have the feeling we will just have jobs that will take CPU time without much benefits.

What do you think?

Copy link
Contributor Author

@j0yu j0yu Apr 27, 2020

Choose a reason for hiding this comment

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

test install.py more in depth, like make sure it installs the rez scripts in the rez folder correctly, and on all platforms. We should also test https://github.com/nerdvegas/rez/blob/master/install.py#L81.

I agree with testing python -E, let's do those in a separate PRs.

We technically already test this type of install quite a lot for all platforms and all python versions.

Does rez-selftest check python ./install.py? The other CI tests checks via python ./install.py ./build where as I wanted this to actually check the wiki's python ./install.py


Originally these tests were written just to double check what is said in the Installation Wiki page actually holds true and is not misleading beginners.

So far, this is just a start and we can expand on it to more in-depth and multi-platform tests in the future to ensure whenever someone discovers this project and tries to follow the wiki step by step, it should just work.

- 'pip install --target /opt/rez .'
include:
- method: 'python ./install.py'
exports: 'PATH=${PATH}:/opt/rez/bin/rez'
- method: 'pip install --target /opt/rez .'
exports: 'PATH=${PATH}:/opt/rez/bin PYTHONPATH=${PYTHONPATH}:/opt/rez'

steps:
- uses: actions/checkout@master
- uses: actions/setup-python@v1
with:
python-version: ${{ matrix.python-version }}

- name: Install
run: |
${{ matrix.method }}
find /opt/rez/ -maxdepth 2

Choose a reason for hiding this comment

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

I'm tempted to suggest to use check-manifest to test that pip would install the correct files. Though, we would probably have to work a little bit on the MANIFEST.in to make it work. But that should be done in another PR IMO if we ever decide to go that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, sounds like a good idea. Let's get a new PR/issue opened for that


- name: rez status
run: |
export ${{ matrix.exports }}
rez status

- name: rez-pip --install .
run: |
export ${{ matrix.exports }}
rez-pip --install .
rez view rez

- name: Import rez package in Python
run: |
export ${{ matrix.exports }}

# Still needed as there's no fallback to use system's python
rez bind python

echo "Checking rez as python package: ==========================================="
rez env rez -- python -c 'import rez;print(rez.__file__)'
115 changes: 90 additions & 25 deletions src/rez/pip.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
from rez.vendor.distlib.markers import interpret
from rez.vendor.distlib.util import parse_name_and_version
from rez.vendor.enum.enum import Enum
from rez.vendor.packaging.version import Version as PackagingVersion
from rez.vendor.packaging.specifiers import Specifier
from rez.vendor.six.six import StringIO
from rez.resolved_context import ResolvedContext
from rez.utils.execution import Popen
Expand All @@ -31,6 +33,9 @@
from textwrap import dedent


PIP_SPECIFIER = Specifier(">=19") # rez pip only compatible with pip>=19


class InstallMode(Enum):
# don't install dependencies. Build may fail, for example the package may
# need to compile against a dependency. Will work for pure python though.
Expand Down Expand Up @@ -81,35 +86,73 @@ def find_pip(pip_version=None, python_version=None):
"""
py_exe = None
context = None
found_pip_version = None
valid_found = False

py_exe, pip_version, context = find_pip_from_context(
python_version,
pip_version=pip_version
)

if not py_exe:
py_exe, pip_version, context = find_pip_from_context(
python_version,
pip_version=pip_version or "latest"
)
for version in [pip_version, "latest"]:
try:
py_exe, found_pip_version, context = find_pip_from_context(
python_version, pip_version=version
)
valid_found = _check_found(py_exe, found_pip_version)
if valid_found:
break
except BuildError as error:
print_warning(str(error))

if not py_exe:
if not valid_found:
import pip
pip_version = pip.__version__

found_pip_version = pip.__version__
py_exe = sys.executable
print_warning(
"Found no pip in python and pip package; "
"falling back to pip installed in rez own virtualenv (version %s)",
pip_version
print_warning("Found no pip in any python and/or pip packages!")
print_warning("Falling back to pip installed in rez own virtualenv:")
logging_arguments = (
("pip", found_pip_version, pip.__file__),
("python", ".".join(map(str, sys.version_info[:3])), py_exe),
)
for warn_args in logging_arguments:
print_warning("%10s: %s (%s)", *warn_args)

pip_major = pip_version.split('.')[0]
if int(pip_major) < 19:
raise RezSystemError("pip >= 19 is required! Please update your pip.")
if not _check_found(py_exe, found_pip_version, log_invalid=False):
message = "pip{specifier} is required! Please update your pip."
raise RezSystemError(message.format(specifier=PIP_SPECIFIER))

return py_exe, context


def find_python_from_package(context, name="python", default=None):
"""Get Python executable from current resolved python/pip context.

Args:
context (ResolvedContext): Resolved context with Python and pip.
name (str): Name of the package for Python instead of "python".
default (str): Force a particular fallback path for Python executable.

Returns:
str or None: Path to Python executable, if any.
"""
py_exe_path = default
context = context.copy()
context.append_sys_path = False # GitHub nerdvegas/rez/pull/826
j0yu marked this conversation as resolved.
Show resolved Hide resolved

py_packages = (v for v in context.resolved_packages if v.name == name)
python_package = next(py_packages)

if platform_.name == "windows" and python_package.version < Version("3"):
j0yu marked this conversation as resolved.
Show resolved Hide resolved
py_exe_path = context.which("python") # GitHub nerdvegas/rez/pull/798
nerdvegas marked this conversation as resolved.
Show resolved Hide resolved
else:
name_template = "python{}"
for trimmed_version in map(python_package.version.trim, [2, 1, 0]):
# exe_name after trimmed: python3.7, python3, python
exe_name = name_template.format(trimmed_version)
py_exe_path = context.which(exe_name)
if py_exe_path is not None:
break

return py_exe_path or default


def find_pip_from_context(python_version, pip_version=None):
"""Find pip from rez context.

Expand Down Expand Up @@ -157,12 +200,7 @@ def find_pip_from_context(python_version, pip_version=None):
print_debug("No rez package called %s found", target)
return None, None, None

py_exe_name = "python"
if platform_.name != "windows":
# Python < 2 on Windows doesn't have versionned executable.
py_exe_name += str(python_major_minor_ver.trim(1))

py_exe = context.which(py_exe_name)
py_exe = find_python_from_package(context)

proc = context.execute_command(
# -E and -s are used to isolate the environment as much as possible.
Expand Down Expand Up @@ -559,6 +597,33 @@ def _cmd(context, command):
raise BuildError("Failed to download source with pip: %s" % cmd_str)


def _check_found(py_exe, version_text, log_invalid=True):
"""Check the Python and pip version text found.

Args:
py_exe (str or None): Python executable path found, if any.
version_text (str or None): Pip version found, if any.
log_invalid (bool): Whether to log messages if found invalid.

Returns:
bool: Python is OK and pip version fits against ``PIP_SPECIFIER``.
"""
is_valid = True
message = "Needs pip%s, but found '%s' for Python '%s'"

if version_text is None or not py_exe:
is_valid = False
if log_invalid:
print_debug(message, PIP_SPECIFIER, version_text, py_exe)

elif PackagingVersion(version_text) not in PIP_SPECIFIER:
is_valid = False
if log_invalid:
print_warning(message, PIP_SPECIFIER, version_text, py_exe)

return is_valid


_verbose = config.debug("package_release")


Expand Down
4 changes: 3 additions & 1 deletion src/rez/resolved_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ def __init__(self, package_requests, verbosity=0, timestamp=None,
effect if `verbosity` > 2.
print_stats (bool): If true, print advanced solver stats at the end.
"""
self.append_sys_path = True
self.load_path = None

# resolving settings
Expand Down Expand Up @@ -1715,7 +1716,8 @@ def _execute(self, executor):
self._append_suite_paths(executor)

# append system paths
executor.append_system_paths()
if self.append_sys_path:
executor.append_system_paths()

# add rez path so that rez commandline tools are still available within
# the resolved environment
Expand Down