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

[tradfri] Update pytradfri, simplify dependencies. #9875

Merged
merged 13 commits into from
Oct 20, 2017
Merged

Conversation

lwis
Copy link
Member

@lwis lwis commented Oct 15, 2017

Description:

Is there any way to pass --process-dependency-links as part of REQUIREMENTS in a component? I see components.nuimo_controller is passing --only-binary=all - but when I pass anything else it fails with a parse error on --proces?

In lieu of a solution, I'm installing all deps via pip in the component.

Related issue (if applicable): fixes #9778

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#3558

Example entry for configuration.yaml (if applicable):

  discover:

Checklist:

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][ex-requir]).
  • New dependencies are only imported inside functions that use them ([example][ex-import]).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

@lwis lwis changed the title [tradfri] update pytradfri [tradfri] Update pytradfri, simplify dependencies. Oct 15, 2017
@pvizeli
Copy link
Member

pvizeli commented Oct 15, 2017

That's not possible to do that only with pytradfri? All this requirements make it complex

@lwis
Copy link
Member Author

lwis commented Oct 15, 2017

@pvizeli check the top of the PR description, I can't use the extra without processing dependency links. Besides, this is far cleaner than it was before. Once aiocoap releases 0.4, this just becomes pytradfri[async]. However, an aiocoap release isn't planned for a while afaik.

@balloob
Copy link
Member

balloob commented Oct 16, 2017

We should ask the author of aiocoap to publish a pre-release to PyPi. That way normal installs of aiocoap will still install 0.3 but we can install 0.4-pre.

@balloob
Copy link
Member

balloob commented Oct 16, 2017

I'm surprised we need to do this, afaik, pip will always also install dependencies. So if pytradfri has set it up correctly, we should get it correctly

@lwis
Copy link
Member Author

lwis commented Oct 16, 2017

It'll ignore dep links unless instructed to follow them, and doesn't pip require a flag for pre releases? --pre.

Also what would you like me to do about aiocoap requiring 3.4.4?

@balloob
Copy link
Member

balloob commented Oct 16, 2017

If you pin the version of requirements, you can install a pre version.

Oh I see now how I misunderstood. You can't put it in normal packages keyword because it's a GitHub repo.

Uuuuuuugh about Python 3.4.4. Debian Jessie is still 3.4.2 :-|

@Landrash
Copy link
Contributor

@balloob Breaking Jessie isn't that bad is it? We got upgrade path's for all the install options with the exception of the AiO script?

@andrey-git
Copy link
Contributor

If we break Jessie we might as well migrate to 3.5 :)

@lwis
Copy link
Member Author

lwis commented Oct 16, 2017

@Landrash do you have the metrics on 3.4.2 users?

Besides, didn't we only just deprecate 3.4 support?

@balloob
Copy link
Member

balloob commented Oct 16, 2017

We announced we are going to deprecate 3.4 support in 2018. We can't jump the gun and do it now. Upgrading a Python version can require some time so I want to give our users some time.

I want to make an exception for Pytradfri and allow it to require a minimal Python of 3.4.4. This is an exception and will not be granted for any other component.

Add aiocoap to https://github.com/home-assistant/home-assistant/blob/dev/script/gen_requirements_all.py#L9 so it will be commented out and not being installed during the linting test.

Make sure to add a comment to the docs.

@lwis
Copy link
Member Author

lwis commented Oct 16, 2017

@balloob ok, I'll make the change now. Which docs are you referring to? I'll leave a line comment in the gen script.

Also, this can be removed once we're on 3.5 min.

@lwis
Copy link
Member Author

lwis commented Oct 16, 2017

Argh, so it looks like DTLSSocket's setup.py references Cython. Any suggestions on how that dep can be resolved?

@balloob
Copy link
Member

balloob commented Oct 17, 2017

I meant the platform docs on the website.

Comment out DTLSSocket too? 😢

@lwis
Copy link
Member Author

lwis commented Oct 17, 2017

Ah ok, will do.

But what about runtime installation? Should I leave a comment on the platform docs for cython also? 😔

@balloob
Copy link
Member

balloob commented Oct 17, 2017

Runtime installation wont work for hass.io because we remove build tools. Cc @pvizeli

Probably need to add cython to docs and docker ?

@balloob balloob merged commit d16c5f9 into dev Oct 20, 2017
@balloob balloob deleted the tradfri-dep-update branch October 20, 2017 06:20
@balloob balloob added this to the 0.56 milestone Oct 21, 2017
fabaff pushed a commit that referenced this pull request Oct 21, 2017
* Update pytradfri

* Process dep links

* Process dep links

* Process dep links

* Install all deps

* Update requirements

* Exclude aiocoap

* Install cython

* Remove cython

* Exclude DTLSSocket

* Add cython
@balloob balloob mentioned this pull request Oct 21, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Mar 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tradfri groups iteration TypeError with async
8 participants