-
-
Notifications
You must be signed in to change notification settings - Fork 339
V3 feat/re2c #992
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
V3 feat/re2c #992
Conversation
|
@copilot Is there any changes I'm doing wrong? Just be smart and cautious, okay? |
|
@crazywhalecc I've opened a new pull request, #993, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces significant architectural improvements to the static PHP build system, focusing on refactoring the registry and package loading system, adding shared extension build support, and implementing new library packages.
Key Changes
- Registry System Refactoring: Moved loader classes (PackageLoader, DoctorLoader, ArtifactLoader) to a dedicated
StaticPHP\Registrynamespace and introducedRegistryExceptionfor better error handling - Shared Extension Build Support: Added comprehensive support for building PHP extensions as shared libraries with new stages, environment configuration, and automatic stage registration
- Package Inheritance Pattern: Enabled package implementation classes to extend base package types (LibraryPackage, TargetPackage) for direct access to utility methods
Reviewed changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/globals/functions.php | Updated strip_ansi_colors to accept Stringable parameter |
| src/StaticPHP/Util/InteractiveTerm.php | Added no-ansi option support for terminal output formatting |
| src/StaticPHP/Registry/Registry.php | Changed exception types to RegistryException and added registry validation |
| src/StaticPHP/Registry/PackageLoader.php | Moved to Registry namespace, refactored stage handling, added validation |
| src/StaticPHP/Registry/DoctorLoader.php | Moved to Registry namespace |
| src/StaticPHP/Registry/ArtifactLoader.php | Moved to Registry namespace |
| src/StaticPHP/Package/PhpExtensionPackage.php | Added shared extension build support with default stages |
| src/StaticPHP/Package/PackageInstaller.php | Refactored package installation logic and SAPI package handling |
| src/StaticPHP/Package/Package.php | Moved build functions from LibraryPackage, added outputs tracking |
| src/StaticPHP/Package/LibraryPackage.php | Added patchPkgconfPrefix method for pkg-config file patching |
| src/StaticPHP/Exception/RegistryException.php | New exception class for registry-related errors |
| src/StaticPHP/Exception/ExceptionHandler.php | Added RegistryException handling |
| src/StaticPHP/DI/CallbackInvoker.php | Added context hierarchy expansion for object inheritance |
| src/StaticPHP/DI/ApplicationContext.php | Added PatchDescription attribute logging |
| src/StaticPHP/ConsoleApplication.php | Added registry validation check on initialization |
| src/StaticPHP/Attribute/Package/Stage.php | Changed parameter from 'name' to nullable 'function' |
| src/StaticPHP/Attribute/Package/BeforeStage.php | Added support for callable array stage references |
| src/StaticPHP/Attribute/Package/AfterStage.php | Added support for callable array stage references |
| src/Package/Target/php.php | Updated to use callable stage references, added shared extension building, extended TargetPackage |
| src/Package/Target/micro.php | New micro SAPI target implementation with embed stage patching |
| src/Package/Library/ncurses.php | New ncurses library build implementation |
| src/Package/Library/libedit.php | New libedit library build implementation extending LibraryPackage |
| config/pkg.lib.json | Added static-libs configuration for ncurses |
| composer.json | Added nette/php-generator dependency |
Comments suppressed due to low confidence (1)
src/StaticPHP/Registry/PackageLoader.php:295
- [nitpick] Inconsistent variable naming in the loop. The iteration variable
$before_eventsis misleading since it's used in both BeforeStage and AfterStage event processing (line 295). Consider using a more generic name like$eventsor$stage_eventsfor clarity.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } elseif ($package instanceof LibraryPackage && $package->getArtifact()->shouldUseBinary()) { | ||
| // install binary | ||
| } elseif ($is_to_build && $has_build_stage || $has_source && $has_build_stage) { |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic condition appears incorrect. The condition should build packages that either:
- Are marked to build (
$is_to_build) AND have a build stage, OR - Have source AND have a build stage (regardless of binary availability)
However, the current condition $is_to_build && $has_build_stage || $has_source && $has_build_stage lacks parentheses and may not express the intended logic correctly due to operator precedence. Without parentheses, this evaluates as ($is_to_build && $has_build_stage) || ($has_source && $has_build_stage).
Consider adding explicit parentheses or revising the logic to make the intent clear.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
What does this PR do?
libeditpackage buildncursespackage buildncursesstatic-libs configphp-micropatch for embed modelibxxx extends LibraryPackage)Checklist before merging
*.phpor*.json, run them locally to ensure your changes are valid:composer cs-fixcomposer analysecomposer testbin/spc dev:sort-configsrc/globals/test-extensions.php.extension testortest extensionsto trigger full test suite.