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 19 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

@@ -19,6 +19,10 @@ env:
- PANTS_CONFIG_FILES="${TRAVIS_BUILD_DIR}/pants.travis-ci.ini"
- PYTEST_PASSTHRU_ARGS="-v --duration=3"
- LC_ALL="en_US.UTF-8"
# This tells the ./pants runner script to avoid trying to clean the workspace when changing
# python versions. CI starts off without the .pants-python-version file, so it would otherwise
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
# python versions. CI starts off without the .pants-python-version file, so it would otherwise
# python versions. CI starts off without the .python-interpreter-constraints file, so it would otherwise
# run a clean-all without this env var.
- 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}
@@ -21,4 +21,9 @@ 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"

# This tells the ./pants runner script to avoid trying to clean the workspace when changing python
# versions. CI starts off without the .pants-python-version file, so it would otherwise run a
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
# versions. CI starts off without the .pants-python-version file, so it would otherwise run a
# versions. CI starts off without the .python-interpreter-constraints file, so it would otherwise run a
# clean-all without this env var.
ENV ONLY_USING_SINGLE_PYTHON_VERSION 'true'

WORKDIR /travis/workdir
@@ -46,4 +46,9 @@ 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"

# This tells the ./pants runner script to avoid trying to clean the workspace when changing python
# versions. CI starts off without the .pants-python-version file, so it would otherwise run a
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
# versions. CI starts off without the .pants-python-version file, so it would otherwise run a
# versions. CI starts off without the .python-interpreter-constraints file, so it would otherwise run a
# clean-all without this env var.
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,9 @@ 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"

# This tells the ./pants runner script to avoid trying to clean the workspace when changing python
# versions. CI starts off without the .pants-python-version file, so it would otherwise run a
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
# versions. CI starts off without the .pants-python-version file, so it would otherwise run a
# versions. CI starts off without the .python-interpreter-constraints file, so it would otherwise run a
# clean-all without this env var.
ENV ONLY_USING_SINGLE_PYTHON_VERSION 'true'

WORKDIR /travis/workdir
@@ -12,6 +12,10 @@ env:
- PANTS_CONFIG_FILES="${TRAVIS_BUILD_DIR}/pants.travis-ci.ini"
- PYTEST_PASSTHRU_ARGS="-v --duration=3"
- LC_ALL="en_US.UTF-8"
# This tells the ./pants runner script to avoid trying to clean the workspace when changing
# python versions. CI starts off without the .pants-python-version file, so it would otherwise
This conversation was marked as resolved by Eric-Arellano

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor
Suggested change
# python versions. CI starts off without the .pants-python-version file, so it would otherwise
# python versions. CI starts off without the .python-interpreter-constraints, so it would otherwise
# run a clean-all without this env var.
- 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}
39 pants
@@ -2,6 +2,39 @@
# 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_file_path='.pants.d/.pants-python-version'

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor

I recommend changing to .python-interpreter-constraints

Also because it's a local variable, I think you can remove the _ here and elsewhere. That's personal style preference though.

Suggested change
_interpreter_version_sentinel_file_path='.pants.d/.pants-python-version'
interpreter_constraints_sentinel_file_path='.pants.d/.python-interpreter-constraints'

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 15, 2019

Author Contributor

Removed _ for consistency (thanks for pointing that out, will stick with lowercase in this repo!) and changed to .python-interpreter-constraints!


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() {
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_file_path}"
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
_python_version_file="${REPO_ROOT}/${_interpreter_version_sentinel_file_path}"
interpreter_constraints_file="${REPO_ROOT}/${interpreter_constraints_sentinel_file_path}"

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 [[ "$PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS" == "$_prev_run_python_version" ]]; then

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor
Suggested change
if [[ "$PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS" == "$_prev_run_python_version" ]]; then
if [[ "${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS}" == "${previous_interpreter_constraints}" ]]; then

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 15, 2019

Author Contributor

I personally keep to the "$VAR" form for strings that are 100% the content of a variable dereference, and that's what we do in https://github.com/pantsbuild/binaries, but that's not what we do in this repo so will make these changes -- thanks for pointing them out!

return 0
fi
else
cat >&2 <<EOF
${_interpreter_version_sentinel_file_path} not found in the buildroot.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor
Suggested change
${_interpreter_version_sentinel_file_path} not found in the buildroot.
${interpreter_constraints_file} not found in the buildroot.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 15, 2019

Author Contributor

I kept this as ${interpreter_constraints_sentinel_file_path} as per the previous conversation we had about the full path name being unnecessary here.

Forcing an initial clean-all.
EOF
fi

export _recursive_pants_shell_script_invocation='y'
cat >&2 <<EOF
Different \$PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS were set compared to the previous Pants run.
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
Different \$PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS were set compared to the previous Pants run.
Different interpreter constraints were set than was used in the previous Pants run.
Clearing the cache and preparing a Python environment for ${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS}...
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
Clearing the cache and preparing a Python environment for ${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS}...
Clearing the cache and preparing a Python environment with these interpreter constraints: ${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS}.
EOF
./pants clean-all
echo "$PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS" > "$_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
echo "$PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS" > "$_python_version_file"
echo "${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS}" > "${interpreter_constraints_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,8 +98,14 @@ 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!

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

# 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

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

if [[ ! -z "${WRAPPER_REQUIREMENTS}" ]]; then
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.