V3c/refactor pgo br - #1228
Conversation
…rectory conflicts
…te cached entries based on request options
crazywhalecc
added
the
need-test
|
The current PR primarily involves splitting the feat/pgo-v3 branch. The following improvements have been completed:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors parts of the v3 artifact download/extract pipeline and Unix CMake toolchain generation to improve cache correctness (when request options/config change) and fix Linux link ordering issues that can break CMake try_compile probes.
Changes:
- Introduces
CacheMatchInterfaceand wires it intoArtifactDownloader::generateQueue()so download types can invalidate stale cache entries based on current request options/config (e.g.,php-release,git,url). - Adjusts Linux CMake runtime library handling by moving runtime libs into the toolchain as standard libraries appended after objects/archives, avoiding
--as-neededordering pitfalls. - Improves artifact source handling: local-source extraction short-circuit, safer cleanup of legacy symlink source dirs, and switches pkg-config CFLAGS collection to full
--cflags.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/StaticPHP/Util/PkgConfigUtil.php | Switches pkg-config CFLAGS retrieval to full --cflags to retain required -I include paths. |
| src/StaticPHP/Runtime/Executor/UnixCMakeExecutor.php | Moves Linux runtime libs to toolchain standard libraries to fix link ordering (esp. try_compile). |
| src/StaticPHP/Artifact/DownloaderOptions.php | Changes --with-php default behavior to better separate cache-matching from fetch-time defaulting. |
| src/StaticPHP/Artifact/Downloader/Type/Url.php | Adds cache match logic to invalidate cache when URL changes. |
| src/StaticPHP/Artifact/Downloader/Type/PhpRelease.php | Adds cache match logic tied to --with-php semantics (including git). |
| src/StaticPHP/Artifact/Downloader/Type/GitHubRelease.php | Improves stable-release selection by consulting /releases/latest when requested. |
| src/StaticPHP/Artifact/Downloader/Type/Git.php | Adds cache match logic so config changes (url/rev/regex) invalidate cached clones. |
| src/StaticPHP/Artifact/Downloader/Type/CacheMatchInterface.php | New interface for download types to validate cached entries against current request context. |
| src/StaticPHP/Artifact/ArtifactExtractor.php | Skips extraction for local sources and safely handles legacy symlink source dirs. |
| src/StaticPHP/Artifact/ArtifactDownloader.php | Uses cache-match hook in queue generation; improves custom downloader display labels; adjusts logging behavior. |
| src/StaticPHP/Artifact/Artifact.php | Adds origin labels for custom download callbacks; makes source dir resolve to local cached dirname when applicable. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The pending-artifacts log filter computes each artifact's queue, and downloadWithType() computes it again, doubling the sha1_file/git rev-parse hash checks. Queues only depend on each artifact's own cache files, so memoize them per download() run.
|
StaticPHP Test Bot Detected: Extensions: none | Libraries: none | Targets: none |
|
Oh wait, I haven't finished it yet. |
# Conflicts: # README-zh.md # src/StaticPHP/Artifact/ArtifactExtractor.php # src/StaticPHP/Artifact/Downloader/Type/PhpRelease.php
# Conflicts: # src/StaticPHP/Artifact/ArtifactExtractor.php
|
Is this ready now? |
|
Yeah. Main part (migration from pgo-v3 branch):
|
There was a problem hiding this comment.
🟡 Changes recommended
Cache matching has stale-binary, mirror, filename, and Git-submodule defects, while the README uses incorrect Windows workflow input names.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 6
- Review effort level: Balanced
| if ($source_downloaded && $artifact->getCustomSourceCallback() === null) { | ||
| $source_config = $artifact->getDownloadConfig('source'); | ||
| $dl_cls = is_array($source_config) ? ($this->downloaders[$source_config['type']] ?? null) : null; | ||
| if ($dl_cls !== null && is_a($dl_cls, CacheMatchInterface::class, true)) { | ||
| $source_lock = ApplicationContext::get(ArtifactCache::class)->getSourceInfo($artifact->getName()) ?? []; | ||
| if (!(new $dl_cls())->cacheMatches($artifact->getName(), $source_config, $source_lock, $this)) { |
| // Some download types fetch content depending on request options rather than config alone | ||
| // (e.g. php-release varies with --with-php): let them veto a stale cache entry. | ||
| // Custom source callbacks carry their own semantics, they bypass type-based checks. | ||
| if ($source_downloaded && $artifact->getCustomSourceCallback() === null) { |
| { | ||
| // A changed filename already invalidates via the file-exists check; a changed url | ||
| // with an unchanged filename (mirror switch, fixed-name tarball) does not. | ||
| return ($lock_entry['config']['url'] ?? null) === ($config['url'] ?? null); |
| // The lock hash (rev-parse HEAD) only proves the cached clone is self-consistent; | ||
| // a changed url/rev/regex in the config must invalidate it. | ||
| $locked = $lock_entry['config'] ?? []; | ||
| foreach (['url', 'rev', 'regex'] as $key) { |
| 'CPPFLAGS' => "-I{$package->getIncludeDir()}", | ||
| 'LDFLAGS' => "-L{$package->getLibDir()} " . getenv('SPC_CMD_VAR_PHP_MAKE_EXTRA_LDFLAGS'), | ||
| 'LIBS' => SystemTarget::getRuntimeLibs(), | ||
| 'LIBS' => $vars['EXTRA_LIBS'] ?? '', |
There was a problem hiding this comment.
Trimming configure LIBS to runtime libs breaks every link-based extension check (ldap, curl, event, imap, snappy, ...) since static archives need their transitive deps on the link line. The strlcpy/strlcat false positives from polyfill archives (libpgport/libedit) are instead fixed for all Linux toolchains via ac_cv_func_strlcpy=no ac_cv_func_strlcat=no in SPC_EXTRA_PHP_VARS — the same mechanism ZigToolchain already used.
There was a problem hiding this comment.
It's not that simple, this will also break other symbol checks of other extensions. We really can't pass all libs here and we never had to in main/v2. The root cause must be found for what's failing here and why, because passing all libs is not the answer.
There was a problem hiding this comment.
While v2 does not pass LIBS, it still injects all libraries into the configure script via a sed patch.
The question is only where to inject it. v3 chose global LIBS in 2676875 and the full ext matrix has been green for a long time. The strlcpy/strlcat false positives are real, but ac_cv_func_strlcpy=no ac_cv_func_strlcat=no are configure-global cache vars — they cover php core and every extension's checks in one shot, now for all Linux toolchains (moved from ZigToolchain to ToolchainManager).
There was a problem hiding this comment.
A bigger problem lies in the legacy issues in the config.m4 files of most PHP extensions, such as:
PHP_EVAL_LIBLINEput the result in LDFLAGS (before the target file, where static links are invalid) andldapdirectly skip thepkg-configbranch.- The PHP core's own OpenSSL extension requires explicit passing of
OPENSSL_LIBS="-lz"via SPC (visible in the CI configure command) to pass the check. - macOS frameworks are clearly ignored.
Therefore, changing to getRuntimeLibs() only depends on the choice: patching a large number of extension config files (including adapting to multiple different versions), or currently only patching strlcpy.
There was a problem hiding this comment.
Hmm, can you please try the craft.yml from static-php/packages repo? I'm not on my pc today.
What does this PR do?
Checklist before merging
*.phpor*.yml, run them locally to ensure your changes are valid:composer cs-fixcomposer analysecomposer testbin/spc dev:lint-config