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

Remove php src the right way for lightweight containers. #245

Closed
wants to merge 2 commits into from
Closed

Remove php src the right way for lightweight containers. #245

wants to merge 2 commits into from

Conversation

shouze
Copy link
Contributor

@shouze shouze commented Jun 23, 2016

Fixes #234.

  • I will call update.sh after the proposed patch have been reviewed.
  • I've also included some kind of debian support.

According to @yosifkit #234 (comment) we should obtain that kind of result (uncompressed size container images):

REPOSITORY          TAG                IMAGE ID            CREATED             VIRTUAL SIZE
# current size
php                 alpine             bdae0b0de098        3 days ago          191.5 MB

# sans all source
<none>              <none>             537592e5c872        53 seconds ago      44.3 MB

# keep the php src xz
<none>              <none>             cb1f5c08b4f9        50 minutes ago      55.79 MB

&& mv "/usr/src/php-$PHP_VERSION" /usr/src/php \
&& rm "$PHP_FILENAME" \
&& docker-php-source download \
&& docher-php-source extract \
Copy link

Choose a reason for hiding this comment

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

Should be docker-php-source extract \

@shouze
Copy link
Contributor Author

shouze commented Jun 28, 2016

@cjunge thx for noticing me the typo... 🙀

@shouze
Copy link
Contributor Author

shouze commented Jun 29, 2016

ping @tianon @yosifkit any feedback? I can run update.sh if you will.

@shouze shouze mentioned this pull request Jun 29, 2016
@yosifkit
Copy link
Member

To simplify this I would rather leave the downloading in the Dockerfile and keep the tar.gz file around. This will make the new script only need to do two things, extract and delete. This will also guarantee that any source compiled against in your modules is the same that built the version of php in use and lessen the impact on php servers as well as time on users.

How much time does the extraction and deletion add to a typical docker-php-ext-install? We should probably do some benchmarks beyond size.

This also has ramifications on the docker-php-ext-* scripts since they use /usr/src/php/ext to figure out what extensions are available in their 'usage' sections (see enable, configure, and install). Minimally, we should extract at the start of the file and then delete with logic similar to the apk deletes (configure should not delete, install should delete, and I'm not sure about enable).

@shouze
Copy link
Contributor Author

shouze commented Jul 1, 2016

@yosifkit ok, will make the changes soon & report numbers about extract/delete times.

@shouze
Copy link
Contributor Author

shouze commented Jul 3, 2016

ping @yosifkit ok done.

About docker-php-ext-*:

  • enable: nothing to do
  • configure only extract from tarball if not already done.
  • install: extract from tarball if not already done then delete.

The only last possible improvement I see is... if somebody decide in its inherited Dockerfile RUN to call manually at the beginning docker-php-source extract then docker-php-ext-install maybe should not delete everytime at the end and then docker-php-source delete should be explicitly at the end of the Dockerfile RUN.

I will make some benchmark about the extra consumed time by extract process.

@shouze
Copy link
Contributor Author

shouze commented Jul 3, 2016

@yosifkit benchmark done... on my local machine the extract step is very fast:

time tar -Jxf "/$PHP_FILENAME" -C /usr/src
real    0m 3.10s
user    0m 1.53s
sys 0m 2.48s
time rm -rf /usr/src/php
real    0m 0.52s
user    0m 0.04s
sys 0m 0.48s

I think we can go with this PR as is, I let you review prior to run update.sh & merge it.

@jeremyFreeAgent
Copy link

👍

@shouze
Copy link
Contributor Author

shouze commented Jul 7, 2016

ping @yosifkit @tianon

@yosifkit yosifkit mentioned this pull request Jul 8, 2016
@tianon tianon closed this in #256 Jul 13, 2016
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