Skip to content

Conversation

benr77
Copy link
Contributor

@benr77 benr77 commented Jan 26, 2021

Latest version of Rector complains about missing intl extension. I would consider it reasonable to add this to the PHP builds.

Notice: RectorPrefix20210124\Nette\Utils\Strings::toAscii(): it is recommended to enable PHP extensions 'intl'. in /tools/.composer/vendor-bin/rector/vendor/rector/rector-prefixed/vendor/nette/utils/src/Utils/Strings.php on line 124

@jakzal
Copy link
Owner

jakzal commented Jan 26, 2021

Thanks for this. Looks like the resulting image won't increase in size much.

However, the way to make this change as per CONTRIBUTING guidelines is to:

  • Make changes to both Debian and Alpine docker file templates (Dockerfile-debian and Dockerfile-alpine). Once you made your changes, regenerate docker files with make generate.

@jakzal jakzal mentioned this pull request Jan 26, 2021
@jakzal
Copy link
Owner

jakzal commented Jan 26, 2021

Actually, the reason I didn't notice the image size change is because build regenerates Docker files, so your changes were not applied.

I see you updated them already, let's see how it goes now.

@jakzal
Copy link
Owner

jakzal commented Jan 26, 2021

You'll need to add the ICU library toLIB_DEPS in both Docker files.

I think on Debian the package is called libicu-dev while on Alpine it's icu-dev.

@jakzal
Copy link
Owner

jakzal commented Jan 26, 2021

Difference in size is:

@benr77
Copy link
Contributor Author

benr77 commented Jan 26, 2021

Yes, I've just seen the final build sizes. What do you think? Too much overhead?

@jakzal
Copy link
Owner

jakzal commented Jan 27, 2021

@dkarlovi suggested to me it's used often enough to warrant the increase in size.

Let's get it merged. First, though, I need to make sure the build will still pass after the merge. Seems there are some issues with php-cs-fixer on PHP 8 again.

@jakzal jakzal merged commit 8c67bbc into jakzal:master Jan 27, 2021
jakzal added a commit that referenced this pull request Jan 27, 2021
This reverts commit 8c67bbc, reversing
changes made to abbc0ec.
@jakzal
Copy link
Owner

jakzal commented Jan 27, 2021

@benr77 I had to revert the merge as docker hub builds started to fail with:

Debian:

checking for icu-uc >= 50.1 icu-io icu-i18n...
no
�[91mconfigure: error: Package requirements (icu-uc >= 50.1 icu-io icu-i18n) were not met:
No package 'icu-uc' found
No package 'icu-io' found
No package 'icu-i18n' found
Consider adjusting the PKG_CONFIG_PATH environment variable if you
installed software in a non-standard prefix.
Alternatively, you may set the environment variables ICU_CFLAGS
and ICU_LIBS to avoid the need to call pkg-config.
See the pkg-config man page for more details.

and Alpine:

checking for location of ICU headers and libraries... checking for pkg-config... /usr/bin/pkg-config
checking for icu-config...
no
not found
�[91mconfigure: error: Unable to detect ICU prefix or no failed. Please verify ICU install prefix and make sure icu-config works.

Can you reopen the PR please?

@benr77
Copy link
Contributor Author

benr77 commented Jan 27, 2021

What do I do to do that? Push more changes to my branch?

@jakzal
Copy link
Owner

jakzal commented Jan 27, 2021

Try rebasing with latest master maybe.

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