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

Enh/docker #238

Merged
merged 4 commits into from
Nov 3, 2017
Merged

Enh/docker #238

merged 4 commits into from
Nov 3, 2017

Conversation

tristan0x
Copy link
Member

On my way to make braynsViewer works within containers, here is a small contribution to shrink both Docker image size and build time.

Image size is now 437MB instead of >1GB

The patch leverages multi-stage builds, a feature released in Docker 17.05.
More information available in the official user guide

An intermediate image is used to build Brayns.
The delivered image only provides the result of the compilation
and runtime libraries.

New image size is 437MB!

Build time is also improved because there is no need to cleanup
the *build* image anymore.
@favreau
Copy link
Member

favreau commented Nov 1, 2017

"On my way" means that your are still working on it, on not ready for review yet?
In any case, this is super cool! Many thanks for your contribution! :)

Dockerfile Outdated
ENV LD_LIBRARY_PATH $LD_LIBRARY_PATH:${DIST_PATH}/lib
ENV PATH $PATH:${DIST_PATH}/bin

ENV LD_LIBRARY_PATH $LD_LIBRARY_PATH:/app/dist/lib
Copy link
Contributor

Choose a reason for hiding this comment

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

why not reusing DIST_PATH instead of hard-coding /app/dist/lib?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, just use DIST_PATH.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. You are right, it should not be hardcoded. I will make the required changes.
  2. Besides ENV and ARG are not available in other build stages. they must be redefined.
  3. I think we should use ARG instead of ENV as DIST_PATH does not seem to be a required variable in the container. Anybody against?

@rolandjitsu
Copy link
Contributor

@tristan0x looks great, I was planning on using multi step builds, but never got the time to do it.

Copy link
Contributor

@rolandjitsu rolandjitsu left a comment

Choose a reason for hiding this comment

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

@tristan0x Just use the dist var and we'll merge.

@tristan0x
Copy link
Member Author

@favreau Regarding BraynViewer, it is still work in progress.

@tristan0x
Copy link
Member Author

Hello @rolandjitsu
Does the last change suits you?

@rolandjitsu rolandjitsu merged commit 3e12cf0 into BlueBrain:master Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants