-
Notifications
You must be signed in to change notification settings - Fork 38
Feat/constant version #37
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
Conversation
WalkthroughThe updates include enhancements to the build process and workflow configuration, adjustments to LZ4 version constant handling and reporting, addition of new global constants in the PHP stub file, and a macro guard typo fix. No changes were made to public APIs or function signatures; all modifications are internal, build-related, or constant declarations. Changes
Sequence Diagram(s)sequenceDiagram
participant BuildScript
participant vswhere
participant MSVC_Toolset
participant phpsdk-starter.bat
BuildScript->>vswhere: Locate latest MSVC tools directory
vswhere-->>BuildScript: Return MSVC toolsets
BuildScript->>MSVC_Toolset: Enumerate toolsets, select highest matching version
BuildScript->>phpsdk-starter.bat: Invoke with VS version, arch, and selected toolset
phpsdk-starter.bat-->>BuildScript: Build process proceeds
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/build.ps1 (1)
5-17
: Remove commented-out code.The commented-out vswhere installation logic should be removed since it's not being used and clutters the script.
-# Toolset -# $installerDir = Join-Path "${env:ProgramFiles(x86)}\Microsoft Visual Studio" 'Installer' -# $vswherePath = Join-Path $installerDir 'vswhere.exe' -# if (-not (Test-Path $vswherePath)) { -# if (-not (Test-Path $installerDir)) { -# New-Item -Path $installerDir -ItemType Directory -Force | Out-Null -# } -# $vsWhereUrl = 'https://github.com/microsoft/vswhere/releases/latest/download/vswhere.exe' -# Invoke-WebRequest -Uri $vsWhereUrl -OutFile $vswherePath -UseBasicParsing -# } -# if($null -eq (Get-Command vswhere -ErrorAction SilentlyContinue)) { -# throw "vswhere is not available" -# } +# Dynamic toolset selection
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/build.ps1
(1 hunks).github/workflows/windows.yaml
(1 hunks)lz4.c
(2 hunks)lz4.stub.php
(1 hunks)php_verdep.h
(1 hunks)
🔇 Additional comments (7)
php_verdep.h (1)
4-4
: LGTM! Critical typo fix.The macro guard correction from
ZED_FE_END
toZEND_FE_END
fixes a typo that would have caused incorrect conditional compilation behavior..github/workflows/windows.yaml (1)
44-44
: LGTM! Workflow simplification.Standardizing on
windows-latest
is a good approach, especially since the build script now handles dynamic toolset selection. This reduces complexity in the CI configuration.lz4.stub.php (1)
5-28
: LGTM! Well-structured constant declarations.The new LZ4 constants are properly documented with type annotations and @cvalue references. This provides typed PHP-level access to important LZ4 library information.
lz4.c (2)
100-101
: LGTM! Proper constant registration.The new constants provide clean access to LZ4 version information. Using
LZ4_versionNumber()
andLZ4_versionString()
directly from the LZ4 library ensures accurate version reporting.
118-118
: LGTM! Simplified version reporting.Removing the conditional logic and always using
LZ4_versionString()
simplifies the code and ensures consistent version reporting regardless of how LZ4 is linked..github/workflows/build.ps1 (2)
18-41
: LGTM! Robust dynamic toolset selection.The logic for dynamically selecting the appropriate MSVC toolset based on the VS version is well-implemented with proper error handling. The version range logic correctly maps vs16 to 14.20-14.29 and vs17 to 14.30+.
48-48
: LGTM! Improved build invocation.Using
phpsdk-starter.bat
with explicit parameters is more flexible than the previous fixed batch file pattern and works well with the dynamic toolset selection.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes