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 additional modules to include #27

Merged
merged 3 commits into from
Oct 27, 2014
Merged

Add additional modules to include #27

merged 3 commits into from
Oct 27, 2014

Conversation

daghack
Copy link
Contributor

@daghack daghack commented Oct 24, 2014

Add in support for the gd, curl, and gzip php modules.

@tianon
Copy link
Member

tianon commented Oct 24, 2014

+1, but can you add it to all the other versions too? 😉

@thaJeztah
Copy link
Contributor

Should --with-mysql still be included? The mysql extension is deprecated as of PHP 5.5; http://php.net/manual/en/intro.mysql.php

Maybe only keep it in the PHP5.4 image?

@daghack
Copy link
Contributor Author

daghack commented Oct 24, 2014

Oh, yes, silly me. One of these days, I'll get on of these PR's accepted without you having issues with it, if it's the last thing I do, tianon ;P

As far as --with-mysql being included, you may be correct, but that's not something this PR was meant to address. IMO, that should be put as a separate issue, rather than an issue with this particular PR.

@tianon
Copy link
Member

tianon commented Oct 24, 2014

Yeah, I'm still +1 on keeping --with-mysql, but definitely agree it should be a separate PR to discuss that (as that's completely unrelated to this one).

@thaJeztah
Copy link
Contributor

should be put as a separate issue, rather than an issue with this particular PR

+1 I just noticed that it was still there and thought it would be an easy fix while you were editing those files. I'm not behind my computer ATM so not able to create a PR.

@daghack
Copy link
Contributor Author

daghack commented Oct 24, 2014

I updated to include the other versions.

Additionally, I opened issue #28 so that we can move the discussion of the --with-mysql flag away from here.

@tianon
Copy link
Member

tianon commented Oct 24, 2014

LGTM; did you build test all three? :)

@daghack
Copy link
Contributor Author

daghack commented Oct 24, 2014

That I did.

@tianon
Copy link
Member

tianon commented Oct 25, 2014

👍

cc @yosifkit

@thaJeztah
Copy link
Contributor

Thanks @Moghedrin!

@thaJeztah
Copy link
Contributor

@Moghedrin I noticed you didn't update the apache versions; I know there's an update.sh script that handles this, but should PRs update both the regular and the apache versions? (not sure)

@yosifkit
Copy link
Member

Good point @thaJeztah. @Moghedrin can you run the update.sh to get the apache versions too?

@thaJeztah
Copy link
Contributor

Slightly related; the update.sh script doesn't work on OS X 10.9.5. It gives an "invalid argument" error. Probably something with awk versions or so, but I didn't look into it properly yet. On Ubuntu it worked correctly.

@yosifkit
Copy link
Member

That would be the readlink -f. OS X does not have the same GNU readlink.

@thaJeztah
Copy link
Contributor

Could be yes; do you want me to look into it? Or just leave it as a "known issue". Manually updating the Dockerfiles isn't too much hassle right now.

@yosifkit
Copy link
Member

It will be a problem in all the Official Images scripts (since readlink -f is such a nice feature and is a known problem in OS X), "known issue" is fine for now.

@daghack
Copy link
Contributor Author

daghack commented Oct 27, 2014

Silly me. I've updated the apache versions as well :)

@yosifkit
Copy link
Member

LGTM

1 similar comment
@tianon
Copy link
Member

tianon commented Oct 27, 2014

LGTM

tianon added a commit that referenced this pull request Oct 27, 2014
Add additional modules to include
@tianon tianon merged commit ba38d4a into docker-library:master Oct 27, 2014
@tianon tianon deleted the additionalModules branch October 27, 2014 22:21
@thaJeztah
Copy link
Contributor

👍

@md5
Copy link
Contributor

md5 commented Oct 28, 2014

Looks like --with-gzip should have been --with-zlib:

configure: WARNING: unrecognized options: --with-gzip

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.

None yet

5 participants