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

Keep the source and instead just "make clean" #41

Merged
merged 2 commits into from
Nov 12, 2014
Merged

Keep the source and instead just "make clean" #41

merged 2 commits into from
Nov 12, 2014

Conversation

tianon
Copy link
Member

@tianon tianon commented Nov 10, 2014

As discussed starting at #39 (comment) (hopefully with more magic to come).

Fixes #42 (4d09bc4)

@tianon
Copy link
Member Author

tianon commented Nov 10, 2014

I'm wondering if we should just install and keep autoconf, since it's necessary to phpize anything.

@md5
Copy link
Contributor

md5 commented Nov 10, 2014

@tianon I think +1 on the autoconf installation. I also ran into some pecl installations that needed pkg-config.

@yosifkit
Copy link
Member

So it looks like we need to expand out some of the build dependencies and then add in "runtime" deps (for pecl installs).

@tianon
Copy link
Member Author

tianon commented Nov 10, 2014

Updated:

diff --git a/5.6/Dockerfile b/5.6/Dockerfile
index afbd82c..5e3c4f7 100644
--- a/5.6/Dockerfile
+++ b/5.6/Dockerfile
@@ -3,6 +3,9 @@ FROM debian:jessie
 # persistent / runtime deps
 RUN apt-get update && apt-get install -y ca-certificates curl libxml2 --no-install-recommends && rm -r /var/lib/apt/lists/*

+# phpize deps
+RUN apt-get update && apt-get install -y autoconf pkg-config --no-install-recommends && rm -r /var/lib/apt/lists/*
+
 ##<autogenerated>##
 ##</autogenerated>##

@@ -20,8 +23,6 @@ RUN buildDeps=" \
                libreadline6-dev \
                libssl-dev \
                libxml2-dev \
-               m4 \
-               pkg-config \
        "; \
        set -x \
        && apt-get update && apt-get install -y $buildDeps --no-install-recommends && rm -rf /var/lib/apt/lists/* \

@tianon
Copy link
Member Author

tianon commented Nov 10, 2014

I've been playing with trying to compile FPM directly from this, and haven't had any luck (it's in sapi/fpm), just FYI.

@md5
Copy link
Contributor

md5 commented Nov 10, 2014

Perhaps running a separate build only works for things under ext (assuming it works for extensions other than mbstring).

I don't have time to do this today, but it would be interesting to iterate through the unbuilt extensions under ext and try to phpize them in an container based on this branch. Something like this:

for ext in $(docker run -it --rm -w /usr/src/php/ext php:5.6-kts ls | grep -v -e mysql -e mysqli -e ETC); do
    docker run -it --rm -w "/usr/src/php/ext/$ext" php:5.6-kts phpize
done

That would at least show that phpize can be run for all the extensions in /usr/src/php/ext. Running configure would involve too much figuring out what packages actually need to be install for each extension.

It seems like it's fine to deal with the sapi stuff as a special case.

@tianon
Copy link
Member Author

tianon commented Nov 10, 2014

Maybe we should also setup the following:

  --with-config-file-path=PATH
                          Set the path in which to look for php.ini [PREFIX/lib]
  --with-config-file-scan-dir=PATH
                          Set the path where to scan for configuration files

I'm thinking something like /etc/php5 and /etc/php5/conf.d (especially so we can have this latter one and it'll actually make sense). Does this sound more logical than the /usr/local/lib we've got currently?

@md5
Copy link
Contributor

md5 commented Nov 10, 2014

Having conf.d sounds cool, assuming that option wasn't added to a more recent version of PHP than 5.4.

I also think having the config under /etc makes more sense than having it under /usr, but I think PHP developers are also used to having php.ini be in a number of different locations. As long as it's documented and stable, I don't think it should matter much.

@tianon
Copy link
Member Author

tianon commented Nov 10, 2014

Well, one easy way to find out... 😄

@tianon
Copy link
Member Author

tianon commented Nov 10, 2014

How about /usr/local/etc/php (and /usr/local/etc/php/conf.d), to make it clear this is source-compiled PHP not distro-installed? Then there's less chance of future problems.

@md5
Copy link
Contributor

md5 commented Nov 10, 2014

👍

@yosifkit
Copy link
Member

/me too 👍

For example, the following makes WordPress work:

```Dockerfile
RUN apt-get update && apt-get install -y libpng12-dev && rm -rf /var/lib/apt/lists/* \
	&& docker-php-ext-install gd \
	&& apt-get purge --auto-remove -y libpng12-dev

RUN docker-php-ext-install mysqli
```
@tianon
Copy link
Member Author

tianon commented Nov 11, 2014

Fixes #42 (4d09bc4) 😉

@yosifkit
Copy link
Member

LGTM

@tianon
Copy link
Member Author

tianon commented Nov 11, 2014

@md5 care to take a look at my scripts? 👍

@tianon
Copy link
Member Author

tianon commented Nov 11, 2014

4d09bc4 Add new docker-php-ext-* scripts for magic

For example, the following makes WordPress work:

RUN apt-get update && apt-get install -y libpng12-dev && rm -rf /var/lib/apt/lists/* \
  && docker-php-ext-install gd \
  && apt-get purge --auto-remove -y libpng12-dev

RUN docker-php-ext-install mysqli

@yosifkit
Copy link
Member

So, this will require a change on the WordPress images (which are now FROM php:apache and much simpler), but that will be fine. Just making a note so we remember.

@tianon
Copy link
Member Author

tianon commented Nov 11, 2014

I've got the change staged locally and ready for committing as soon as @md5 is happy with my scripts here. 😈

yosifkit added a commit that referenced this pull request Nov 12, 2014
Keep the source and instead just "make clean"
@yosifkit yosifkit merged commit ad9b781 into docker-library:master Nov 12, 2014
@yosifkit yosifkit deleted the keep-the-sauce branch November 12, 2014 00:24
@yosifkit
Copy link
Member

@md5, if you have issues with this let us know or make a PR. We need these changes to fix wordpress.

@md5
Copy link
Contributor

md5 commented Nov 12, 2014

@tianon: the scripts look fine. Sorry I couldn't review them earlier, but I was AFK today.

The one thing I'd say is that there will probably be people making images based on this one who will end up needing to delete or modify the $ext.ini files. I'm thinking of someone making a "fat" image with extensions installed but not enabled by default. That being said, I expect you've accommodated the main use case by enabling the extensions by default in docker-php-ext-install.

@tianon
Copy link
Member Author

tianon commented Nov 12, 2014

Indeed. I guess if it becomes common enough, we could always add a flag to the script to disable the behavior. However, this is part of the reason I made each "extension" go into a separate file, so it would be easy to do things like docker-php-ext-install mysqli && rm /usr/local/etc/php/conf.d/ext-mysqli.ini without suffering any real harm.

@tianon
Copy link
Member Author

tianon commented Nov 12, 2014

See also docker-library/wordpress#26 for a concrete implementation of this in WordPress. 😄

tianon added a commit to infosiftr/stackbrew that referenced this pull request Nov 12, 2014
- `php`: fix some Apache woes (docker-library/php#43), and add `docker-php-ext-install` and `docker-php-ext-configure` (docker-library/php#41)
- `tomcat`: 7.0.57
- `wordpress`: push more of the PHP+Apache logic into `php` where it belongs (docker-library/wordpress#25), and update the image to use the new `docker-php-ext-install` magic for the extra core extensions it needs (docker-library/wordpress#26)
tianon added a commit to infosiftr/stackbrew that referenced this pull request Nov 12, 2014
- `php`: fix some Apache woes (docker-library/php#43), and add `docker-php-ext-install` and `docker-php-ext-configure` (docker-library/php#41)
- `tomcat`: 7.0.57
- `wordpress`: push more of the PHP+Apache logic into `php` where it belongs (docker-library/wordpress#25), and update the image to use the new `docker-php-ext-install` magic for the extra core extensions it needs (docker-library/wordpress#26)
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.

Tool to docker-phpize
3 participants