Skip to content

fix appsec int tests fail on frankenphp - #4182

Merged
morrisonlevi merged 2 commits into
masterfrom
glopes/fix-appsec-frankenphp
Sep 10, 2026
Merged

fix appsec int tests fail on frankenphp#4182
morrisonlevi merged 2 commits into
masterfrom
glopes/fix-appsec-frankenphp

Conversation

@cataphract

Copy link
Copy Markdown
Contributor

Description

After the appsec image upgrade, the builds of ddappsec.so started targeting a version of glibc too recent for the frankenphp image.

  • In appsec integration tests, build ddappsec.so portably. No separate musl/glibc builds now, like already happened for the tracer and libdatadog/components-rs
  • Do away with elf patching which used to remove the libc dependency. The new toolchain image adds a libc dependency that need not be removed
  • Fix some task input mismatches in build.gradle

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@cataphract
cataphract requested review from a team as code owners September 10, 2026 16:51
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-10T16:57:06.478705Z 61be8e7 PR opened
🔒 Security Review Completed 2026-09-10T16:57:06.477521Z 61be8e7 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@datadog-prod-us1-3

datadog-prod-us1-3 Bot commented Sep 10, 2026

Copy link
Copy Markdown

Pipelines  Tests

Unblock PR with BitsAI

⚠️ Warnings

Your PR has failed checks. Please review the issues below and take necessary action before merging.

🚦 6 Pipeline jobs failed

DataDog/apm-reliability/dd-trace-php | check libxml2 version — 🔧 Needs a code fix, caused by this PR

View more details · View in GitLab

DataDog/apm-reliability/dd-trace-php | helper-rust integration coverage — 🔧 Needs a code fix, caused by this PR

View more details · View in GitLab

DataDog/apm-reliability/dd-trace-php | installer tests — 🔧 Needs a code fix, caused by this PR

View more details · View in GitLab

View all 6 failed jobs.

ℹ️ Info

No other issues found (see more)

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 55.03% (+0.00%)

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: ca499db | Docs | View more details | Give us feedback!

@morrisonlevi morrisonlevi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to Claude, and it does fix the tests it aims to. I don't quite grasp it, but the way it makes it portable is instead of patching away glibc symbols, it builds them on musl to begin with. Something like that.

@morrisonlevi
morrisonlevi merged commit fcbed87 into master Sep 10, 2026
2177 of 2193 checks passed
@morrisonlevi
morrisonlevi deleted the glopes/fix-appsec-frankenphp branch September 10, 2026 23:13
@github-actions github-actions Bot added this to the 1.26.0 milestone Sep 10, 2026
@cataphract

cataphract commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

@morrisonlevi

Looks good to Claude, and it does fix the tests it aims to. I don't quite grasp it, but the way it makes it portable is instead of patching away glibc symbols, it builds them on musl to begin with. Something like that.

Sorry, I gave a truncated explanation, but to be clear what fixes it is to build ddappsec against a patched musl. With some previous toolchains, doing so would leave a NEEDED entry in the ELF for the musl shared library's explicit name, which would have to be patched away with patchelf in order to load it on glibc. However, the new toolchain doesn't require this anymore for reasons not worth going into. So I removed the step that would do the patching as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants