Skip to content

Feat: UnixCMakeExecutor #750

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

Merged
merged 14 commits into from
Jun 9, 2025
Merged

Feat: UnixCMakeExecutor #750

merged 14 commits into from
Jun 9, 2025

Conversation

crazywhalecc
Copy link
Owner

@crazywhalecc crazywhalecc commented Jun 8, 2025

What does this PR do?

Replace all cmake builder to cmake executor. After refactoring we can control all cmake build process instead of updating every single file every time. This is one of prerequisites for #726 .

After this PR is merged, it will continue to:

  • UnixAutoconfExecutor.
  • New setDefaultEnv() for UnixShell to replace the redundant setEnv(['XXXFLAGS']), remove redundant execWithEnv().
  • WindowsCMakeExecutor.

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, run composer cs-fix at local machine.
  • Test all cmake libraries.

@crazywhalecc crazywhalecc requested a review from henderkes June 8, 2025 17:10
@crazywhalecc crazywhalecc added the kind/framework Issues related to CLI app framework label Jun 8, 2025
@henderkes
Copy link
Collaborator

Could you also change execWithEnv to put the environment silently in prod mode and only print the executed command without environment flags? For debug mode I think we should print all environment flags we use, so pkg_config and pkg_config_path as well.

@crazywhalecc
Copy link
Owner Author

Could you also change execWithEnv to put the environment silently in prod mode and only print the executed command without environment flags? For debug mode I think we should print all environment flags we use, so pkg_config and pkg_config_path as well.

I will do it next PR.

@henderkes
Copy link
Collaborator

Could you base the next PR off of my PR by chance? This one will have quite a few conflicts, but the next will have even more.

@crazywhalecc
Copy link
Owner Author

Could you base the next PR off of my PR by chance? This one will have quite a few conflicts, but the next will have even more.

I plan to start resolving #726 conflicts after the next setEnv PR. This will make it clearer how we should modify the giant PR.

I've already done part of it, and it seems like basing the next PR on it will lead to even more confusion. Maybe breaking up parts of #726 into smaller PRs and merging them into the main branch first would be clearer.

@crazywhalecc crazywhalecc requested a review from henderkes June 9, 2025 02:24
@henderkes
Copy link
Collaborator

Yeah that's fair.

@crazywhalecc crazywhalecc merged commit 3bb9a7b into main Jun 9, 2025
9 checks passed
@crazywhalecc crazywhalecc deleted the feat/wrap-builder branch June 9, 2025 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/framework Issues related to CLI app framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants