Use ./pants3 for majority of CI #6981
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Eric-Arellano
force-pushed
the
pants3-in-ci
branch
from
a9b63a9 to
d6812eb
Compare
Eric-Arellano
force-pushed
the
pants3-in-ci
branch
from
d6812eb to
f4cc591
Compare
Travis recommends doing this for some reason
Locally, I ran into an issue when trying to bootstrap with Py3. The command ./pants -V was running with a Py2 subprocess even though the PEX was Py3 under-the-hood, so I ran into the _Py_Dealloc problem. Now we properly set the constraint before anything else.
Was causing the linting shard to fail
YAML has no way to merge lists. Instead, we exploit Travis's ability to define multiple environment variables in the same line. So, we simply move the variables to one line and assign that to an anchor. FYI inputting `.travis.yml` into https://yamlvalidator.com is extremely helpful to debug things and see the corresponding JSON.
Note that you can't mix an anchor with a property. `&new_var *old_var "test"` is invalid. So, we have to duplicate the Homebrew workaround in two places.
Same issue we were having on Linux. Even when exposing the path to pyenv, we must call pyenv global.
We need it because we aren't running `eval "$(pyenv init -)"`, so we need to manually expose the PATH to work properly.
Eric-Arellano
changed the title
Eric-Arellano
requested review from
benjyw,
cosmicexplorer,
stuhood,
jsirois,
illicitonion and
wisechengyi
|
Reviewers, this is an enormous PR and the git history is not very clean. But it represents a huge milestone in our migration! Here are some recommendations for review:
-- As a followup to discussion in #6959, we've now resolved 2 of the 3 major issues: stack traces now print with The only remaining issue is Thanks you all! |
illicitonion
approved these changes
Jan 15, 2019
There was a problem hiding this comment.
I'm not sure I fully grasp all the things, but it looks reasonable, and the results look good :)
Two comments from looking at the CI results:
- Why do we run
Python contrib tests (Py2 PEX w/ Py3 constraints)with py2 not py3? - Linux Rust tests (Py3 PEX) - this doesn't have a PEX (but does make a venv)
|
|
||
|
|
||
| HEADER = """ | ||
| # GENERATED, DO NOT EDIT! | ||
| # To change, edit build-support/travis/travis.yml.mustache and run | ||
| # ./pants --quiet run build-support/travis:generate_travis_yml > .travis.yml | ||
| # | ||
| # Tip: Copy the generated `.travis.yml` into https://yamlvalidator.com to validate the YAML |
There was a problem hiding this comment.
As a quick check, you can also run yq .travis.yml, which will echo the yaml if it validates, but give a completely useless error message if it doesn't... Online YAML validators like this one give much more helpful output.
Good suggestion Daniel!
It doesn't use a PEX, so we don't need py2 vs py3 setup. Instead, we specify it has no pex and run it in daily CI (no cron).
Cleaner to define them at the top root level, and simply extend in test matrix. This matches how we do it for every other shard (except for integration shard because there we need mustache magic).
Eric-Arellano
commented
Jan 15, 2019
There was a problem hiding this comment.
Thanks @illicitonion for the review!
We have to run contrib as Py2 PEX w/ Py3 subprocesses because contrib fails when ran with Py3. I tried to fix it with #7067 (I have one more commit locally I haven't pushed), but there are still two failures left. Both seem related to pantsd and @ensure_daemon, which I've been confused on how to fix.
So to avoid blocking this PR, we punt on it. This setup mirrors 100% how we currently run master's Py3 branch—we don't lose any py3 compatibility coverage, although don't gain anything new for contrib. Same setup for integration test blacklist.
|
Looks great to me! And thanks for the helpful review instructions. One small thing: I'm not sure a reader will grok "under the hood" as a term to distinguish the pants.pex interpreter from subprocess interpreters (those are just as metaphorically "under the hood"...). Could you maybe sharpen the comments in |
benjyw
approved these changes
Jan 16, 2019
I accidently had the ci.sh command in `before_script`, rather than `script`. While everything had an exit code of 0, Travis didn't know what we're trying to test so would fail.
Thanks Benjy for pointing this out and working with me on a few iterations! This rewrite tries to be more explicit with what is still actually happening, while keeping things fairly concise as they're Travis banners.
This was referenced Jan 16, 2019
Eric-Arellano
added a commit
that referenced
this pull request
Jan 17, 2019
## Problem #6981 led to the `Deploy unstable multiplatform pants.pex` shard failing consistently. The error can be reproduced locally via `./build-support/bin/release.sh -p`. The error message, however, is not very descriptive, even with `set -x` and `PANTS_VERBOSE=9`. ## Solution The likely culprit is removing `osx_image: xcode8` from the `OSX build wheels` shard. We had this hardcoded before #7091 for a reason, and removing it is probably what messed things up. The removal was not intentional, merely an oversight. Also, this changes the wheel shards to not have py2 vs py3 setups, as they don't use a PEX so the distinction is not meaningful at the moment. ## Result The result *cannot* be verified until being merged into master. This is because Travis never allows the `deploy` stage to happen from pull requests, as it's a security concern.
cosmicexplorer
added a commit
that referenced
this pull request
Apr 5, 2019
### Problem We have used a python 3 interpreter in CI for a while now, using `pyenv` to install this in our Dockerfiles instead of relying on CentOS's `scl` features, which don't allow for multiple pythons at a time. Using `pyenv` to install Python 2.7 as well instead of relying on the package manager allows us to upgrade or modify the image without screwing up our Python installation. That work began in this PR and ended up in #6981, #7352, #7418, #7483, and more, but it then led to having to duplicate code across `travis_ci` and `travis_ci_py36` Dockerfiles. Using `pyenv` to install python in our `travis_ci` images also involves a significant setup time in bootstrap phases to install our desired pythons. Installing Python in the CentOS 6 base image avoids having to install Python at all in our CI, which saves us minutes in the bootstrap phase. ### Solution - Use Pyenv to install Python 2.7 and Python 3.6 in the `centos6` Dockerfile. - Remove `travis_cy_py36`. - Modify the `travis_ci` Dockerfile to bootstrap pyenv itself if not available. ### Result We get some CI time back in the bootstrap phase. ### TODO - [ ] merge this PR - [ ] publish the new `centos6` image to dockerhub at `pantsbuild/centos6:latest` - [ ] merge a followup PR removing the manual pyenv bootstrapping from the `travis_ci` image.
stuhood
pushed a commit
that referenced
this pull request
Apr 8, 2019
### Problem We have used a python 3 interpreter in CI for a while now, using `pyenv` to install this in our Dockerfiles instead of relying on CentOS's `scl` features, which don't allow for multiple pythons at a time. Using `pyenv` to install Python 2.7 as well instead of relying on the package manager allows us to upgrade or modify the image without screwing up our Python installation. That work began in this PR and ended up in #6981, #7352, #7418, #7483, and more, but it then led to having to duplicate code across `travis_ci` and `travis_ci_py36` Dockerfiles. Using `pyenv` to install python in our `travis_ci` images also involves a significant setup time in bootstrap phases to install our desired pythons. Installing Python in the CentOS 6 base image avoids having to install Python at all in our CI, which saves us minutes in the bootstrap phase. ### Solution - Use Pyenv to install Python 2.7 and Python 3.6 in the `centos6` Dockerfile. - Remove `travis_cy_py36`. - Modify the `travis_ci` Dockerfile to bootstrap pyenv itself if not available. ### Result We get some CI time back in the bootstrap phase. ### TODO - [ ] merge this PR - [ ] publish the new `centos6` image to dockerhub at `pantsbuild/centos6:latest` - [ ] merge a followup PR removing the manual pyenv bootstrapping from the `travis_ci` image.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Right now our CI exclusively uses Python 2 under-the-hood. It tries to test Python 3 support by setting
--setup-python-interpreter-constraintsto Python 3, but this only constrains subprocesses like tests and is not the same thing as fully supporting Python 3.We want our CI to test using both Python 2 under-the-hood and Python 3 under-the-hood, because for the next two releases are promising users that either interpreter is fully supported.
Daily CI should run primarily with Python 3, and the nightly cron job should check for Python 2 regressions.
Solution
Use the new
./pants3script added in #6959.Installing Python 3 on each shard
languagefeature because several shards are supposed to belanguage: general.travis_ci/Dockerfileto download pyenv and install Python 3. This change is being pulled out thecentos6image in Install Python 2.7 and 3.6 on Centos6 base image through Pyenv #7064 to avoid rebuilding Py3 every CI run, but it must be published to Docker hub so will be addressed in a followup PR.language: pythonfeatureAdd integration test blacklist
23 integration test targets are failing when ran with Py3 under-the-hood, so we restore support for an integration test blacklist. This allows us to track Py3 fixes we make and to collaborate more easily.
Further, the contrib tests, linter, and osx rust shards when ran with Py3. So, we run these 3 shards and the blacklist with a Py2 pex but Py3 subprocesses, which is what we currently do in master.
Expand Cron job
We add non-integration test jobs to the nightly cron run to check for any regressions in shards like
Linux build wheels, now that we run daily with a Py3 PEX and need to check that they still work with a Py2 PEX.Default to Python 3 in CI
Defaulting to Py3 reflects that we are transitioning towards exclusive Py3 use.
This is not user-facing, and only impacts the
.travis.ymlcode, so this change is safe to make, whereas we want to wait to rename./pants3->./pantsuntil it is a bit more robust.FYI - new .travis.yml idiom
This required a major rewrite of
.travis.ymlto avoid duplication. The new idiom we use when defining a shard is to have abase_my_new_shardwhich defines the minimum amount of config necessary for that job, and then to have apy2_my_new_shardandpy3_my_new_shardthat extend that base along with their corresponding OS + python version config.