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

run clean-all before switching pants python version runner scripts #7151

Merged
Changes from 16 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
7cf44e5
run clean-all before switching pants python version runner scripts
cosmicexplorer Jan 25, 2019
d16c292
add sentinel file to gitignore, with an issue link
cosmicexplorer Jan 25, 2019
c003ebc
respond to review comments
cosmicexplorer Jan 25, 2019
9d5eb2b
introduce a temporary env var to avoid triggering a clean in CI
cosmicexplorer Jan 25, 2019
78dac49
don't use an environment variable, just check tty before cleaning
cosmicexplorer Jan 28, 2019
abdb335
add set -x to trace what's failing
cosmicexplorer Jan 29, 2019
e8c3060
Revert "add set -x to trace what's failing"
cosmicexplorer Feb 3, 2019
e7e8ab1
correctly check whether we are in the CI environment
cosmicexplorer Feb 6, 2019
82d193f
try adding an env var to .travis.yml again
cosmicexplorer Feb 7, 2019
3b9d1e6
copy the single python version env var to the wheel building shards
cosmicexplorer Feb 10, 2019
0658c8b
add ONLY_USING_SINGLE_PYTHON_VERSION to all docker command lines
cosmicexplorer Mar 10, 2019
a96856e
move ONLY_USING_SINGLE_PYTHON_VERSION to the travis_ci Dockerfile
cosmicexplorer Mar 10, 2019
aecfc2b
add the env var to the ucs2 dockerfile as well
cosmicexplorer Mar 10, 2019
0a26141
use full python version for output file
cosmicexplorer Mar 10, 2019
25e0023
refactor interpreter checking logic
cosmicexplorer Mar 10, 2019
2efeb79
respond to review comments
cosmicexplorer Mar 11, 2019
075a7d2
remove references to #7152
cosmicexplorer Mar 13, 2019
0642281
move .pants-python-version to .pants.d!
cosmicexplorer Mar 13, 2019
30667d0
use interpreter constraints as the cache key and remove unnecessary e…
cosmicexplorer Mar 15, 2019
760edec
respond to review comments
cosmicexplorer Mar 15, 2019
4693413
move interpreter constraints file out of .pants.d AND make interprete…
cosmicexplorer Mar 16, 2019
107d821
add ONLY_USING_SINGLE_PYTHON_VERSION to hermetic env whitelist
cosmicexplorer Mar 16, 2019
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -53,3 +53,5 @@ GRTAGS
GSYMS
GTAGS
.mypy_cache/
# TODO(#7152): remove this after the python 3 migration!
/.pants-python-version
This conversation was marked as resolved by Eric-Arellano

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jan 25, 2019

Contributor

Think the / isn't supposed to be there. I'm surprised the file isn't being tracked as is.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jan 25, 2019

Author Contributor

The leading / means the pattern doesn't recurse into subdirectories! See https://git-scm.com/docs/gitignore#_pattern_format!

This conversation was marked as resolved by cosmicexplorer

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 12, 2019

Member

Driveby comment, sorry. But: could maybe put this under .pants.d?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 13, 2019

Author Contributor

Oooh, good call actually. @Eric-Arellano since we're not planning to use the .pants-python-version file in the setup repo would .pants.d/.pants-python-version make sense here?

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 13, 2019

Contributor

Yes, very good idea! Always an advocate for less at the root level.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 13, 2019

Author Contributor

Done!

@@ -19,6 +19,9 @@ env:
- PANTS_CONFIG_FILES="${TRAVIS_BUILD_DIR}/pants.travis-ci.ini"
- PYTEST_PASSTHRU_ARGS="-v --duration=3"
- LC_ALL="en_US.UTF-8"
# TODO(#7152): this tells the ./pants runner script to avoid trying to clean the workspace when
# changing python versions. Remove all instances of this after the python 3 migration!
- ONLY_USING_SINGLE_PYTHON_VERSION='true'
- BOOTSTRAPPED_PEX_BUCKET=ci-public.pantsbuild.org
- BOOTSTRAPPED_PEX_KEY_PREFIX=${TRAVIS_BUILD_NUMBER}/${TRAVIS_BUILD_ID}/pants.pex
- BOOTSTRAPPED_PEX_URL_PREFIX=s3://${BOOTSTRAPPED_PEX_BUCKET}/${BOOTSTRAPPED_PEX_KEY_PREFIX}
@@ -395,7 +398,7 @@ cargo_audit: &cargo_audit
base_build_wheels: &base_build_wheels
stage: *test
env:
- &base_build_wheels_env PREPARE_DEPLOY=1
- &base_build_wheels_env PREPARE_DEPLOY=1 ONLY_USING_SINGLE_PYTHON_VERSION='true'

py27_linux_build_wheels_no_ucs: &py27_linux_build_wheels_no_ucs
# Similar to the bootstrap shard, we build Linux wheels in a docker image to maximize
@@ -462,7 +465,7 @@ py27_osx_build_wheels_ucs2: &py27_osx_build_wheels_ucs2
- CACHE_NAME=osxwheelsbuild.ucs2
script:
- ./build-support/bin/check_pants_pex_abi.py cp27m
- RUN_PANTS_FROM_PEX=1 ./build-support/bin/release.sh -n
- RUN_PANTS_FROM_PEX=1 ONLY_USING_SINGLE_PYTHON_VERSION='true' ./build-support/bin/release.sh -n

py27_osx_build_wheels_ucs4: &py27_osx_build_wheels_ucs4
<<: *py27_osx_config
@@ -495,7 +498,7 @@ py27_osx_build_wheels_ucs4: &py27_osx_build_wheels_ucs4
script:
- ./build-support/bin/ci.sh -2b
- ./build-support/bin/check_pants_pex_abi.py cp27mu
- RUN_PANTS_FROM_PEX=1 ./build-support/bin/release.sh -n
- RUN_PANTS_FROM_PEX=1 ONLY_USING_SINGLE_PYTHON_VERSION='true' ./build-support/bin/release.sh -n

# -------------------------------------------------------------------------
# Rust tests
@@ -21,4 +21,8 @@ USER ${TRAVIS_USER}:${TRAVIS_GROUP}
# Our newly created user is unlikely to have a sane environment: set a locale at least.
ENV LC_ALL="en_US.UTF-8"

# TODO(#7152): this tells the ./pants runner script to avoid trying to clean the workspace when
# changing python versions. Remove all instances of this after the python 3 migration!
ENV ONLY_USING_SINGLE_PYTHON_VERSION 'true'

WORKDIR /travis/workdir
@@ -46,4 +46,8 @@ USER ${TRAVIS_USER}:${TRAVIS_GROUP}
# Our newly created user is unlikely to have a sane environment: set a locale at least.
ENV LC_ALL="en_US.UTF-8"

# TODO(#7152): this tells the ./pants runner script to avoid trying to clean the workspace when
# changing python versions. Remove all instances of this after the python 3 migration!
ENV ONLY_USING_SINGLE_PYTHON_VERSION 'true'

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 10, 2019

Contributor

Note to other reviewers, we will always duplicate most of this Dockerfile from travis_ci because we don't want Py27 w/ UCS2 on most of our Travis shards. See https://github.com/pantsbuild/pants/blob/master/build-support/docker/travis_ci_py27_ucs2/Dockerfile#L4 for further explanation.

This duplication is frustrating, but necessary until we drop Py2.


WORKDIR /travis/workdir
@@ -43,4 +43,8 @@ USER ${TRAVIS_USER}:${TRAVIS_GROUP}
# Our newly created user is unlikely to have a sane environment: set a locale at least.
ENV LC_ALL="en_US.UTF-8"

# TODO(#7152): this tells the ./pants runner script to avoid trying to clean the workspace when
# changing python versions. Remove all instances of this after the python 3 migration!
ENV ONLY_USING_SINGLE_PYTHON_VERSION 'true'

WORKDIR /travis/workdir
@@ -12,6 +12,9 @@ env:
- PANTS_CONFIG_FILES="${TRAVIS_BUILD_DIR}/pants.travis-ci.ini"
- PYTEST_PASSTHRU_ARGS="-v --duration=3"
- LC_ALL="en_US.UTF-8"
# TODO(#7152): this tells the ./pants runner script to avoid trying to clean the workspace when
# changing python versions. Remove all instances of this after the python 3 migration!
- ONLY_USING_SINGLE_PYTHON_VERSION='true'
- BOOTSTRAPPED_PEX_BUCKET=ci-public.pantsbuild.org
- BOOTSTRAPPED_PEX_KEY_PREFIX=${TRAVIS_BUILD_NUMBER}/${TRAVIS_BUILD_ID}/pants.pex
- BOOTSTRAPPED_PEX_URL_PREFIX=s3://${BOOTSTRAPPED_PEX_BUCKET}/${BOOTSTRAPPED_PEX_KEY_PREFIX}
@@ -359,7 +362,7 @@ cargo_audit: &cargo_audit
base_build_wheels: &base_build_wheels
stage: *test
env:
- &base_build_wheels_env PREPARE_DEPLOY=1
- &base_build_wheels_env PREPARE_DEPLOY=1 ONLY_USING_SINGLE_PYTHON_VERSION='true'
py27_linux_build_wheels_no_ucs: &py27_linux_build_wheels_no_ucs
# Similar to the bootstrap shard, we build Linux wheels in a docker image to maximize
@@ -415,7 +418,7 @@ py27_osx_build_wheels_ucs2: &py27_osx_build_wheels_ucs2
- CACHE_NAME=osxwheelsbuild.ucs2
script:
- ./build-support/bin/check_pants_pex_abi.py cp27m
- RUN_PANTS_FROM_PEX=1 ./build-support/bin/release.sh -n
- RUN_PANTS_FROM_PEX=1 ONLY_USING_SINGLE_PYTHON_VERSION='true' ./build-support/bin/release.sh -n
py27_osx_build_wheels_ucs4: &py27_osx_build_wheels_ucs4
<<: *py27_osx_config
@@ -442,7 +445,7 @@ py27_osx_build_wheels_ucs4: &py27_osx_build_wheels_ucs4
script:
- ./build-support/bin/ci.sh -2b
- ./build-support/bin/check_pants_pex_abi.py cp27mu
- RUN_PANTS_FROM_PEX=1 ./build-support/bin/release.sh -n
- RUN_PANTS_FROM_PEX=1 ONLY_USING_SINGLE_PYTHON_VERSION='true' ./build-support/bin/release.sh -n
# -------------------------------------------------------------------------
# Rust tests
41 pants
@@ -2,6 +2,41 @@
# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

REPO_ROOT=$(cd $(dirname "${BASH_SOURCE[0]}") && pwd -P)

_interpreter_version_sentinel_filename='.pants-python-version'

# TODO(#7152): remove this after the python 3 migration!
function clean_if_interpreter_changed() {
This conversation was marked as resolved by cosmicexplorer

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor
Suggested change
function clean_if_interpreter_changed() {
function clean_if_interpreter_constraints_changed() {
_python_version_to_run="$1"

if [[ -n "${_recursive_pants_shell_script_invocation:-}" ]]; then
return 0
fi
This conversation was marked as resolved by Eric-Arellano

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jan 25, 2019

Contributor

Can use the Bash ternary idiom 😎

major_version="2"; [ "${PANTS_USE_PYTHON3}" = true ] && python_bin_name="3"
echo $major_version

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jan 25, 2019

Author Contributor

I was thinking about that, but thought it would be unclear. However, given your framing of it as default 2 then 3, this makes sense. Thanks! This could even be outside of a function at this point.

This comment has been minimized.

Copy link
@cosmicexplorer

_python_version_file="${REPO_ROOT}/${_interpreter_version_sentinel_filename}"

if [[ -f "$_python_version_file" ]]; then
This conversation was marked as resolved by cosmicexplorer

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor
Suggested change
if [[ -f "$_python_version_file" ]]; then
if [[ -f "${interpreter_constraints_file}" ]]; then
_prev_run_python_version="$(cat "$_python_version_file")"
This conversation was marked as resolved by cosmicexplorer

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor
Suggested change
_prev_run_python_version="$(cat "$_python_version_file")"
previous_interpreter_constraints="$(cat "$interpreter_constraints_file")"
if [[ "$_python_version_to_run" == "$_prev_run_python_version" ]]; then
return 0
fi
else
cat >&2 <<EOF
${_interpreter_version_sentinel_filename} not found in the buildroot. Forcing an initial clean-all...
EOF
fi

export _recursive_pants_shell_script_invocation='y'
cat >&2 <<EOF
Different Python interpreter version detected compared to the previous Pants run.
Clearing the cache and preparing a Python ${_python_version_to_run} run...
EOF
./pants clean-all
echo "$_python_version_to_run" > "$_python_version_file"
unset _recursive_pants_shell_script_invocation
}

# This bootstrap script runs pants from the live sources in this repo.
#
# Further support is added for projects wrapping pants with custom external extensions. In the
@@ -65,6 +100,12 @@ export PY="${PY:-python3}"
# 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_patch=$(${PY} -c 'import sys; print(".".join(map(str, sys.version_info[0:3])))')

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor

We don't want this extra whitespace because the comment explains both the above and the below line.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 15, 2019

Author Contributor

Thanks!

# We can currently assume the CI environment does not switch python versions within a shard.
if [[ "${ONLY_USING_SINGLE_PYTHON_VERSION:-false}" != 'true' ]]; then

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor

Danny and I were talking over Slack about alternatively naming this DISABLE_AUTO_CLEAN_ALL. We have to decide if we want to describe the property the environment has or what we're specifically doing to it.

Preferences from others?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 15, 2019

Author Contributor

I think if we used DISABLE_AUTO_CLEAN_ALL and then want to do something completely different (such as eventually not clean-all), we would have to change the variable name, and it runs the danger of making this confusing part of our init more confusing. I think describing the reason why we're not doing the clean-all, then leaving a comment (which itself can be updated) about what that means is a more stable approach.

clean_if_interpreter_changed "$py_major_minor_patch"

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor

We only need to clean if the interpreter constraints, not if PY changed. In the default case, how we have set up ./pants and ./pants2, changing PY will by consequence change the interpreter constraints, but what we want to monitor is the constraints rather than PY.

Suggested change
clean_if_interpreter_changed "$py_major_minor_patch"
clean_if_interpreter_constraints_changed

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 15, 2019

Author Contributor

Oops, yes, forgot to remove that! Thanks!

fi

export PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS:-['CPython==${py_major_minor_patch}']}"

PANTS_EXE="${HERE}/src/python/pants/bin/pants_loader.py"
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.