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

Streamline the build of the image #163

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

andrewnicols
Copy link
Contributor

@andrewnicols andrewnicols commented Mar 15, 2023

This change makes it much faster to make smaller changes to the image by:

  • moving the ADD commands to more targeted additions in order of build size
  • breaking out the Oracle and sqlsrv builds into their own ADD and RUN sections in the Dockerfile
  • moving the addition of non build-related files to right at the end of the Dockerfile

These changes mean that it is possible to more easily develop the image, for example:

  • you can now make changes to files within the /usr directory without recompiling all PHP extensions
  • you can now iterate on the Oracle and/or sqlsrv extensions without recompiling all PHP extensinos
  • you can now iterate on the sqlsrv extension, without recompiling all PHP extensions, or Oracle

Whist this has little effect on the end image, or the build process within CI systems, local development is substantially improved (unless you're making changes to the php-extensions.sh script).

@andrewnicols
Copy link
Contributor Author

Grr. sqlsrv build is failing because of a bug in msphpsql:
microsoft/msphpsql#1438

Have to pin some packages to old versions.

@andrewnicols andrewnicols force-pushed the streamline_build branch 3 times, most recently from af9c56e to 6553f25 Compare March 15, 2023 04:14
@andrewnicols andrewnicols mentioned this pull request Mar 15, 2023
@andrewnicols andrewnicols force-pushed the streamline_build branch 2 times, most recently from 60702f7 to 6f1d842 Compare March 15, 2023 04:28
@andrewnicols
Copy link
Contributor Author

Build failure resolved. There seems to be a version of unixodbc in the microsoft repo which is broken and leads to incorrect builds.

We have to keep the install of unixodbc to a point before we add the microsoft apt repo.

@stronk7
Copy link
Member

stronk7 commented Mar 15, 2023

Hi Andrew,

it looks perfect! Only 1 little thing and 1 question:

  1. little thing: Now that we have completely moved the sqlsrv handling (both the libraries and the extension) to sqlsrv-extension.sh, it looks like we can remove all (two) remaining occurrences of unixodbc from php-extensions.sh.
  2. What's better... to make each RUN command to perform the cleanup:
# Keep our image size down..
pecl clear-cache
apt-get remove --purge -y $BUILD_PACKAGES
apt-get autoremove -y
apt-get clean
rm -rf /var/lib/apt/lists/*
// Other things may come here...

Ot to add one more RUN, at the end, to perform it only once?

Ciao :-)

@andrewnicols
Copy link
Contributor Author

  1. little thing: Now that we have completely moved the sqlsrv handling (both the libraries and the extension) to sqlsrv-extension.sh, it looks like we can remove all (two) remaining occurrences of unixodbc from php-extensions.sh.

Thanks - I wasn't sure if unixodbc was used by any other DBs for compilation as it's not a package I'm very familiar with. Moved to the sqlsrv step only.

  1. What's better... to make each RUN command to perform the cleanup:
    Or. to add one more RUN, at the end, to perform it only once?

It's much better to have each RUN command perform a complete setup and teardown itself.

The rationale is:

  • each step of a Dockerfile (RUN, ARG, ADD, etc.) generates a Docker 'layer'.
  • you want to keep each layer as small as possible because those are what are downloaded when you do a docker pull.
  • if you have a setup phase, then some other phases, and finally a teardown phase, you're essentially creating a really large layer, which must be downloaded, only for a subsequent layer to remove those files
  • you do lots of docker pulls, but comparatively few builds
  • therefore it is better to spend an extra 20 seconds at build time to make each layer small, rather than save time during the build and ending up with bigger layers which waste bandwidth

In this case I'd say this applies too, even though we have some shared setup in each step. Whatever we do, local development will be significantly faster than it previously was.

For reference, whilst trying to iterate on the new entrypoint issue I was spending 6-7 minutes rebuilding the image each time I tweaked the entrypoint shell script. With these commit in place, it takes about 5-10 seconds to do exactly the same change.

This change makes it much faster to make smaller changes to the image
by:
- moving the ADD commands to more targeted additions in order of build
  size
- breaking out the Oracle and sqlsrv builds into their own ADD and RUN
  sections in the Dockerfile
- moving the addition of non build-related files to right at the end of
  the Dockerfile

These changes mean that it is possible to more easily developer the
image, for example:
- you can now make changes to files within the /usr directory without
  recompiling all PHP extensions
- you can now iterate on the Oracle and/or sqlsrv extensions without
  recompiling all PHP extensinos
- you can now iterate on the sqlsrv extension, without recompiling all
  PHP extensions

Whist this has little effect on the end image, or the build process
within CI systems, local development is substantially improved (unless
you're making changes to the php-extensions.sh script).
Copy link
Member

@stronk7 stronk7 left a comment

Choose a reason for hiding this comment

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

Sold, thanks!

(I'll try to backport this to all the >= 8.0 7.4 images after master's rebuild ends)

Edited: s/8.0/7.4, so we can backport other PRs, based on this one also to 7.4.

@stronk7 stronk7 merged commit 25904b2 into moodlehq:master Mar 16, 2023
@stronk7
Copy link
Member

stronk7 commented Mar 16, 2023

Done, this has been backported to all >= 7.4 versions. They are still being built, but all ok, so far.

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.

2 participants