T8969: vyos-build failing to build, missing dependencies - #1220
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🧰 Additional context used📓 Path-based instructions (1).github/workflows/**⚙️ CodeRabbit configuration file
Files:
🪛 zizmor (1.25.2).github/workflows/trigger_rebuild_packages.yml[info] 184-184: code injection via template expansion (template-injection): may expand into attacker-controllable code (template-injection) 🔍 Remote MCP Context7Additional context relevant to reviewing PR
|
| Layer / File(s) | Summary |
|---|---|
libhtp2 package build configuration scripts/package-build/libhtp/package.toml (lines 1-4), scripts/package-build/libhtp/build.py (line 1), scripts/package-build/libhtp/.gitignore (line 1), .github/workflows/trigger_rebuild_packages.yml (lines 56-57, 184-187), data/live-build-config/archives/bookworm-backports.pref.chroot (line 5) |
package.toml (1-4) adds a [[packages]] entry for libhtp2 with commit_id and scm_url. build.py (1) is a symlink to ../build.py. .gitignore (1) ignores /libhtp/ artifacts. Workflow trigger_rebuild_packages.yml (56-57, 184-187) adds a libhtp paths-filter and a conditional trigger_build "libhtp". bookworm-backports.pref.chroot (5) changes Package: suricata libhtp2 to Package: suricata, removing libhtp2 from that pin. |
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | Title references T8969 task but describes generic 'missing dependencies' issue, while actual changes focus on building libhtp2 packages from source. | Clarify title to reflect primary change: consider 'Add libhtp2 package build support' or similar to better represent the changeset's core purpose. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description check | ✅ Passed | Description relates to changeset—it identifies the change summary as 'Build libhtp2 packages from sources' and links to task T8969, aligning with modifications in build configuration files. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
✨ Simplify code
- Create PR with simplified code
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.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/package-build/libhtp/package.toml`:
- Line 3: The commit_id field in package.toml is pinned to an abbreviated SHA
("e851d98f"); replace it with the full 40-character Git commit SHA to ensure
deterministic source pinning (update the commit_id value to the repository's
full SHA for the intended commit). Ensure the new value preserves quotes and
TOML formatting and verify the full SHA corresponds to the exact upstream commit
referenced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1df63042-741c-48d4-857d-acf5cf6de1da
📒 Files selected for processing (4)
data/live-build-config/archives/bookworm-backports.pref.chrootscripts/package-build/libhtp/.gitignorescripts/package-build/libhtp/build.pyscripts/package-build/libhtp/package.toml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_iso
- GitHub Check: codeql-analysis-call / Analyze (python)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run Pylint to check for unused-imports in Python files
Files:
scripts/package-build/libhtp/build.py
🔍 Remote MCP Context7
Summary of Additional Context Found
Based on the search results, here is the relevant context for reviewing this pull request:
libhtp2 and Suricata Relationship
LibHTP is a security-aware parser for the HTTP protocol designed primarily to support the Suricata use case. The HTP Library is an HTTP normalizer and parser that integrates and provides advanced processing of HTTP streams for Suricata, and while required by the engine, may also be used independently in other applications.
Package Versioning Context
The Ubuntu/Debian ecosystem has separate packages: libhtp-dev (development files), libhtp2 (runtime library), and libhtp2-dbgsym (debug symbols). This explains the PR's change to separate the APT pin entry for Suricata and libhtp2, as they are now treated as distinct packages.
APT Pin Significance
The removal of libhtp2 from the APT pin entry alongside suricata (changing the configuration to pin only suricata to bookworm-backports) suggests that libhtp2 is being built locally from source rather than pinned to backports. This aligns with the PR's objective to build libhtp2 packages from sources instead of relying on pre-built packages.
Build System Pattern
The PR's approach of creating symbolic links to shared build scripts (as seen with build.py) and adding package metadata to package.toml follows a consistent pattern for managing multiple related packages in the vyos-build system.
Sources Used:, [::web_search::]
🔇 Additional comments (3)
scripts/package-build/libhtp/.gitignore (1)
1-1: LGTM!data/live-build-config/archives/bookworm-backports.pref.chroot (1)
5-7: LGTM!scripts/package-build/libhtp/build.py (1)
1-1: Run Pylint unused-imports on the symlinked build.py
scripts/package-build/libhtp/build.py(symlink target../build.py) andscripts/package-build/build.pypasspylint --disable=all --enable=unused-import(exit code 0; rated 10.00/10), satisfying the**/*.pyunused-imports guideline.
| @@ -0,0 +1,4 @@ | |||
| [[packages]] | |||
| name = "libhtp2" | |||
| commit_id = "e851d98f" | |||
There was a problem hiding this comment.
Use full Git commit SHA for deterministic source pinning.
Line 3 pins commit_id to an abbreviated hash (e851d98f). For build reproducibility and long-term immutability, use the full 40-character SHA to avoid future ambiguity/collision in upstream history.
Suggested change
-commit_id = "e851d98f"
+commit_id = "<full-40-char-commit-sha>"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| commit_id = "e851d98f" | |
| commit_id = "e851d98f1234567890abcdef1234567890abcdef" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/package-build/libhtp/package.toml` at line 3, The commit_id field in
package.toml is pinned to an abbreviated SHA ("e851d98f"); replace it with the
full 40-character Git commit SHA to ensure deterministic source pinning (update
the commit_id value to the repository's full SHA for the intended commit).
Ensure the new value preserves quotes and TOML formatting and verify the full
SHA corresponds to the exact upstream commit referenced.
sever-sever
left a comment
sever-sever
left a comment
There was a problem hiding this comment.
You also should add a trigger to rebuild the package
vyos-build/.github/workflows/trigger_rebuild_packages.yml
Lines 40 to 41 in 8c7bf1f
|
CI integration ❌ failed! Details
|
dmbaturin
left a comment
dmbaturin
left a comment
There was a problem hiding this comment.
We need it to unbreak rolling release builds after Debian backports dropped one of Suricata's dependencies so let's merge it.
vyos-bot
Bot
added
mirror-initiated
4 participants
Change summary
Build libhtp2 packages from sources.
Types of changes
Related Task(s)
Related PR(s)
How to test / Smoketest result
Checklist: