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: respect PHP_MEMORY_LIMIT for composer #246

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

PixelJonas
Copy link

@PixelJonas PixelJonas commented May 26, 2019

After setting PHP_MEMORY_LIMIT to -1 composer still exited with an OOM error.
Setting the parameter directly through php fixed the issue

After setting PHP_MEMORY_LIMIT to `-1` composer still exited with an OOM error.
Setting the parameter directly through php fixed the issue
@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

5 similar comments
@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@rhscl-bot
Copy link

Can one of the admins verify this patch?

@rhscl-bot
Copy link

Can one of the admins verify this patch?

@rhscl-bot
Copy link

Can one of the admins verify this patch?

@PixelJonas
Copy link
Author

any chance this is getting merged? I'm currently monkey-patching the assemble file :(

@rhscl-bot
Copy link

Can one of the admins verify this patch?

@PixelJonas
Copy link
Author

This is still an open issue for me? can anyone verify this patch?

@PixelJonas
Copy link
Author

any chance this will ever get merged?

@phracek
Copy link
Member

phracek commented Jan 13, 2022

[test-all]

@phracek
Copy link
Member

phracek commented Jan 13, 2022

@PixelJonas Thanks for the PR!. Any documentation about this change? I am not familiar with php ;)

@phracek
Copy link
Member

phracek commented Jan 13, 2022

@PixelJonas Can you please include it into the other versions, 7.4?

@phracek phracek self-requested a review January 13, 2022 09:15
@PixelJonas
Copy link
Author

I added the snippet to 7.3 and 7.4 as well. TBH my PHP knowledge is not that extensive and this PR was from a small engagement where needed to install a contao-cms based system in OpenShift V3. Digging through the code PHP_MEMORY_LIMIT seems to be the community default to set memory limit for PHP.

This is not respected when directly using the composer.phar as I was getting OOM errors when installing contao. I monkey-patched this and it worked.

As I don't find any documentation about PHP_MEMORY_LIMIT inside this repo, where would be a good place to add this to an existing doc?

Lastly, looking at the test-bed I do see errors all over the place. Is the test-bed in a "known good state", so these errors are coming from my change?

@phracek
Copy link
Member

phracek commented May 31, 2022

[test-all]

@phracek
Copy link
Member

phracek commented Jun 6, 2022

@remicollet Does it make sense for you this PR?

@PixelJonas Can you please add this change also for (https://github.com/sclorg/s2i-php-container/blob/master/8.0/s2i/bin/assemble)

[test-all]

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

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

Please add this also into 8.0 version.

@PixelJonas
Copy link
Author

I added the patch to 8.0

@remicollet
Copy link
Contributor

I can understand the need to set memory limit for cli command (e.g. composer)
but I don't think it make sense to use the same value for web process (usually quite low) and for cli process (usually quite high)

It could be better to use 2 configuration options

  • PHP_MEMORY_LIMIT for web process, set using php_value in httpd.conf (mod_php) or www.conf (php-fpm)
  • PHP_MEMORY_LIMIT_CLI set for other process in php.ini

BTW, for now PHP_MEMORY_LIMIT is set is php.ini, so should be used by composer, which mean php.ini is NOT properly populated (after composer step ?)

@phracek
Copy link
Member

phracek commented Jul 28, 2022

@PixelJonas Can you please fix it as @remicollet propose two solutions?

@PixelJonas
Copy link
Author

That heavily depends on the timeframe you are expecting this to be done. Right now I don't see the bandwidth to do that, as this looks like something which needs to be tested and I don't have a scenario this is actually being used in (this PR is from 2019).

Copy link

Pull Request validation

Failed

🔴 Approval - missing or changes were requested

Success

🟢 CI - All checks have passed
🟢 Review - Reviewed by a member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants