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

Enable Swoole curl-native on Alpine #1046

Merged
merged 3 commits into from
Feb 6, 2025

Conversation

MichaelGooden
Copy link
Contributor

At minimum Swoole 6 on Alpine doesn't experience the segmentation fault with the native curl implementation, under default installation criteria.

This is the initial attempt at a change, we will want to ensure the test cases for Swoole curl cover this change sufficiently.

@mlocati
Copy link
Owner

mlocati commented Jan 22, 2025

@MichaelGooden is this PR ready for review?

@MichaelGooden
Copy link
Contributor Author

@MichaelGooden is this PR ready for review?

I need your input on the test cases as I am not familiar with GitHub Actions.

My understanding is that it will have run the scripts/tests/swoole test file, however I don't see any test output in the tests so I can't confirm that is being checked.

@mlocati
Copy link
Owner

mlocati commented Jan 22, 2025

When the tests compile a PHP extension, they also run the file with the same name as the extension in the scripts/tests directory (if it exists).
Since we have this file which should test that swoole doesn't break PHP, it should be enough to add a commit (even message-only commit) that tests installing curl and swoole toghether, that is, a commit whose message contains this line:

Test: curl+swoole

@mlocati mlocati force-pushed the alpine-swoole-native-curl branch from 748a727 to 086d968 Compare February 3, 2025 08:24
@MichaelGooden MichaelGooden force-pushed the alpine-swoole-native-curl branch from 81aea04 to f338e57 Compare February 5, 2025 11:14
@MichaelGooden
Copy link
Contributor Author

I have tested locally that forcing Swoole to use Curl on Debian + PHP 8 fails the test with a segfault.

Based on that I believe the current tests are sufficient for this change in behavior.

@MichaelGooden MichaelGooden marked this pull request as ready for review February 5, 2025 11:41
@MichaelGooden
Copy link
Contributor Author

@MichaelGooden is this PR ready for review?

This is now ready for review.

@mlocati
Copy link
Owner

mlocati commented Feb 6, 2025

Thanks!

@mlocati mlocati merged commit 51f905d into mlocati:master Feb 6, 2025
40 checks passed
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