-
-
Notifications
You must be signed in to change notification settings - Fork 32k
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
PyPI Openzwave #7415
PyPI Openzwave #7415
Conversation
PYOZW now has much more comprehensive default handling for the config path (in src-lib/libopenzwave/libopenzwave.pyx:getConfig()). It looks in the same place we were looking, plus _many_ more. It will certainly do a much better job of finding the config files than we will (and will be updated as the library is changed, so we don't end up chasing it). The getConfig() method has been there for a while, but was subsntially improved recently. This change simply leaves the config_path as None if it is not specified, which will trigger the default handling in PYOZW.
As of version 0.4, python-openzwave supports installation from PyPI, which means we can use our 'normal' dependency management tooling to install it. Yay. This uses the default 'embed' build (which goes and downloads statically sources to avoid having to compile anything locally). Check out the python-openzwave readme for more details.
@JshWright, thanks for your PR! By analyzing the history of the files in this pull request, we identified @turbokongen, @fabaff and @balloob to be potential reviewers. |
from pydispatch import dispatcher | ||
# pylint: disable=import-error | ||
from openzwave.option import ZWaveOption | ||
from openzwave.network import ZWaveNetwork | ||
from openzwave.group import ZWaveGroup | ||
|
||
default_zwave_config_path = os.path.join(os.path.dirname( |
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.
Is the configuration not a part of the package? That is unfortunate. I wonder if we should auto download the config in a future PR. Let's make sure that we note in the docs how to obtain it.
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.
No, it does, it's just in a different place. The behavior when config_path is None is now basically an improved version of what we were doing (it checks the new location, along with a number of other places where it has been historically). There's no longer a need for us to get clever with the default path.
Not sure what's up with Travis. Tests run fine in a clean virtualenv locally. Will have to take a look tomorrow. |
|
If this gets merged its also worth at the same time removing; virtualization/Docker/scripts/python_openzwave As it won't be needed and; virtualization/Docker/setup_docker_prereqs and;
|
I think the travis errors actually due to; b'/tmp/pip-build-jwlooorh/python-openzwave/openzwave-embed/open-zwave-master/cpp/hidapi/linux/hid.c:46:21: fatal error: libudev.h: No such file or directory\n' You need libudev deb pkg installed to compile it. |
@eperdeme good catch. I added |
This is getting messy. Now we need to apt-get install a bunch of stuff. Since we don't need this dependency for tests, we should consider to rollback all changes to travis.yml and instead make it one of the requirements that we comment out: https://github.com/home-assistant/home-assistant/blob/dev/script/gen_requirements_all.py#L9 The only downside to that is that it will also make sure that it won't get installed in the Docker container, although I guess we can address that. |
Whats the downside of |
Container based env on travis ( sudo: false
addons:
apt:
packages:
- libudev-dev https://docs.travis-ci.com/user/installing-dependencies/#Installing-Packages-with-the-APT-Addon |
Got this build time the second time after a caching run: |
Python OpenZwave require the libudev headers to build. This adds the libudev-dev package to Travis runs via the 'apt' addon for Travis. Thanks to @MartinHjelmare for this fix.
Thanks for the .travis.yml tweaks @MartinHjelmare. I've force pushed an update to this branch including those tweaks (I ended up adding a new commit, as your commit had a couple other things included in it). |
Now that PYOZW can be install from PyPI, the docker image build process can be simplified to remove the explicit compilation of PYOZW.
I think the docker should keep `libudev-dev |
@andrey-git it has been added to the main Docker install script. I think that this is ok to merge. The 0.44 branch has been cut so it means that people will be able to test this for two weeks. This PR has slowed down the tests (as Travis now has to compile OpenZWave). I am planning to update |
Any idea on how to toggle this and build your own version OZW and PYOZW in the docker image? I copy/tweaked the old python_openzwave and call it in the docker build, but that didn't work. Also tried to:
and then build/install python_openzwave and that didn't work. I next attempt will be to patch requirements_all.txt and homeassistant/components/zwave/init.py and see if that might be it (remove the requirement). The reason I need this is I need to patch OZW and PYOZW to add support for the Linear NGDZ00-4 Garage Door |
Home Assistant will not install the python Open Z-Wave dependency if it has already been installed in the current Python environment. Make sure the version number is the same though. |
Just following up: So it looks like you have to git clone the exact version of python-openzwave from requirements_all.txt otherwise it will uninstall it and re-install the version the requirements_all.txt version. It seems python-openzwave 0.4.x changed the way it builds and installs. A build target will git clone OZW in the openzwave directory and build it. A install target will download a OZW source tar file, expand it to openzwave-embed, build and install it with the rest of python-openzwave. Once I figured that out and patched both copies of OZW, I have a working zwave garage door cover again. I am testing the 3.6 image change now and will also test just doing a install instead of build/install. |
Description:
Initial PR to switch to PyPI based openzwave installation. Documentation PR forthcoming shortly.
Related issue (if applicable): fixes #7403
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#
Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
.