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

Add HHVM image #1043

Closed
wants to merge 7 commits into from
Closed

Conversation

baptistedonaux
Copy link

The PR documentation is here docker-library/docs#339

@baptistedonaux
Copy link
Author

Image can be pull at baptistedonaux/hhvm

@md5
Copy link
Contributor

md5 commented Sep 11, 2015

@baptistedonaux I'm curious why you didn't use the prebuilt hhvm package for Debian. It seems to be well-supported by the HHVM team and is the first thing they recommend in the docs about installing HHVM:

Refer to our directions on installing a prebuilt package. You can build from source if you really want, but doing that doesn't include nice things like our init script or automatic updates via apt-get, and so isn't recommended until you get a basic install working and have a better feel for what you're doing.

I did a POC Dockerfile using the Debian package a while back and it was dead simple: https://gist.github.com/md5/3b8cef5bc53ac0b54f69

I expect that building it from source will result in a smaller image due to leaving out some of the "nice things" they mention that aren't really needed in a Docker image, but I'm curious what else it leaves out and whether it's worth the maintenance burden.

@baptistedonaux
Copy link
Author

Hi @md5 ! I know the Dockerfile recommendations and these recommendations are really good. But I really reflected on the method to use.

I made the choice to build HHVM and not use pre build debian package because, in my case I like use a specific version of HHVM. Whereas HHVM repository gives only the LTS versions (3.3, 3.6, 3.9) and latest stable.

With this build, the user can choice the version.

Your feedback is really important for me because, this solution can be good (but the implementation can be improved), or it's a bad solution and my problem concerns only me.

Thank you for your contribution.

@md5
Copy link
Contributor

md5 commented Sep 14, 2015

That makes sense. I thought that more versions were available since they're available in the pool directory: http://dl.hhvm.com/debian/pool/main/h/hhvm/

Now that I look at the actual Packages file for jessie, it looks like they don't include all of those versions there: http://dl.hhvm.com/debian/dists/jessie/main/binary-amd64/Packages

In terms of maintaining the official images themselves, I don't know that there's a need to have every minor version available since not all of those versions will be supported tags. I do think there's value in having a Dockerfile that shows how to build HHVM from source for those who want to build their own image for specific versions, but I'm torn on whether that's the approach the official image should take.

@baptistedonaux
Copy link
Author

I work on new integration which will use pre build packages that you have found in the official repository.

@md5
Copy link
Contributor

md5 commented Sep 15, 2015

@baptistedonaux I wasn't necessarily suggesting using those packages, just pointing out that I had seen that they're there. Since they aren't listed in Packages, it doesn't seem like there's any guarantee that they won't be deleted at some point. That may mean that your approach of simply pulling the source from GitHub and building may be better long term.

@tianon
Copy link
Member

tianon commented Sep 15, 2015 via email

@md5
Copy link
Contributor

md5 commented Sep 15, 2015

@tianon 👍

@baptistedonaux
Copy link
Author

I understand that the community prefers using pre-build package because the official documentation recommends it.

So, I propose you, an other version to build/use hhvm with specific version (not latest which use apt-get install hhvm).

It's an solution not entirely pleasing but really simple to maintain.

https://github.com/baptistedonaux/docker-images/blob/hhvm/hhvm/3.9/Dockerfile
(This solution is on an other branch)

@baptistedonaux
Copy link
Author

@tianon @md5 What's your preference between hard compilation or install downloaded package ?

@md5
Copy link
Contributor

md5 commented Sep 25, 2015

@baptistedonaux I can't speak for @tianon, but I think this comment indicates a preference for packages. I would tend to agree in terms of maintaining an hhvm image for Docker Hub, though that doesn't mean that having a source-compiled image isn't a useful alternative.

@tianon
Copy link
Member

tianon commented Sep 25, 2015

To be perfectly clear, what I'd like to see in the ideal case (since the packages are built, maintained, and recommended directly by upstream), is something like the following.

For latest:

FROM debian:jessie

# pub   2048R/1BE7A449 2013-12-25
#       Key fingerprint = 36AE F64D 0207 E7EE E352  D487 5A16 E728 1BE7 A449
# uid                  Paul Tarjan <[email protected]>
# sub   2048R/452A652F 2013-12-25
RUN apt-key adv --keyserver ha.pool.sks-keyservers.net --recv-keys 36AEF64D0207E7EEE352D4875A16E7281BE7A449

RUN echo 'deb http://dl.hhvm.com/debian jessie main' > /etc/apt/sources.list.d/hhvm.list
# could also be: (but not sure if this will hold for future releases based on their LTS status)
#RUN echo 'deb http://dl.hhvm.com/debian jessie-lts-3.9 main' > /etc/apt/sources.list.d/hhvm.list

ENV HHVM_VERSION 3.9.1~jessie

RUN apt-get update && apt-get install -y --no-install-recommends \
        hhvm=$HHVM_VERSION \
    && rm -rf /var/lib/apt/lists/*

For older versions (like the 3.6 series, which is also LTS):

FROM debian:jessie

# pub   2048R/1BE7A449 2013-12-25
#       Key fingerprint = 36AE F64D 0207 E7EE E352  D487 5A16 E728 1BE7 A449
# uid                  Paul Tarjan <[email protected]>
# sub   2048R/452A652F 2013-12-25
RUN apt-key adv --keyserver ha.pool.sks-keyservers.net --recv-keys 36AEF64D0207E7EEE352D4875A16E7281BE7A449

RUN echo 'deb http://dl.hhvm.com/debian jessie-lts-3.6 main' > /etc/apt/sources.list.d/hhvm.list

ENV HHVM_VERSION 3.6.6~jessie

RUN apt-get update && apt-get install -y --no-install-recommends \
        hhvm=$HHVM_VERSION \
    && rm -rf /var/lib/apt/lists/*

@baptistedonaux
Copy link
Author

@tianon @md5 I worked on the image with your recommendations.

I use the prebuild package and propose latest and LTS releases.

@md5
Copy link
Contributor

md5 commented Oct 16, 2015

LGTM

Looks like it would be a good idea to add update.sh and generate-stackbrew-library.sh scripts, similar to the other official image repos.

@md5
Copy link
Contributor

md5 commented Oct 16, 2015

Actually, I noticed that you're using ENTRYPOINT ["hhvm"]. This is going to make it difficult to do something like docker run --rm -it hhvm bash (the user would have to do docker run --rm -it --entrypoint bash hhvm instead). Generally an exception is made for binary-only images, but I don't see any reason that command shouldn't be possible with this image. You might want to add a simple entrypoint.sh script as is done for many of the other repositories.

@baptistedonaux
Copy link
Author

Great ! I take notes and I come back with required scripts.

@md5
Copy link
Contributor

md5 commented Oct 19, 2015

@baptistedonaux The update.sh and generate-stackbrew-library.sh scripts aren't required, but they're nice to have for consistency and make maintaining the image easier.

The entrypoint changes would be needed to pass the override-cmd test. It looks like you may have already started testing out the hhvm image with the official images tests, so hopefully you're good there.

@tianon
Copy link
Member

tianon commented Oct 19, 2015

Looks really good! 👍

Build test of #1043; 9d4d069 (hhvm):

$ url="https://raw.githubusercontent.com/docker-library/official-images/9d4d069a279a9dc077f90bd70f7494bdaf0fb23d/library/hhvm"
$ bashbrew build "$url"
Cloning hhvm (git://github.com/baptistedonaux/docker-images) ...
Processing hhvm:3.3 ...
Processing hhvm:3.3.7 ...
Processing hhvm:3.6 ...
Processing hhvm:3.6.6 ...
Processing hhvm:3.9 ...
Processing hhvm:3.9.1 ...
Processing hhvm:3 ...
Processing hhvm:3.10 ...
Processing hhvm:3.10.0 ...
Processing hhvm:latest ...
$ bashbrew list --uniq "$url" | xargs test/run.sh
testing hhvm:3.3
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...failed
testing hhvm:3.6
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...failed
testing hhvm:3.9
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...failed
testing hhvm:3
    'utc' [1/4]...passed
    'cve-2014--shellshock' [2/4]...passed
    'no-hard-coded-passwords' [3/4]...passed
    'override-cmd' [4/4]...failed

The only thing left that I can see is the ENTRYPOINT issue @md5 pointed out and I think we're ready to move forward.

@baptistedonaux
Copy link
Author

@md5 @tianon

update.sh and generate-stackbrew-library.sh added !

I think it's definitely good ! :)

@baptistedonaux
Copy link
Author

Travis fixed !

@tianon
Copy link
Member

tianon commented Oct 21, 2015

I'm still not understanding why we're setting an entrypoint. What about users who don't want to use fastcgi? What about users who don't want a web server at all? I could understand something like what we've got for memcached (https://github.com/docker-library/memcached/blob/5c034b3b8ab7216181f3c25f080b4d1dbb8bf888/docker-entrypoint.sh; and the default being to run in fastcgi mode like it is now), but in general, I think users will want to do other things with HHVM in a container than just run a fastcgi server, right?

As an additional thought, has upstream been contacted to see if they're interested in being involved?

@md5
Copy link
Contributor

md5 commented Oct 21, 2015

@tianon It seems to me that having this parallel the php image and having cli and fpm variants could make sense.

@tianon
Copy link
Member

tianon commented Oct 21, 2015 via email

@md5
Copy link
Contributor

md5 commented Oct 21, 2015 via email

@baptistedonaux
Copy link
Author

@md5 It's exactly that. HHVM can be use like FastCGI server and/or CLI mode. In the first proposition, I added /usr/bin/hhvm in entrypoint and server option in command.

I transformed entrypoint and command in only entrypoint but it's mistake of me. I remove now ENTRYPOINT and set HHVM server command in CMD.

@baptistedonaux
Copy link
Author

Done !

entrypoint.sh isn't necessary now so I remove it.

ENTRYPOINT has been remove and CMD contains HHVM server call.

docker run hhvm:latest runs a FastCGI server.
docker run hhvm:latest hhvm foo.php runs a script in CLI mode.

With these changements, docker run hhvm:latest bash works and the command can be override.

@md5
Copy link
Contributor

md5 commented Oct 22, 2015

Thanks @baptistedonaux! 👍

I've opened a small PR that adds back a simple entrypoint.sh script at baptistedonaux/docker-images#1

This will allow things like docker run hhvm:latest foo.php to work as well. I tested this out both with a non-executable foo.php and one that was chmod +x and had #!/usr/bin/env hhvm as a shebang line. It might be nice to add tests for those scenarios specifically, as well as some that pass flags (e.g. docker run hhvm --version).

One more thing I might suggest is that having this in its own docker-hhvm repo will make diffs easier to review. This doc can help with doing that while preserving the history: https://help.github.com/articles/splitting-a-subfolder-out-into-a-new-repository/

@baptistedonaux
Copy link
Author

@md5 PR accepted and links updated to baptistedonaux/docker-hhvm repository.

@yosifkit
Copy link
Member

This is looking great. A few more points:

  • has upstream been contacted to see if they're interested in being involved?
  • in hhvm:3.10, the entrypoint needs a chmod +x in git 😉

@baptistedonaux
Copy link
Author

Execution right fixed on master/entrypoint.sh.

@baptistedonaux
Copy link
Author

Images tested. Entrypoint works in CLI and FastCGI modes.

All these changements have added errors. I hope this is fixed now.

entrypoint works
CLI/FastCGI modes works
documentation completed
update.sh and generate-stackbrew-library.sh works
source code move in new repsoitory (baptistedonaux/docker-hhvm)

@tianon @md5 @yosifkit Do you have others improvements ?

@md5
Copy link
Contributor

md5 commented Oct 23, 2015

@baptistedonaux Nothing comes to mind. Thanks for working on this! 👍

@baptistedonaux
Copy link
Author

ping @tianon

@tianon
Copy link
Member

tianon commented Nov 5, 2015

With regards to reaching out to upstream, @yosifkit opened an issue with them that's currently getting some decent discussion: hhvm/packaging#138

@yosifkit
Copy link
Member

yosifkit commented Nov 9, 2015

I'm really sorry to make you go through all this work 😞. It seems upstream would rather that it come from them, hhvm/packaging#138 (comment). Thanks anyway for contributing.

@baptistedonaux
Copy link
Author

@yosifkit Thank you for your feedback. I'm really sad that this work can't be use like official image and help the community.

I hope that Facebook will propose a good image now that they want to recover the baby.

Thank you @yosifkit @tianon and @md5 for the help given.

My image stays available if it can help anyone.

@daghack
Copy link
Contributor

daghack commented Dec 18, 2015

I'm going to go ahead and close this - if anything changes with upstream such that this image would be the most appropriate official image candidate for hhvm, we can reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants