Skip to content

Conversation

@henderkes
Copy link
Collaborator

@henderkes henderkes commented Nov 18, 2025

What does this PR do?

Checklist before merging

If your PR involves the changes mentioned below and completed the action, please tick the corresponding option.
If a modification is not involved, please skip it directly.

  • If you modified *.php or *.json, run them locally to ensure your changes are valid:
    • composer cs-fix
    • composer analyse
    • composer test
    • bin/spc dev:sort-config
  • If it's an extension or dependency update, please ensure the following:
    • Add your test combination to src/globals/test-extensions.php.
    • If adding new or fixing bugs, add commit message containing extension test or test extensions to trigger full test suite.

@henderkes
Copy link
Collaborator Author

okay, musl doesn't have python?

other one just needs LD_LIBRARY_PATH.

@crazywhalecc
Copy link
Owner

crazywhalecc commented Nov 19, 2025

I remember that the reason I didn't add these dependencies to curl in the beginning was because it had a library dependency on Python. Dependent on a language with various system-level dependency issues can be quite a headache.

@henderkes
Copy link
Collaborator Author

Yeah I think I'll remove psl. Looks like python is no longer default in Ubuntu either.

Comment on lines +14 to +15
$this->source_dir .= '/src';
shell()->cd($this->source_dir)->exec('autoreconf -if');
Copy link
Owner

Choose a reason for hiding this comment

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

If there is no need to modify the root directory, it is best to pass the subpath directly instead of modifying it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How do we pass a subpath directly? I tried searching for it, but couldn't find it.

Copy link
Owner

@crazywhalecc crazywhalecc Nov 20, 2025

Choose a reason for hiding this comment

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

I mean shell()->cd("{$this->source_dir}/src")->exec() instead of modifying it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not working for UnixAutoconfExecutor, that's why I changed it.

Copy link
Owner

@crazywhalecc crazywhalecc Nov 20, 2025

Choose a reason for hiding this comment

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

Ah that makes sense. But I think it would be more appropriate to add an arg for executor. I would make changes in 3.0 rather than this PR. I will test later to see if it affects other functions, or we can restore it at the end.

@henderkes henderkes merged commit 28ae424 into main Nov 21, 2025
12 checks passed
@henderkes henderkes deleted the feat/libcurl-extra branch November 21, 2025 14:05
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.

4 participants