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

Pin OSX wheel release shards to pyenv. #7591

Merged
merged 8 commits into from Apr 21, 2019

Conversation

Projects
None yet
4 participants
@jsirois
Copy link
Member

commented Apr 19, 2019

Previously we only did this for the UCS4 OSX shard which allowed the
system interpreters to leak in to interpreter discovery on the other
shards. This was problematic since the system interpreters were linked
against too-old openssl leading to HTTPS errors attempting to talk to
pypi with too-old TLS protocols.

Also switch away from pyenv shims to the explicit version we need for
maximum clarity.

Finally, ensure failures running packages.py propagate. The fact that
they did not led to the issue here going undiscovered in branch CI.

Fixes #7593

@jsirois

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

Not ready for review.

@jsirois jsirois force-pushed the jsirois:ci/wheel-build/osx-fix branch 2 times, most recently from d1e35ae to b854ce1 Apr 19, 2019

@cosmicexplorer cosmicexplorer added this to the 1.16.x milestone Apr 19, 2019

@jsirois jsirois force-pushed the jsirois:ci/wheel-build/osx-fix branch 2 times, most recently from 2df3989 to 2ee66c7 Apr 19, 2019

@jsirois jsirois changed the title Setup CI debugging. Pin OSX wheel release shards to pyenv. Apr 19, 2019

@jsirois jsirois force-pushed the jsirois:ci/wheel-build/osx-fix branch 2 times, most recently from e283e83 to 813ad1b Apr 19, 2019

Pin OSX wheel release shards to pyenv.
Previously we only did this for the UCS4 OSX shard which allowed the
system interpreters to leak in to interpreter discovery on the other
shards. This was problematic since the system interpreters were linked
against too-old openssl leading to HTTPS errors attempting to talk to
pypi with too-old TLS protocols.

Also switch away from pyenv shims to the explicit version we need for
maximum clarity.

Finally, ensure failures running packages.py propagate. The fact that
they did not led to the issue here going undiscovered in branch CI.

Fixes #7593

@jsirois jsirois force-pushed the jsirois:ci/wheel-build/osx-fix branch from 113106c to 5fdc6e1 Apr 20, 2019

@stuhood
Copy link
Member

left a comment

Thank you so much for investigating this John!

Show resolved Hide resolved .travis.yml Outdated
@jsirois

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2019

I won't get all the pyenv shims / global use in this PR, so I filed #7601 to track nabbing the last few.

@stuhood

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

Thanks John!

@Eric-Arellano
Copy link
Contributor

left a comment

Thank you for tracking this down!

Any ideas on why this only started happening with the Pex upgrade? Even if not confirmed, if you have any hypotheses would be great to include in the PR description.

@@ -150,6 +151,8 @@ base_linux_config: &base_linux_config
language: python
before_install:
- ./build-support/bin/install_aws_cli_for_ci.sh
# TODO(John Sirois): Get rid of this in favor of explicitly adding pyenv versions to the PATH:

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 21, 2019

Contributor

Think this should be TODO(7601) for consistency.

This comment has been minimized.

Copy link
@jsirois

jsirois Apr 21, 2019

Author Member

That style is reader-hostile. With the full link, depending on my text viewer, I can open the url right there or - at the worst - directly copy and paste it in a browser.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 21, 2019

Contributor

Agreed on the value of a full link, which is why I always provide the full link to things in Slack and PR descriptions etc.

This comment is entirely motivated by this PR #6479, which I took to be the authority of how Pants does TODOs.

We should decide on a consistent style and add it to the style guide https://www.pantsbuild.org/styleguide.html.

@@ -150,6 +151,8 @@ base_linux_config: &base_linux_config
language: python
before_install:
- ./build-support/bin/install_aws_cli_for_ci.sh
# TODO(John Sirois): Get rid of this in favor of explicitly adding pyenv versions to the PATH:
# https://github.com/pantsbuild/pants/issues/7601
- pyenv global 2.7.15 3.6.7 3.7.1

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 21, 2019

Contributor

While I agree with not using shims for our own custom installed Python versions, I'm less convinced here.

On Linux, Travis preinstalls these 3 Pythons for us, but due to a bug on their end does not actually expose them to the PATH. This line is simply to fix that.

While we could also do the more verbose approach of exposing the PATH to pyenv/versions, I don't see as much benefit in doing so. We never will install a custom Pyenv version on Linux (at least till 3.8 is released), because it's significantly faster to use what they pre-install, even with us caching Pyenv. We also never need to build the interpreter differently, i.e. UCS2 vs. UCS4, because Python 3 has flexible string storage. Given that these are the only 3 that we will use, I see little ambiguity to humans and to the machine with this line.

This comment has been minimized.

Copy link
@jsirois

jsirois Apr 21, 2019

Author Member

I see little ambiguity to humans and to the machine with this line.

How do you know the pyenv shimming magic is even working? IE: pyenv is partially broken already so what says the shiming magic (https://github.com/pyenv/pyenv/blob/master/README.md see "3. Add pyenv init to your shell to enable shims and autocompletion.") is installed and available for the travis user?

If speed is an issue we should simply be using images on all linux shards and thus only relying on travis to checkout our code and have docker installed. The wins there are plentiful, including being able to repro any shard on your own machine.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 21, 2019

Contributor

How do you know the pyenv shimming magic is even working?

Because with this line, everything works. With it removed, none of the desired interpreters are discovered. Hence #7411, for example.

Perhaps other parts of Pyenv don't work. I don't know, as no one has audited that. But I think that's irrelevant here - the only thing we care about is getting the pre-installed interpreters discoverable, not that things like autocompletion work.

We can achieve this goal through either the current single line, or the more verbose pyenv/versions approach, so the question is simply which approach is better for this case.

If speed is an issue we should simply be using images on all linux shards and thus only relying on travis to checkout our code and have docker installed.

Good idea for the sake of reproducible builds. If we do go this route, in the meantime, sticking with Travis's preinstalled Pythons seems to be worth it.

This comment has been minimized.

Copy link
@jsirois

jsirois Apr 21, 2019

Author Member

Because with this line, everything works.

OK. Well, part of the difficulty of fixing CI is that there were bits sprinkled in that had nothing to do with getting things working. Reading these global lines it's easy to assume they are cruft. Now that I have your word this was a pivotal line, I can trust its not cruft - but hopefully you see the problem here.

I'll leave the TODO to save trees and perhaps the issue can hold dissenting views / be closed by deleting associated comments in the code.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 21, 2019

Contributor

If you end up making other changes in this PR, then I think it'd be helpful to add a comment explaining why we have these lines like this:

Travis preinstalls Python interpreters via Pyenv. Due to Travis's design, we must include this line to expose the interpreters to the PATH.

I should have included these when I added it. Alas.

- curl -L https://github.com/stedolan/jq/releases/download/jq-1.5/jq-osx-amd64 -o /usr/local/bin/jq
- chmod 755 /usr/local/bin/jq
- ./build-support/bin/install_aws_cli_for_ci.sh
- ./build-support/bin/install_python_for_ci.sh "${PYENV_PY36_VERSION}"

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 21, 2019

Contributor

In the PR to add Py3, I intentionally had moved the Python install to this entry rather than py27_osx_config and py36_osx_config both for reducing duplication and for the semantics that our default shard is guaranteed to have Py27 and Py36.

Py37 shard then overrides this default to have Py27 and Py36.

I'd advocate moving the before_install back to here.

This comment has been minimized.

Copy link
@jsirois

jsirois Apr 21, 2019

Author Member

I found this another confusing thing. As I understand it:

  1. Having Py36 available on the PATH on all OSX shards is a feature waiting for a customer.
  2. Having py27 available onthe PATH on all OSX shards is a needed feature (for build-support/bin/check_pants_pex_abi.py).

As such the old setup was installing an un-needed thing - it was noise when trying to figure out interpreter selection issues.

I sucked up the spirit of 1 by explicitly ensuring we have a sane 2.7 and 3.6+ on each OSX shard. We still don't need the 3.6+ yet though.

Generally I think it would be wise to avoid adding features to CI that we don't use. This beast is absurdly complex and it's hard enough to debug without unused bits floating about. Its nice to be able to assume every thing I'm reading is required and intentional.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 21, 2019

Contributor

Okay. Contrary to earlier request, I'm fine with removing Py36 from the Py2 shards then.

I was planning on porting some of our scripts in between 1.15.x and 1.17.x, but given the tepid response to modernizing check_pants_pex_abi.py and isort.py (which was warranted) and lots going on with school, gave up on this initiative. I'm not going to work on it the next few weeks, and by then we won't even have Python 2 shards.

# env vars.
ENV PY "${PYENV_ROOT}/shims/python2.7"
ENV PEX_PYTHON_PATH "${PYENV_ROOT}/shims/python2.7"
ENV PY "${PYENV_ROOT}/versions/${PYTHON_27_VERSION_UCS2}/bin/python"

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 21, 2019

Contributor

Yay! I could not for the life of me get this working last time I tried.

This comment has been minimized.

Copy link
@jsirois

jsirois Apr 21, 2019

Author Member

The ENTRYPOINT below was the missing key. These things are not inherited from base images.

@jsirois

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2019

Any ideas on why this only started happening with the Pex upgrade?

Yes. I added a comment in release.sh that points to pantsbuild/pex#709 and pantsbuild/pex#710:

# We use `PEX_PYTHON_PATH="${PY}" "${PY}" "${pex}"` instead of either of:
# 1. PEX_PYTHON_PATH="${PY}" "${pex}"
# 2. "${PY}" "${pex}"
# This works around Pex re-exec-ing when it need not and subsequently losing constraints when
# it need not. The fixes for these are tracked in:
# 1. https://github.com/pantsbuild/pex/issues/709
# 2. https://github.com/pantsbuild/pex/issues/710
PEX_PYTHON_PATH="${PY}" "${PY}" "${pex}" "$@"

I think this would all be noise in the description though since its not really relevant to the fixes, which stand on their own (explicitly selecting interpreters to avoid OSX bad ones and fixing short-circuiting bash logic).

@jsirois

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2019

A few good suggestions have come out of this review / been worked through in conversation and I'd like to follow through, but in a follow-up. Master CI has been red far too long, so I'll submit on green and circle back on a few things after that / proof master CI is green.

@Eric-Arellano
Copy link
Contributor

left a comment

Sgtm with the followup. Thanks again for landing this!

@jsirois jsirois merged commit 4a19087 into pantsbuild:master Apr 21, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jsirois jsirois deleted the jsirois:ci/wheel-build/osx-fix branch Apr 21, 2019

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Apr 22, 2019

Eric-Arellano added a commit that referenced this pull request Apr 22, 2019

Hotfix #7591 breaking OSX Rust tests shard due to Pyenv global issue (#…
…7602)

In #7591, we decided that (in general) we should avoid `pyenv global` and `~/.pyenv/shims` to instead favor the more explicit `~/.pyenv/versions/{py_version}/bin`.

Because of Travis caching, we did not catch that this change would break the OSX Rust shard, until merging the PR into master. https://travis-ci.org/pantsbuild/pants/jobs/522771730#L216

Here, we teach the OSX Rust shard to point to the `versions` folder. We also install Python 2.7 through Pyenv for consistency with the other OSX shards, as system Python has an outdated OpenSSL.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.