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
Fix pants_requirement by allowing Python 3 #6391
Fix pants_requirement by allowing Python 3 #6391
Conversation
cc @jsirois |
# TODO(John Sirois): Modify to constraint to >=3.5,<4 as part of | ||
# https://github.com/pantsbuild/pants/issues/6062 | ||
env_marker = "python_version>='2.7' and python_version<'3'" | ||
env_marker = "python_version>='3.4'" if PY3 else "python_version>='2.7' and python_version<'3'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be "python_version>='3.4' or (python_version>='2.7' and python_version<'3)". Reference: https://www.python.org/dev/peps/pep-0496/#micro-language
There is no need to make the marker conditional with full grouping and dis/conjunction support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
env_marker = "python_version>='3.4' or (python_version>='2.7' and python_version<'3')"
leads to tests failing on both Py2 and Py3.
Py2 failure:
self.assertFalse(py3_used)
E AssertionError: True is not false
Py3 failure:
self.assertFalse(evaluate_version('2.7'))
E AssertionError: True is not false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, because the asserts should change now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay, so the desired behavior is allowing both Python 2 and Python 3? I misunderstood, thought it was to require Py2 if using Py2, require Py3 if using Py3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - desired behavior is allowing all pythons Pants supports.
@jsirois This is ready for re-review when you have time. Would be great if we can slip this in to today's release. |
I'm pretty sure you'll need to rebase once @illicitonion gets in a fix for shard 3: https://travis-ci.org/pantsbuild/pants/jobs/421135626 |
Yeah, if you rebase, CI should go green :) Sorry! |
CI is green on this one. Thanks for the help all! |
Adds support for Py3 to #6361.