Avoiding breaking changes such as PR #256

Question

Avoiding breaking changes such as PR #256

Closed this issue 9 years ago · 23 comments

Hi,

PR #256 changed the way how it finds the installable packages, and then now I am no longer able to install third party packages such as PHPRedis using only docker-php-ext-install and have to rewrite all the existing Dockerfiles.

I've found some solution at #262 (comment) which wraps the extension installation process with docker-php-source extract and docker-php-source delete, then adds the extension name to /usr/src/php-available-exts to "whitelist" the additional extension name. Obviously this solution "works as intended", however, as you can see it requires all the developers to rewrite their customized php Dockerfiles.

The question is, why was this big breaking change applied to all the existing php Dockerfiles?
Was there no option to create "a brand new version with breaking changes" and leave the existing Dockerfiles untouched?
I have been using this php docker container for months on some enterprise-class solution, so I would like to avoid this kind of breaking changes to be happening as much as possible.

There is still last-resort solution which "copies the Dockerfile itself and customize the copied one," however I think it's not how this kind of publicly-used Dockerfile should be :(

I would very much appreciated if you could consider not to make such a breaking change to all the existing Dockerfiles within the same version. Thanks!

Answer 1 · 2016-07-20T12:37:58.000Z

I agree. I had to rewrite my custom Dockerfiles to be able to build the images again.
Moreover, the error message displayed here is misleading. It should be something likepackage $ext not listed in /usr/src/php-available-exts.
I had to look at the source code to figure out what the problem really was.

Answer 2 · 2016-07-20T18:00:08.000Z

Yeah, fair points.

@yosifkit @shouze what do you think about reverting #256 to only apply to Alpine-based variants (where we're a lot more proactive/aggressive about getting and staying slim on-disk)?

Answer 3 · 2016-07-20T18:39:14.000Z

@tianon yes maybe it was a bad idea to make it happen on debian images... but I would ensure few things before. @ktogo would you please provide your custom dockerfile before and after you applied the fix? I think you were building your custom docker images in a way I didn't realize it was possible.

Answer 4 · 2016-07-20T18:45:36.000Z

@taueres in fact the message is not misleading... docker-ext-install script only install extensions bundled into php source code at the origin, for all other extentions installs you should use pecl (or pickle) or even custom/manual download & install.
For example with redis extension as you have to provide a different version requirement I'm curious, how can you install a specific version of the ext with the docker-ext-install script?

Answer 5 · 2016-07-21T03:10:41.000Z

@shouze Sorry, the Dockerfile I had to rewrite was a part of proprietary system so I can't paste it here unfortunately.
But the code I added is pretty much same to the one that you described at #262 (comment), with some addition (sed command) described at #262 (comment)

In regards to the error message, even it is not actually misleading, it still confuses users who don't know the command was only made for official extension installation. Since the error message does not give any idea to users that they are attempting to install the third-party extension in a wrong way, they would need to spend a time to figure out why those error messages suddenly came up. I spent not only minutes to find the reason why it suddenly broke.
To help their troubleshooting, How about adding the message for example say "docker-ext-install no longer supports installing third-party extensions"? This does not solve the breaking change problem but at least it helps users to recover quickly.

Answer 6 · 2016-07-21T10:11:22.000Z

@ktogo @taueres as docker-php-ext-install never permits to install pecl extensions I still don't understand how #256 has introduced some BC break.

If you can help me to figure out what was working before the PR with a Dockerfile example it would be really handy as I never wanted to introduce some BC break.

@ktogo yes a solution could be to return a more explicit message of course. This could be done in a first PR I guess... but I wonder if in fact we couldn't do something more useful! 😄

In fact if you try to install redis extension for example, which should be installed with pecl, maybe that docker-php-ext install should:

Check if redis if one of php source code officially bundled extensions
If not, then run pecl remote-info redis
If return code of pecl remote-info redis equals zero, then display some very nice message like this one:

ERROR: docker-php-ext-install only permit to install php source code bundled extensions.
You should install 'redis' extension with pecl:
Package details:
================
Latest      3.0.0
Installed   - no -
Package     redis
License     PHP
Category    Database
Summary     PHP extension for interfacing with Redis
Description This extension provides an API for communicating
            with Redis servers.

If everything fail, we could return some error message like this one:

ERROR: sorry but 'asdq' extension is either not a pecl extension 
nor an officially bundled extension from php source code.

@tianon @yosifkit does it looks good for you too?

Answer 7 · 2016-07-21T11:26:39.000Z

@shouze Thanks.

Regarding how it lost BC, The original installation codes which was working and has lost the compatibility is something like this: (Not exactly same, but almost same to the one that I was using)

RUN curl -L -o /tmp/redis.tar.gz https://github.com/phpredis/phpredis/archive/3.0.0.tar.gz \
    && tar xfz /tmp/redis.tar.gz \
    && rm -r /tmp/redis.tar.gz \
    && mv phpredis-3.0.0 /usr/src/php/ext/redis \
    && docker-php-ext-install redis

This code tries to install PHPRedis from official source code with the help of docker-php-ext-install which was working completely fine before #256, because original implementation of docker-php-ext-install was every time scanning /usr/src/php/ext directory and was able to locate the additional package which manually deployed into that directory at any time.
However the scanning process is now run only once after the first call of docker-php-source extract, therefore this change caused the additional packages to be "unfindable" by docker-php-ext-install. This is how this compatibility break happened.

This "misusage" is I believe widely used since it's been shared on bunch of sites for example some top answer on Stackoverflow which comes at first result of Google search "docker php install redis".
So I guess there should definitely be not only few people got affected.

Answer 8 · 2016-07-21T11:39:27.000Z

@ktogo ok I see... it's effectively a not so good idea to pollute php source code with some externally fetched extension but it's permitted so ok it breaks indeed.

BTW as redis for php7 is officially out, the quickest way to install redis 3.0.0 for php7 in your Dockerfile is:

RUN pecl install redis-3.0.0

So I can make a PR either:

Introducing a better descriptive message about the breaking failure to help people fix their Dockerfile quickly as suggested in #266 (comment)
Reverting #256 for debian images only (and keeping it for alpine)
Reverting #256 for debian images only (and keeping it for alpine, plus adding the better failure error message).

Please vote for 1, 2 or 3 and let's go for it!

Answer 9 · 2016-07-21T11:43:41.000Z

@ktogo from the example you give, can you confirm that the build of your docker container was failing at this step?

mv phpredis-3.0.0 /usr/src/php/ext/redis

Because php source code is no more present in the container unless you call docker-php-source extract

Answer 10 · 2016-07-21T11:58:29.000Z

@shouze Both of move and docker-php-ext-install have failed.
Even I added either docker-php-source extract or mkdir -p /usr/src/php/ext, it still failed at following line:

docker-php-ext-install redis

Regarding the revert solution, we need to consider that there might be some users who have already fixed their Dockerfiles by adding docker-php-source. The revert would let them face breaking change again, since docker-php-source command is introduced at PR #256 and it will disappear after revert.

To avoid this and recover the backward compatibility, I have another proposal:

Update docker-php-source delete to remove everything inside of /usr/src/php/ext, except the directly itself so that some codes such as mv phpredis-3.0.0 /usr/src/php/ext/redis would not crash.
Then, fix the conditional code here to lookup both /usr/src/php-available-exts and -d '/usr/src/php/ext'.

I haven't tested this, but I guess this would probably solve the BC issue. Needs more thoughts.

Answer 11 · 2016-07-21T12:05:21.000Z

@shouze Since fixing the backward-compatibility may need more discussion, how about firstly improve the error message suggested in #266 (comment) and then think about the BC fix later on?

Answer 12 · 2016-07-21T12:05:59.000Z

@ktogo yes indeed:

docker-php-source delete should in fact run a mkdir -p /usr/src/php/ext after the code deletion.
The condition should effectively try to lookup into /usr/src/php/ext.

This should ensure BC whilst keeping the newly introduced feature that reduce produced container images size.

@ktogo will be in Japan for 2 weeks from Aug. 15th, but of course I expect that everything will be fine before that ;)

Answer 13 · 2016-07-21T12:30:09.000Z

@ktogo ok done for the PR about #266 (comment) please take a look at #268

Answer 14 · 2016-07-21T12:39:49.000Z

@shouze How quick! Thanks 👍

Glad to hear that you are coming to Japan! Yeah lets fix this before it then ;)

Answer 15 · 2016-07-21T13:07:12.000Z

@ktogo and here's the fix of the BC break I guess #269.

Answer 16 · 2016-07-21T13:19:47.000Z

@shouze Just reviewed that! Thanks a lot!

Answer 17 · 2016-07-21T13:21:23.000Z

@shouze I'll need to pull it and test if everything works but I have to go now so will try it tomorrow ;)

Answer 18 · 2016-07-22T22:21:36.000Z

Yikes... I thought I wascrazy for a moment... one minute everything working, the next tens of containers failing... in my case it was ldap... will see what i do abt it... Maybe best is to not depend at all on docker-php-ext-install & similar? we never know what will change again tomorrow....

EDIT: ldap was actually a question of adding a symbolic link, but the changes def broke stuff all around the extensions... I'm aso getting "no such file or directory" errors on the php/ext dir... going after all the changes to see where to put stuff... :/ gonna be a long friday lol

Answer 19 · 2016-07-27T09:09:15.000Z

@shouze Any progress on PR #269 ? Just noticed that CI is failing.

Answer 20 · 2016-07-27T09:40:09.000Z

@ktogo #269 is in review, CI is failing for some other reason as I need to execute update.sh script after the review to make the changes effective for the builded containers ;)

Answer 21 · 2016-07-27T10:17:16.000Z

@shouze I see, thanks ;)

@yosifkit @tianon Could you please take a look into PR #269 as soon as possible? I think this issue has some kinda urgency since it has been continuously making headaches for numbers of users e.g. #271...

Answer 22 · 2016-08-09T12:57:56.000Z

Any news on this front?

Answer 23 · 2016-08-09T22:17:51.000Z

Sorry for the slow response on this breaking change. We wanted to make sure we fix the issue while also keeping the benefits of removing the extracted source. What we have come up with is #288; let us know if this does or doesn't cover all the issues you encountered. Or if it creates unforeseen catastrophes.