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

PyPI Openzwave #7415

Merged
merged 4 commits into from May 5, 2017
Merged

PyPI Openzwave #7415

merged 4 commits into from May 5, 2017

Conversation

JshWright
Copy link
Contributor

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:

  • 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.

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.
@mention-bot
Copy link

@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(
Copy link
Member

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.

Copy link
Contributor Author

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.

@JshWright
Copy link
Contributor Author

Not sure what's up with Travis. Tests run fine in a clean virtualenv locally. Will have to take a look tomorrow.

@balloob
Copy link
Member

balloob commented May 3, 2017

    g++ -pthread -shared -L/opt/python/3.4.2/lib -Wl,-rpath=/opt/python/3.4.2/lib build/temp.linux-x86_64-3.4/openzwave-embed/open-zwave-master/python-openzwave/src-lib/libopenzwave/libopenzwave.o openzwave-embed/open-zwave-master/libopenzwave.a -L/opt/python/3.4.2/lib -ludev -lstdc++ -lresolv -lpython3.4m -o build/lib.linux-x86_64-3.4/libopenzwave.cpython-34m.so
    g++: error: openzwave-embed/open-zwave-master/libopenzwave.a: No such file or directory
    error: command 'g++' failed with exit status 1

@eperdeme
Copy link

eperdeme commented May 3, 2017

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;

virtualization/Docker/setup_docker_prereqs
-  cython3 libudev-dev
+  libudev-dev

@eperdeme
Copy link

eperdeme commented May 3, 2017

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'
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.

@balloob
Copy link
Member

balloob commented May 3, 2017

@eperdeme good catch. I added apt-get install libudev-dev to our travis.yml, see if this can get it building.

@balloob
Copy link
Member

balloob commented May 3, 2017

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.

@andrey-git
Copy link
Contributor

Whats the downside of apt-get install ?
Although we don't need it for tests - it follows more closely the environment the users need to have.

@MartinHjelmare
Copy link
Member

MartinHjelmare commented May 5, 2017

Container based env on travis (sudo: false) has much faster boot time. Using the APT addon is preferred over sudo apt-get install. Then you can keep sudo: false if setting system_site_packages: true under virtualenv. Edit: system_site_packages is not relevant since it's not a python package in question, and that would only have worked on python 2, anyway. Eg:

sudo: false
addons:
  apt:
    packages:
      - libudev-dev

https://docs.travis-ci.com/user/installing-dependencies/#Installing-Packages-with-the-APT-Addon

@MartinHjelmare
Copy link
Member

Got this build time the second time after a caching run:
https://travis-ci.org/MartinHjelmare/home-assistant/builds/229042786

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.
@JshWright
Copy link
Contributor Author

JshWright commented May 5, 2017

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.
@andrey-git
Copy link
Contributor

I think the docker should keep `libudev-dev

@balloob
Copy link
Member

balloob commented May 5, 2017

@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 gen_requirements_all.py to make a specific test file that just installs the dependencies necessary for tests. That way we should get a pretty good boost.

@balloob balloob merged commit 2e4ae3e into home-assistant:dev May 5, 2017
@JshWright JshWright deleted the pypi-openzwave branch May 5, 2017 22:11
@hawk259
Copy link
Contributor

hawk259 commented May 7, 2017

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:

pip3 uninstall -y python_openzwave

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

@balloob
Copy link
Member

balloob commented May 7, 2017

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.

@hawk259
Copy link
Contributor

hawk259 commented May 9, 2017

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.

@home-assistant home-assistant locked and limited conversation to collaborators Aug 12, 2017
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.

Z-Wave: migrate to pip package
9 participants