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

🚑 Fix some BC break in ext install. #269

Closed
wants to merge 1 commit into from
Closed

🚑 Fix some BC break in ext install. #269

wants to merge 1 commit into from

Conversation

shouze
Copy link
Contributor

@shouze shouze commented Jul 21, 2016

Fixes #266. Related to #256 changes.

usage >&2
exit 1
if ! grep -qE "^$ext$" /usr/src/php-available-exts ; then
if [ ! -d "/usr/src/php/ext/$ext" ] ; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nested if as its not that easy to make some XOR in shell scripts.

@ktogo
Copy link

ktogo commented Jul 21, 2016

Looks good to me thanks @shouze !

# were extracting some of their custom extensions in this directory
# See https://github.com/docker-library/php/issues/266
# Was breaked by https://github.com/docker-library/php/pull/256
mkdir -p /usr/src/php/ext
Copy link
Member

Choose a reason for hiding this comment

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

This will break our detection of the php source being already extracted on line 22

Also, this is somehow spaces instead of tabs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, will fix that

Copy link
Member

Choose a reason for hiding this comment

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

❤️ Thanks

@shouze
Copy link
Contributor Author

shouze commented Jul 23, 2016

ping @yosifkit ok, amended fixes in the commit.

@@ -19,7 +19,7 @@ case "$1" in
echo >&2 "$dir exists and is not a directory"
exit 1
fi
if [ ! -d "$dir" ]; then
if [[ ! -d "$dir" || $(ls -1 "$dir") == "ext" ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just don't check that ext is a directory here... but I think we shouldn't care because people in this case would intentionally make their builds fail I guess ^^

@yosifkit
Copy link
Member

I don't feel like this is a complete solution. Seeing that many users were using ext-install in an unexpected way makes me realize that this current fix will only break a different set of users. We need to only delete folders from /usr/src/php/ext/ that are part of the tar file. They could be doing something like the following:

RUN git clone https://github.com/php-memcached-dev/php-memcached /usr/src/php/ext/memcached \
  && cd /usr/src/php/ext/memcached && git checkout -b php7 origin/php7 \
  && docker-php-ext-install zip \
# at which point their memcached source is now gone
  && docker-php-ext-configure memcached \
  && docker-php-ext-install memcached

Or we could do as @tianon suggested and revert the change on the regular Debian images. I am leaning toward this idea right now

@shouze
Copy link
Contributor Author

shouze commented Jul 28, 2016

@yosifkit yes good catch indeed. Anyway we should return a clear error message as suggested in #268. Then taking the decision to revert or no for debian.

@ktogo
Copy link

ktogo commented Aug 4, 2016

@yosifkit ping #269 (comment)

@ktogo
Copy link

ktogo commented Aug 9, 2016

@yosifkit @shouze Do you have any update on this?

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.

3 participants