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

Default to pants devs using Python 3 by renaming `./pants3`->`./pants` and `./pants`->`./pants2` #7257

Merged
merged 8 commits into from Mar 1, 2019
@@ -104,26 +104,31 @@ esac
# We're running against a Pants clone.
export PANTS_DEV=1

# Note that we set PY, and when running with Python 3, also set PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS.
# This would usually not be necessary when developing locally, because the `./pants` and `./pants3`
# scripts set these constraints for us already. However, we must set the values here because in non-bootstrap shards
# we run CI using `./pants.pex` instead of the scripts `./pants` and `./pants3`, so those scripts cannot set
# the relevant environment variables. Without setting these environment variables, the Python 3 shards will try to
# execute subprocesses using Python 2, which results in the _Py_Dealloc error (#6985), and shards that do not
# pull down `./pants.pex` but still use a virtualenv (such as Rust Tests) will fail to execute.
# Determine the Python version to use for bootstrapping pants.pex. This would usually not be
# necessary to set when developing locally, because the `./pants` and `./pants2` scripts set
# these constraints for us already. However, we must set the values here because in
# non-bootstrap shards we run CI using `./pants.pex` instead of the scripts `./pants`
# and `./pants2`, so those scripts cannot set the relevant environment variables.
if [[ "${python_two:-false}" == "false" ]]; then
py_version_number="3.6"
bootstrap_pants_script="./pants3"
export PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="['CPython==${py_version_number}.*']"
else
py_version_number="2.7"
py_major_minor="3.6"
bootstrap_pants_script="./pants"
else
py_major_minor="2.7"
bootstrap_pants_script="./pants2"
fi
export PY="python${py_version_number}"
banner "Using Python ${py_version_number} to execute spawned subprocesses (e.g. tests)"
export PY="${PY:-python${py_major_minor}}"

# Also set PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS. We set this to the exact Python version
# to resolve any potential ambiguity when multiple Python interpreters are discoverable, such as
# Python 2.7.10 vs. 2.7.13. When running with Python 3, we must also set this constraint to ensure
# all spawned subprocesses use Python 3 rather than the default of Python 2. This is in part
# necessary to avoid the _Py_Dealloc error (#6985).
py_major_minor_patch=$(${PY} -c 'import sys; print(".".join(map(str, sys.version_info[0:3])))')
export PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS:-['CPython==${py_major_minor_patch}']}"
banner "Setting interpreter constraints to ${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS}"

if [[ "${run_bootstrap:-false}" == "true" ]]; then
start_travis_section "Bootstrap" "Bootstrapping pants as a Python ${py_version_number} PEX"
start_travis_section "Bootstrap" "Bootstrapping pants as a Python ${py_major_minor_patch} PEX"
(
if [[ "${run_bootstrap_clean:-false}" == "true" ]]; then
./build-support/python/clean.sh || die "Failed to clean before bootstrapping pants."
15 pants
@@ -32,6 +32,8 @@
# WRAPPER_SRCPATH=/src/wrapper/src/main/python \
# exec /src/pantsbuild-pants/pants "$@"
#
# The script defaults to running with Python 3. To use a specific Python version,
# such as 3.7, prefix the script with `PY=python3.7`.

set -e

@@ -53,8 +55,17 @@ source ${HERE}/build-support/pants_venv
# + bootstrap_native_code: Builds target-specific native engine binaries.
source ${HERE}/build-support/bin/native/bootstrap_code.sh

# Default to using Python 2.7 if not otherwise specified.
export PY="${PY:-python2.7}"
# Default to using Python 3 if not otherwise specified.
export PY="${PY:-python3}"

# Set interpreter constraints to exactly match the interpreter used for the venv, if not already set.
# Note that $PY only impacts which virtualenv we use for the parent Pants process; we must also set
# PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS to constrain spawned subprocesses such as tests. Without
# any interpreter constraints, when PY is set to Python 3 we get the _Py_Dealloc exception (#6985) as
# Pants will try to use Python 2 for subprocesses. Without setting minor version constraints, when using Python 3
# we could end up using a different Python minor version for subprocesses than we do for Pants.
py_major_minor=$(${PY} -c 'import sys; print(".".join(map(str, sys.version_info[0:2])))')
export PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS:-['CPython==${py_major_minor}.*']}"

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Feb 27, 2019

Author Contributor

Note: here we allow any patch version, but in ci.sh we determined we must specify the patch version.

This was discovered while working on #7235. There, we have system Python 2.7 already, but must install a new version of Python 2.7 to get different unicode settings (UCS2 vs UCS4). We were running into issues with the interpreter changing between what was used to run Pants and what we used for subprocesses.

Should we default to constraining to the exact patch version too? Because this constraint is determined dynamically at runtime, I would advocate yes. The dynamic determination means that my computer could be using 3.7.1 and another's could be using 3.7.2, for example, without any issues. All it ensures is that that Pants run uses the exact same interpreter for subprocesses that it used for $PY / the virtualenv.

This comment has been minimized.

Copy link
@stuhood

stuhood Feb 27, 2019

Member

It doesn't feel like requiring a precise version in the pants script is strictly necessary... since the tests themselves don't require a precise version, and since the tests should run under some stableish version, even if pants is running under something else.

But it doesn't seem like it hurts to force them to match, so if it's caused you issues in that regard, it seems fine to align them. It's also possible that requiring a precise version only in CI (via the ci.sh/travis.yml) would be sufficient.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Feb 27, 2019

Author Contributor

I ended up making the change to require the exact patch version because while it's most often not important which interpreter it uses for tests—so long as they're the same minor version—there is a distinct case this would matter: interpreters with different UCS settings (i.e. how they store unicode).

For example, assume we have Python 2.7.13 built with UCS4 and have 2.7.15 built with UCS2. There is a meaningful difference between 2.7.13 vs. 2.7.15 here, due to the unicode setting. So, here, it does matter that we always consistently use 2.7.13 completely or 2.7.15 completely, and do not mix the two. This is what is going on in our ci.sh for #7235.

I can't think of a situation where it would be wrong to not have this guarantee that $PY interpreter is the same interpreter for subprocesses. Especially because we have the safety valve of letting the user pre-define PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS, I think this is the more correct implementation.


PANTS_EXE="${HERE}/src/python/pants/bin/pants_loader.py"

14 pants2
@@ -0,0 +1,14 @@
#!/usr/bin/env bash
# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

# This bootstrap script invokes Pants using a Python 2 interpreter.

export PY="python2.7"

# Allow spawned subprocesses, such as unit tests, to execute with either Python 2 and Python 3.
# So long as the target does not have a compatibility constraint that requires only Python 3, the
# interpreter selection will default to using Python 2 as this is the minimum acceptable interpreter.
export PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="['CPython>=2.7,<3','CPython>=3.6,<4']"

./pants "$@"
21 pants3

This file was deleted.

@@ -57,7 +57,7 @@ def console_output(self, _):
if self._closed:
# N.B. Sorting is necessary because `roots` is a set, and in Python 3 sets
# are no longer consistently sorted (even though dictionaries are in 3.6+).
# Without this sort, the output of `./pants3 dependees` will vary every run.
# Without this sort, the output of `./pants dependees` will vary every run.
for root in sorted(roots):
yield root.address.spec

@@ -39,6 +39,8 @@ def setup_warnings():
# TODO: Eric-Arellano has emailed the author to see if he is willing to accept a PR fixing the deprecation warnings
# and to release the fix. If he says yes, remove this once fixed.
warnings.filterwarnings('ignore', category=DeprecationWarning, module="ansicolors")
# TODO(7186): remove as part of work to land this PR.
warnings.filterwarnings('ignore', category=DeprecationWarning, module="pex")

@classmethod
def ensure_locale(cls):
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.