docs(claude): fix stale CI and profiler notes - #4243
morrisonlevi wants to merge 1 commit into
Conversation
- Image tags bookworm-6 -> bookworm-11 and clang versions in the CI docs. - The profiler is built from the root datadog-php crate; there is no profiling/Cargo.toml, and the root rust-toolchain.toml is the pin. - Profiler CI job table/matrix: add UBSAN, drop the stale ASAN row name. - buildPortableLibdatadogPhp uses stable Rust (compile_rust.sh sets RUSTC_BOOTSTRAP=1), not nightly.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 13 Pipeline jobs failed
ℹ️ InfoNo other issues found (see more)❄️ No new flaky tests detected 🎯 Code Coverage (details) Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 8e9b2ff | Docs | View more details | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e9b2ff0cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - The profiler is built from the root `datadog-php` crate (`Cargo.toml`, `--features profiling`); | ||
| there is no `profiling/Cargo.toml`. The `profiler-release` profile is defined there too and | ||
| inherits from `release` with `panic = "abort"`. |
There was a problem hiding this comment.
Complete the root-crate migration in the reproduction commands: the direct Cargo examples still omit --no-default-features, so the root crate's default tracer feature remains enabled and standalone_profiler is not selected. They also try to load/copy libdatadog_php_profiling.so, while the consolidated crate produces libdatadog_php.so (as reflected by config.m4). Consequently, users following the NTS, ZTS, debug, or ASAN examples cannot use the documented output path; update the commands to build profiling,trigger_time_sample without defaults and adjust the artifact paths.
Useful? React with 👍 / 👎.
| dockerh --cache profiler-asan-8.3-nts --php nts-asan \ | ||
| datadog/dd-trace-ci:php-8.3_bookworm-6 --user root --privileged -- bash -c ' | ||
| datadog/dd-trace-ci:php-8.3_bookworm-11 --user root --privileged -- bash -c ' |
There was a problem hiding this comment.
After switching this example to bookworm-11, the command below still exports CC=clang-17 and RUSTC_LINKER=lld-17. The image's Dockerfile installs only clang/lld 21, and the corresponding ASAN workflow also uses version 21, so this local reproduction stops with a missing compiler or linker before building. Update the hard-coded toolchain values to 21 along with the image tag.
Useful? React with 👍 / 👎.
| - `buildPortableLibdatadogPhp` uses the | ||
| `nginx-fpm-php-8.5-release-musl` image with nightly Rust. The image must | ||
| `nginx-fpm-php-8.5-release-musl` image with stable Rust (`compile_rust.sh` | ||
| sets `RUSTC_BOOTSTRAP=1` for `-Zbuild-std`). The image must |
There was a problem hiding this comment.
Correct the image named here: buildPortableLibdatadogPhp sets imageTag: 'php-buildonly-rust' in appsec/tests/integration/build.gradle, while nginx-fpm-php-8.5-release-musl belongs to a separate runtime-image task. As written, someone troubleshooting a missing portable-build image will pull or inspect the wrong image and still be unable to run this Gradle task.
Useful? React with 👍 / 👎.
| | `Profiling correctness / prof-correctness ({ver}, nts)` | `ubuntu-24.04` | Builds profiler + runs NTS correctness test cases | | ||
| | `Profiling correctness / prof-correctness ({ver}, zts)` | `ubuntu-24.04` | Same + `exceptions_zts` (requires `parallel` PECL extension) | | ||
| | `Profiling ASAN Tests / prof-asan ({ver}, {arch})` | `arm-8core-linux` / `ubuntu-8-core-latest` | Builds profiler with ASAN + runs `.phpt` profiling tests | | ||
| | `Profiling ASAN/UBSAN Tests / PHP 8.5 {nts,zts} UBSAN ({arch})` | `arm-8core-linux` / `ubuntu-8-core-latest` | Builds profiler with UBSAN + runs `.phpt` profiling tests | |
There was a problem hiding this comment.
Keep a separate ASAN entry when adding UBSAN. The referenced workflow still defines prof-asan for PHP 8.3–8.5 across nts-asan and debug-zts-asan, but this replacement row documents only the PHP 8.5 UBSAN job, leaving the CI Jobs table incomplete even though the ASAN matrix immediately below remains documented.
Useful? React with 👍 / 👎.
| Toolchain is pinned — see `Cargo.toml` (`rust-version`) and | ||
| `profiling/rust-toolchain.toml`, not a hardcoded version. | ||
| `rust-toolchain.toml`, not a hardcoded version. |
There was a problem hiding this comment.
Do not replace the profiler toolchain pointer with only the root file: profiling/rust-toolchain.toml still exists, and Rustup selects it rather than the root rust-toolchain.toml whenever commands run from profiling/, as the profiler reproduction guide instructs. The two pins happen to match now, but this documentation directs maintainers to the wrong controlling file and will become incorrect as soon as either pin changes.
Useful? React with 👍 / 👎.
| - **Expected test counts (PHP 8.5):** ASAN 47 total, 32 pass, 15 skip, 0 fail; UBSAN nts | ||
| 47 total, 35 pass, 12 skip, 0 fail. The skips are normal | ||
| (platform/env conditions). A non-zero fail count indicates a real problem. |
There was a problem hiding this comment.
Qualify this ASAN count by build type and add the other matrix result. The workflow runs both nts-asan and debug-zts-asan, and their skip totals necessarily differ: zts_01.phpt runs only under ZTS, while exceptions_zts_01.phpt also runs there because the bookworm ZTS image installs ext-parallel; both skip under NTS. A single unqualified 32 pass, 15 skip expectation therefore misclassifies normal output for one of the ASAN variants.
Useful? React with 👍 / 👎.
1 participant
Description
Fixes stale docs:
Reviewer checklist