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

Build libcec for Docker image #5230

Merged
merged 3 commits into from
Jan 9, 2017
Merged

Build libcec for Docker image #5230

merged 3 commits into from
Jan 9, 2017

Conversation

emlove
Copy link
Contributor

@emlove emlove commented Jan 8, 2017

Description:
Adds libcec to the docker image.

@mention-bot
Copy link

@armills, thanks for your PR! By analyzing the history of the files in this pull request, we identified @auduny, @balloob and @allanglen to be potential reviewers.

@michaelarnauts
Copy link
Contributor

FYI, this won't work for rpi, since you need to have the development headers for broadcom installed. See https://github.com/Pulse-Eight/libcec/blob/master/docs/README.raspberrypi.md, but I'm not sure if hass runs on docker on rpi.

Could you also add these changes to virtualization/Docker/Dockerfile.dev so the development image matches the production one?

@emlove
Copy link
Contributor Author

emlove commented Jan 9, 2017

I updated the PR to modify the development Dockerfile. I do think it's a good idea to include the Broadcom development headers for the rpi, but I think someone who can test that is going to need to add it.

@michaelarnauts
Copy link
Contributor

The problem with rpi is that the cmake argements are different. So a different docker image should be made depending on where the image will be ran, and that is against the purpose of docker images...

@emlove
Copy link
Contributor Author

emlove commented Jan 9, 2017

Looking quickly at their build system it looks like if the rpi headers are included it builds rpi support alongside support for other devices, so they could all be run from the same image. I could be wrong there. I agree building different images for different targets is a bad idea.

@balloob
Copy link
Member

balloob commented Jan 9, 2017

Well you sadly will need different images for things like an RPi because it has a different architecture, so you usually depend on a different base image that already has a kernel that supports the image. There are some people that have suggested RPi Dockerfile but until now I have denied officially supporting them because they need to be manually build. We do have a community one at https://hub.docker.com/r/homeassistant/rpi2-home-assistant/

mkdir -p build && cd build

cmake \
-DPYTHON_LIBRARY="/usr/local/lib/libpython3.5m.so" \
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this depend on the current running version of Python? Because I was planning on updating our docker image to use Python 3.6 but I don't want such updates to be blocked by installing a dependency.

@emlove
Copy link
Contributor Author

emlove commented Jan 9, 2017

Updated to dynamically load python paths from the active python executable. I've also opened an issue upstream to track the hardcoded Debian path in their cmake files: Pulse-Eight/libcec#288

@balloob balloob merged commit c7249a3 into home-assistant:dev Jan 9, 2017
@emlove emlove deleted the docker-libcec branch January 9, 2017 16:49
apt-get clean && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*

COPY script/build_python_openzwave script/build_python_openzwave
RUN script/build_python_openzwave && \
mkdir -p /usr/local/share/python-openzwave && \
ln -sf /usr/src/app/build/python-openzwave/openzwave/config /usr/local/share/python-openzwave/config

COPY script/build_libcec script/build_libcec
RUN script/build_libcec
Copy link
Contributor

Choose a reason for hiding this comment

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

You should remove the build directory after the script invocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

This wasn't done in this PR.
-> #5243

@home-assistant home-assistant locked and limited conversation to collaborators Apr 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants