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.

Sign up for GitHub

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

Jump to bottom

Specify the minimum python version in the setup.py. #12144

Merged
merged 2 commits into from
Feb 7, 2018
Merged

Specify the minimum python version in the setup.py. #12144

merged 2 commits into from
Feb 7, 2018

Conversation

pelson
Copy link
Contributor

@pelson pelson commented Feb 3, 2018

Description:

Makes it explicit about which versions of python are supported. If pip install homeassistant is called from a version of python <= 3.4 the following error comes from pip (pip >= v9.0.1):`

(py2) pelson@~/dev/IOT/home-assistant> python --version
Python 2.7.12
(py2) pelson@~/dev/IOT/home-assistant> pip install -e .
Obtaining file:///Users/pelson/dev/IOT/home-assistant
homeassistant requires Python '>=3.4' but the running Python is 2.7.12

See also:

Checklist:

  • The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@pelson pelson requested a review from a team as a code owner February 3, 2018 07:11
@pelson
Copy link
Contributor Author

pelson commented Feb 3, 2018

FWIW pip 9 has been around since late 2016: https://pip.pypa.io/en/stable/news/

@fabaff
Copy link
Member

fabaff commented Feb 3, 2018

The issue is not for how long something is around but is it shipped with Debian. Raspbian is a very popular OS platform among Home Assistant users. Around 30 % of the users are using Debian 8/ Rapbian 8 and there pip 1.5.x was shipped. pip 9.0.1 was introduced with Stretch and Raspian 9.

@pelson
Copy link
Contributor Author

pelson commented Feb 3, 2018

The issue is not for how long something is around but is it shipped with Debian

Fair. I notice that the version of aiohttp that we depend upon also uses python_requires in its setup.py. https://github.com/aio-libs/aiohttp/blob/v2.3.7/setup.py#L113. I don't have an old version of pip to hand to test whether this attribute is simply ignored for old pips.

pvizeli
pvizeli requested changes Feb 3, 2018
View reviewed changes
setup.py Outdated
@@ -75,6 +75,7 @@
zip_safe=False,
platforms='any',
install_requires=REQUIRES,
python_requires=">=3.4",
Copy link
Member

Choose a reason for hiding this comment

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

'>=3.5.2'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent! Does #9328 need to be updated+closed then?

Copy link
Member

Choose a reason for hiding this comment

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

You can update it

Copy link
Member

Choose a reason for hiding this comment

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

I think aiohttp wants 3.5.3+ as minimum for the future.

Copy link
Member

Choose a reason for hiding this comment

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

We are still Python 3.4 Pascal. We're waiting till aiohttp releases v3 to bump min requirement to 3.5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given this conversation, I don't think there is anything I need to do to get this PR ready.

Clearly it will be good to have the minimum python version explicitly stated in the codebase (setup.py).

Copy link
Member

Choose a reason for hiding this comment

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

We already define the minimum required version in homeassistant.const, can you import it from there? We also have a different minimum version between Windows and other OSes (because of the asyncio loop that we use).

https://github.com/home-assistant/home-assistant/blob/7e246e4680d71fe9913678ede731e8c6b51aa7ae/homeassistant/const.py#L8-L9

Since we will soon align both min-versions, just sticking with 3.4.2 will do.

@fabaff
Copy link
Member

fabaff commented Feb 3, 2018

...whether this attribute is simply ignored for old pips.

It looks that way. Installing aiohttp 2.3.10 with pip 8.0.0 doesn't show an error in regard of the presence of python_requires.

houndci-bot
houndci-bot reviewed Feb 7, 2018
View reviewed changes
setup.py Outdated
MIN_PY_VERSION = '.'.join(map(str,
hass_const.REQUIRED_PYTHON_VER_WIN
if sys.platform.startswith('win')
else hass_const.REQUIRED_PYTHON_VER))
Copy link

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

setup.py Outdated
@@ -61,9 +63,14 @@
'certifi>=2017.4.17',
]

MIN_PY_VERSION = '.'.join(map(str,
hass_const.REQUIRED_PYTHON_VER_WIN
if sys.platform.startswith('win')
Copy link

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

setup.py Outdated
@@ -61,9 +63,14 @@
'certifi>=2017.4.17',
]

MIN_PY_VERSION = '.'.join(map(str,
hass_const.REQUIRED_PYTHON_VER_WIN
Copy link

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

@pelson
Copy link
Contributor Author

pelson commented Feb 7, 2018 •
edited
Loading

We already define the minimum required version in homeassistant.const, can you import it from there? We also have a different minimum version between Windows and other OSes (because of the asyncio loop that we use).

This added a little more complexity, but the current proposal now deals with both minimum versions.

@balloob
Copy link
Member

balloob commented Feb 7, 2018

I knew it would get more complex, that's why I added this sentence at the bottom of my comment 😉

Since we will soon align both min-versions, just sticking with 3.4.2 will do.

But this is even better. Awesome!

@balloob balloob merged commit 9d5dee5 into home-assistant:dev Feb 7, 2018
@pelson pelson deleted the py3_setup branch February 8, 2018 05:03
@balloob balloob mentioned this pull request Feb 9, 2018
Merged
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MartinHjelmare MartinHjelmare MartinHjelmare left review comments

@houndci-bot houndci-bot houndci-bot left review comments

@pvizeli pvizeli pvizeli requested changes

@balloob balloob balloob approved these changes

Assignees
No one assigned
Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@pelson @fabaff @balloob @MartinHjelmare @houndci-bot @pvizeli @homeassistant