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

Add runtime check for valid Python interpreter #7365

Merged
merged 13 commits into from Mar 14, 2019

Conversation

Projects
None yet
3 participants
@Eric-Arellano
Copy link
Contributor

commented Mar 12, 2019

Problem

Now that we'll soon allow running Pants with more than Python 2.7 for every day users, we should eagerly ensure that they are using a supported Python version. This is necessary because Pants is typically ran as a script, rather than as a library, and the script may be ran with whichever interpreter the user chooses.

Even though the setup repo is supposed to ensure people use a valid interpreter, it is easy to get around this with PYTHON=3.5 ./pants or with putting pants_engine_python_version: 3.5 in their pants.ini. Both these scenarios are totally valid to do, and we should not teach the setup repo or --pants-engine-python-version what are valid options, because the former is easy to fall out of sync with users and the latter is easy to circumvent.

While Pants might work with something like 3.5, we should at least warn users they're treading into dangerous waters. If they want to proceed by setting an env var bypass, all power to them. But for the sake of UX we should at least give the warning, rather than relying on strange bugs and SyntaxErrors to enforce using the correct version.

Solution

Add a runtime check to pants_loader.py with a human readable error message. Allow skipping the check with PANTS_IGNORE_UNSUPPORTED_PYTHON_INTERPRETER.

Add unit tests for this all.

Result

When running with anything other than 2.7 or 3.6+, Pants will error out.

Once we drop Py2, we can change this check to 3.6+.

Eric-Arellano added some commits Mar 12, 2019

Simplify skip_unless_any_pythons_present()
Use any() builtin. Earlier we were checking how many pythons are discovered. I realized that skipIf doesn't print the message when False, so this isn't very useful to spend the time figuring out as no one will read the message. Instead, make the code simpler.

@Eric-Arellano Eric-Arellano requested review from stuhood, jsirois and kwlzn Mar 12, 2019

@Eric-Arellano
Copy link
Contributor Author

left a comment

Note if you all would like to test this locally, pull it down, then run pyenv global 3.5.* && ./pants -V.

Thanks!

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

Eric-Arellano added some commits Mar 12, 2019

Convert integration test to 2 unit tests
Integration test was never going to work because we run from pants.pex and can't change the interpreter for the subprocess. Even if we could, the subprocess would start bootstrapping Pants, which we do NOT want.

Further, patching the integration test won't work because the ./pants run is a subprocess so won't pick up on the patch.

Instead, we can split the functionality into two dedicated unit tests and actually end up with better coverage and faster tests!
Add future backports
Ironic I forgot to do this. Can't wait for us to stop having the cognitive load of remembering these imports..

Eric-Arellano added some commits Mar 12, 2019

Deduplicate calling sys.version_info[0:2]
Use string interpolation rather than ".".join() to avoid an unnecessary line.
Remove unneeded `as e` in with statement
Sorry for the email spam. This is ready for review now.
Fix bad import
Mock only was made part of stdlib in Py3.
@benjyw

benjyw approved these changes Mar 13, 2019

Eric-Arellano added some commits Mar 14, 2019

Fix unit test assertIns not actually ever running
TIL, with self.assertRaises() is only meant to capture the error. The code will exit the with statements once the exception is caught. So, the 3 assertions we had were never actually running.

Also, one assertion was too brittle so it's removed.

@Eric-Arellano Eric-Arellano merged commit 5879cf1 into pantsbuild:master Mar 14, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:valid-interpreter branch Mar 14, 2019

Eric-Arellano added a commit that referenced this pull request Mar 29, 2019

Revert unnecessary runtime check for valid Python interpreter (#7451)
### Problem
Pants developers run Pants differently than end users do. Whereas they either download the wheel from PyPi or use a Pex, developers build from source. This allows us to run Pants with any Python version we'd like.

Due to a misunderstanding thinking this meant end users could also run Pants with any Python interpreter they wanted, we added in #7365 a runtime check to ensure they use 2.7 or 3.6+.

However, we realized it is impossible (or at least extremely unlikely) that end users will run Pants with an invalid interpreter, due to install time checks, as follows:

* Pip will not resolve for invalid interpreters like 3.5, as it will not be able to find a valid wheel.
* Pex includes interpreter constraints when being built, so the Pex cannot be ran with an invalid interpreter.

Thus, the runtime check is unnecessary.

### Solution
Remove the runtime check.

### Result
Less code and slightly faster performance due to one less check.

stuhood added a commit that referenced this pull request Mar 29, 2019

Revert unnecessary runtime check for valid Python interpreter (#7451)
### Problem
Pants developers run Pants differently than end users do. Whereas they either download the wheel from PyPi or use a Pex, developers build from source. This allows us to run Pants with any Python version we'd like.

Due to a misunderstanding thinking this meant end users could also run Pants with any Python interpreter they wanted, we added in #7365 a runtime check to ensure they use 2.7 or 3.6+.

However, we realized it is impossible (or at least extremely unlikely) that end users will run Pants with an invalid interpreter, due to install time checks, as follows:

* Pip will not resolve for invalid interpreters like 3.5, as it will not be able to find a valid wheel.
* Pex includes interpreter constraints when being built, so the Pex cannot be ran with an invalid interpreter.

Thus, the runtime check is unnecessary.

### Solution
Remove the runtime check.

### Result
Less code and slightly faster performance due to one less check.
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.