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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃彈 Add check for python v2.7 #23870

Merged
merged 1 commit into from
Aug 9, 2019
Merged

馃彈 Add check for python v2.7 #23870

merged 1 commit into from
Aug 9, 2019

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Aug 9, 2019

New laptops that don't have the recommended python version for AMP development (v2.7) can return mysterious errors. See #23801 (comment)

This PR prints a warning when a python version other than v2.7 is detected.

Addresses #23801 (comment)

@rsimha rsimha requested a review from wassgha August 9, 2019 18:35
@rsimha rsimha self-assigned this Aug 9, 2019
Copy link
Contributor

@wassgha wassgha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you :) LGTM

@wassgha
Copy link
Contributor

wassgha commented Aug 9, 2019

One suggestion I'd add is to make pythonExecutable configurable so that for example in my case I can set it to python2.7 (maybe a different issue).

@rsimha
Copy link
Contributor Author

rsimha commented Aug 9, 2019

One suggestion I'd add is to make pythonExecutable configurable so that for example in my case I can set it to python2.7 (maybe a different issue).

Unfortunately, this script can't be called with command line args because it's a pre install task. Let me know if python --version doesn't resolve on your machine.

@rsimha rsimha merged commit 085c0d9 into ampproject:master Aug 9, 2019
@rsimha rsimha deleted the 2019-08-09-Python branch August 9, 2019 19:41
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants