Kernel: T9181: modernize packaging and remove redundant work - #1262
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesKernel package builders now use shared validation, explicit kernel flavor propagation, and Debian-native packaging. New shell builders replace deleted Python builders for Jool and Realtek drivers. VPP builds can reuse existing libraries and tolerate absent package artifacts. Kernel packaging changes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Comment |
fecad94 to
0d78fa7
Compare
Accel-PPP-ng only links against a few VPP libraries, but the build triggered a full VPP package rebuild (make pkg-deb) on every run, with no caching. Skip that step when the required VPP libraries already exist from a previous run. Delete them to force a rebuild.
…o bash Rewrite build-jool.py, build-realtek-r8126.py and build-realtek-r8152.py as build-*.sh, matching the Bash convention already used by every other sub-dependency script in this directory (kernel-vars sourcing, debmake + debuild packaging).
build-kernel.sh parsed kernel_flavor out of defaults.toml itself via a brittle awk one-liner, even though build.py already has that value from its own defaults.toml read. Pass kernel_flavor down as an env var instead, so defaults.toml is read in exactly one place (build.py's main) and everything downstream flows through kernel-vars or explicit parameters.
Convert build-nat-rtsp.sh, build-ipt-netflow.sh, build-intel-nic.sh, build-intel-qat.sh and build-linux-firmware.sh from fpm to debmake + debuild, matching the jool/realtek packaging style.
0d78fa7 to
07a3aa7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
scripts/package-build/linux-kernel/build-ipt-netflow.sh (1)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the Debian multiarch triplet over
uname -m.
dpkg-architecture -qDEB_HOST_MULTIARCHreports the exact path component dpkg expects.uname -mhappens to match on amd64 and arm64, but it reports the build host, not the target architecture.♻️ Proposed change
-UNAME_ARCH=$(uname -m) +MULTIARCH=$(dpkg-architecture -qDEB_HOST_MULTIARCH)Then use
${MULTIARCH}at lines 87-88.🤖 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/linux-kernel/build-ipt-netflow.sh` at line 36, Replace the UNAME_ARCH assignment with a MULTIARCH value obtained from dpkg-architecture -qDEB_HOST_MULTIARCH, then update the architecture-dependent paths around the existing uses at lines 87–88 to reference ${MULTIARCH}. Ensure the build uses the Debian target triplet rather than the host architecture reported by uname.scripts/package-build/linux-kernel/build-intel-nic.sh (1)
87-87: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse unsigned binary-only builds.
debmakegeneratesUNRELEASEDchangelogs, so missing GPG keys do not make thesedebuildcalls fail. Baredebuildstill builds an unnecessary source package. Replace each listed invocation withdebuild -i -us -uc -b, especially to avoid archiving the firmware tree inbuild-linux-firmware.sh.🤖 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/linux-kernel/build-intel-nic.sh` at line 87, Replace each bare debuild invocation with unsigned binary-only options (-i -us -uc -b) in scripts/package-build/linux-kernel/build-intel-nic.sh:87, build-intel-qat.sh:121, build-ipt-netflow.sh:92, build-nat-rtsp.sh:72, build-jool.sh:59-61, build-realtek-r8126.sh:58-62, build-realtek-r8152.sh:59-63, and build-linux-firmware.sh:132; ensure the firmware build does not archive the firmware tree.
🤖 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/linux-kernel/build-accel-ppp-ng.sh`:
- Around line 26-30: Update the VPP cache logic in the build script to reuse
artifacts only when the cache metadata records the current VPP commit_id, all
required library files exist, and the prior build completed successfully. When
validation fails, run ./build.py from the VPP source directory with fail-fast
error propagation, and write the commit metadata only after a successful build;
ensure directory changes and build failures cause the script to exit rather than
continue with partial or stale output.
- Around line 79-82: Update the VPP package-copy logic in build-accel-ppp-ng.sh
to detect whether any matching .deb files exist before invoking cp; skip the
copy only when none are present. When packages exist, quote all paths, run cp
without masking its status, and preserve its stderr so permission, disk,
missing-directory, or partial-copy failures cause the script—and build.py’s
check=True caller—to fail.
In `@scripts/package-build/linux-kernel/build-jool.sh`:
- Line 24: Make all three archive-based builders non-interactive by adding
debmake’s confirmation flag to the invocations in
scripts/package-build/linux-kernel/build-jool.sh:24-24,
scripts/package-build/linux-kernel/build-realtek-r8126.sh:24-24, and
scripts/package-build/linux-kernel/build-realtek-r8152.sh:24-24. Then update
build_jool in scripts/package-build/linux-kernel/build.py:255-257 to invoke
build-jool.sh directly with run and check enabled, removing the shell=True and
piped-stdin workaround.
- Line 24: Update the debmake invocation in the build script to include the
non-interactive confirmation option -y, keeping the existing package metadata
and source archive arguments unchanged so the script no longer requires piped
shell input.
- Around line 46-53: Remove the $@ arguments from the dh_auto_build and
dh_auto_install invocations in the override_dh_auto_build and
override_dh_auto_install targets. Leave the kernel module make commands
unchanged, and pass only supported build-system parameters after -- if any are
needed.
In `@scripts/package-build/linux-kernel/build-nat-rtsp.sh`:
- Line 25: Update the VERSION assignment using debian_version in the nat-rtsp
build script to remove one leading “v” from git describe output before
conversion, matching the normalization used by build-intel-nic.sh and
build-ipt-netflow.sh.
In `@scripts/package-build/linux-kernel/build.py`:
- Around line 255-257: Update build_jool() to invoke build-jool.sh without
shell=True or the echo y pipe, matching the other builder functions. Modify
build-jool.sh so its debmake invocation includes -y, preserving non-interactive
behavior while clearing the command-injection and subprocess lint findings.
---
Nitpick comments:
In `@scripts/package-build/linux-kernel/build-intel-nic.sh`:
- Line 87: Replace each bare debuild invocation with unsigned binary-only
options (-i -us -uc -b) in
scripts/package-build/linux-kernel/build-intel-nic.sh:87,
build-intel-qat.sh:121, build-ipt-netflow.sh:92, build-nat-rtsp.sh:72,
build-jool.sh:59-61, build-realtek-r8126.sh:58-62, build-realtek-r8152.sh:59-63,
and build-linux-firmware.sh:132; ensure the firmware build does not archive the
firmware tree.
In `@scripts/package-build/linux-kernel/build-ipt-netflow.sh`:
- Line 36: Replace the UNAME_ARCH assignment with a MULTIARCH value obtained
from dpkg-architecture -qDEB_HOST_MULTIARCH, then update the
architecture-dependent paths around the existing uses at lines 87–88 to
reference ${MULTIARCH}. Ensure the build uses the Debian target triplet rather
than the host architecture reported by uname.
🪄 Autofix
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: e72a00cc-685f-4165-bf8a-88a91151aa54
📒 Files selected for processing (16)
scripts/package-build/linux-kernel/build-accel-ppp-ng.shscripts/package-build/linux-kernel/build-intel-nic.shscripts/package-build/linux-kernel/build-intel-qat.shscripts/package-build/linux-kernel/build-ipt-netflow.shscripts/package-build/linux-kernel/build-jool.pyscripts/package-build/linux-kernel/build-jool.shscripts/package-build/linux-kernel/build-kernel.shscripts/package-build/linux-kernel/build-linux-firmware.shscripts/package-build/linux-kernel/build-mellanox-ofed.shscripts/package-build/linux-kernel/build-nat-rtsp.shscripts/package-build/linux-kernel/build-realtek-r8126.pyscripts/package-build/linux-kernel/build-realtek-r8126.shscripts/package-build/linux-kernel/build-realtek-r8152.pyscripts/package-build/linux-kernel/build-realtek-r8152.shscripts/package-build/linux-kernel/build.pyscripts/package-build/linux-kernel/common.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
💤 Files with no reviewable changes (3)
- scripts/package-build/linux-kernel/build-jool.py
- scripts/package-build/linux-kernel/build-realtek-r8152.py
- scripts/package-build/linux-kernel/build-realtek-r8126.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- 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/linux-kernel/build.py
🪛 ast-grep (0.45.0)
scripts/package-build/linux-kernel/build-accel-ppp-ng.sh
[warning] 81-81: mv/cp is invoked with an unquoted variable argument (e.g. mv $src $dst). Unquoted expansions undergo word splitting and pathname (glob) expansion, so a value containing spaces, */?/[, or leading - can split into extra arguments, expand to unintended files, or be parsed as an option — leading to files being moved/overwritten/clobbered at attacker-controlled paths. Always double-quote the expansion and terminate options with --: mv -- "$src" "$dst".
Context: cp ${CWD}/../vpp/*.deb ${CWD}
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(mv-cp-unquoted-variable-bash)
scripts/package-build/linux-kernel/build-realtek-r8126.sh
[error] 61-61: Recursive rm is given an unquoted variable expansion as a target. If the variable is empty, contains whitespace, or holds glob characters, word splitting and pathname expansion can make rm delete unintended paths (e.g. rm -rf $dir with dir="" or dir="/ tmp"). Always double-quote the expansion and guard against empty values: rm -rf -- "${dir:?dir is unset}".
Context: ${PACKAGE_DIR}
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(rm-rf-unquoted-variable-bash)
scripts/package-build/linux-kernel/build-realtek-r8152.sh
[error] 62-62: Recursive rm is given an unquoted variable expansion as a target. If the variable is empty, contains whitespace, or holds glob characters, word splitting and pathname expansion can make rm delete unintended paths (e.g. rm -rf $dir with dir="" or dir="/ tmp"). Always double-quote the expansion and guard against empty values: rm -rf -- "${dir:?dir is unset}".
Context: ${PACKAGE_DIR}
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(rm-rf-unquoted-variable-bash)
🪛 OpenGrep (1.26.0)
scripts/package-build/linux-kernel/build.py
[ERROR] 257-257: Dynamic command passed to subprocess with shell=True. Use a command list without shell=True, or use shlex.quote() to sanitize input.
(coderabbit.command-injection.python-shell-true)
🪛 Ruff (0.16.1)
scripts/package-build/linux-kernel/build.py
[error] 257-257: subprocess call with shell=True identified, security issue
(S602)
[error] 257-257: Starting a process with a partial executable path
(S607)
🪛 Shellcheck (0.11.0)
scripts/package-build/linux-kernel/build-accel-ppp-ng.sh
[info] 26-26: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 27-27: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[info] 29-29: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 82-82: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 82-82: Double quote to prevent globbing and word splitting.
(SC2086)
scripts/package-build/linux-kernel/build-realtek-r8126.sh
[info] 6-6: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 11-11: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
[info] 11-11: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 60-60: Double quote to prevent globbing and word splitting.
(SC2086)
scripts/package-build/linux-kernel/build-jool.sh
[info] 6-6: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 11-11: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
[info] 11-11: Double quote to prevent globbing and word splitting.
(SC2086)
scripts/package-build/linux-kernel/build-realtek-r8152.sh
[info] 6-6: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 11-11: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
[info] 11-11: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 61-61: Double quote to prevent globbing and word splitting.
(SC2086)
scripts/package-build/linux-kernel/build-linux-firmware.sh
[info] 10-10: Not following: ./common.sh was not specified as input (see shellcheck -x).
(SC1091)
[info] 10-10: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 95-95: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 97-97: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 134-134: Double quote to prevent globbing and word splitting.
(SC2086)
scripts/package-build/linux-kernel/build-ipt-netflow.sh
[info] 5-5: Not following: ./common.sh was not specified as input (see shellcheck -x).
(SC1091)
[info] 5-5: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 38-38: Double quote to prevent globbing and word splitting.
(SC2086)
scripts/package-build/linux-kernel/build-nat-rtsp.sh
[info] 5-5: Not following: ./common.sh was not specified as input (see shellcheck -x).
(SC1091)
[info] 5-5: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 27-27: Double quote to prevent globbing and word splitting.
(SC2086)
scripts/package-build/linux-kernel/build-intel-qat.sh
[info] 5-5: Not following: ./common.sh was not specified as input (see shellcheck -x).
(SC1091)
[info] 5-5: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 58-58: Use '[:upper:]' to support accents and foreign alphabets.
(SC2019)
[info] 58-58: Use '[:lower:]' to support accents and foreign alphabets.
(SC2018)
[info] 61-61: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 61-61: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 64-64: Double quote to prevent globbing and word splitting.
(SC2086)
scripts/package-build/linux-kernel/build-intel-nic.sh
[info] 5-5: Not following: ./common.sh was not specified as input (see shellcheck -x).
(SC1091)
[info] 5-5: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 42-42: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 42-42: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
scripts/package-build/linux-kernel/build-mellanox-ofed.sh
[info] 6-6: Not following: ./common.sh was not specified as input (see shellcheck -x).
(SC1091)
[info] 6-6: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 8-8: Quote this to prevent word splitting.
(SC2046)
🔍 Remote MCP Context7, vyos.dev
Additional review context
- Matching task T9181, “Kernel: modernize packaging and remove redundant work” is In progress, priority Low, and covers the exact stated goals: replacing legacy packaging, correcting version/package-name issues, standardizing scripts, avoiding duplicate kernel-flavor parsing, and preventing redundant VPP rebuilds. It has no comments.
- T7960 reports a resolved high-priority issue where image builds used stale VPP binaries; it documents differing available and installed VPP package revisions. This makes version/freshness handling important when skipping VPP builds.
-
T7749 records a resolved ARM failure caused by hardcoded
x86_64VPP-library builds. Since this PR introduces/reuses amd64 checks, architecture scope should be verified. - Debian policy requires upstream versions to begin with a digit; hyphens require a Debian revision, while Debian revisions cannot contain hyphens. The documented
debian_versionnormalization is consistent with these rules. -
debuildwrapsdpkg-buildpackage, runs lintian by default, and handles signing. Debian documentsdebuild -i -us -uc -bfor unsigned binary-only builds; verify each new invocation requests the intended build/signing mode.
🔇 Additional comments (9)
scripts/package-build/linux-kernel/common.sh (2)
7-12: LGTM!
23-29: LGTM!scripts/package-build/linux-kernel/build.py (2)
208-210: LGTM!
140-140: 🩺 Stability & AvailabilityNo issue:
kernel_flavoris defined indata/defaults.tomland preserved bymerge_dicts.> Likely an incorrect or invalid review comment.scripts/package-build/linux-kernel/build-kernel.sh (1)
25-31: LGTM!scripts/package-build/linux-kernel/build-intel-nic.sh (1)
39-45: LGTM!scripts/package-build/linux-kernel/build-intel-qat.sh (1)
58-119: LGTM!scripts/package-build/linux-kernel/build-linux-firmware.sh (1)
93-130: LGTM!scripts/package-build/linux-kernel/build-mellanox-ofed.sh (1)
6-6: LGTM!Also applies to: 13-13
| if [ ! -d ${VPP_LIB_CHECK_PATH} ]; then | ||
| cd ../vpp/ | ||
| ./build.py | ||
| cd ${CWD} | ||
| fi |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make VPP reuse commit-aware and fail closed.
-d "$VPP_LIB_CHECK_PATH" proves only that a directory exists. It does not prove that the required libraries exist, that they match the current VPP commit_id, or that the previous build completed successfully. If the VPP source changes while build-root remains, this script can link stale libraries. If ./build.py fails after creating the directory, the script can also continue with partial output.
Store the VPP commit with the cached libraries, validate the required library files, and propagate directory and build failures.
Suggested failure handling
-if [ ! -d ${VPP_LIB_CHECK_PATH} ]; then
- cd ../vpp/
- ./build.py
- cd ${CWD}
+if [ ! -d "$VPP_LIB_CHECK_PATH" ]; then
+ (
+ cd ../vpp/ || exit 1
+ ./build.py
+ ) || exit 1
fi🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 26-26: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 27-27: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 29-29: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[info] 29-29: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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/linux-kernel/build-accel-ppp-ng.sh` around lines 26 -
30, Update the VPP cache logic in the build script to reuse artifacts only when
the cache metadata records the current VPP commit_id, all required library files
exist, and the prior build completed successfully. When validation fails, run
./build.py from the VPP source directory with fail-fast error propagation, and
write the commit metadata only after a successful build; ensure directory
changes and build failures cause the script to exit rather than continue with
partial or stale output.
Source: MCP tools
| # move VPP binaries to linux-kernel dir, CI will get VPP .deb here. | ||
| # If the VPP build was skipped above (libraries already present from a | ||
| # prior run), there may be no fresh .deb here to copy - that's fine. | ||
| cp ${CWD}/../vpp/*.deb ${CWD} 2>/dev/null || echo "I: No freshly built VPP .deb to copy" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not convert VPP package copy failures into success.
cp ... || echo treats “no matching .deb” and real failures identically. Permission errors, disk-full errors, missing source directories, and partial copies all make this script return success. The call at Lines 221-225 in scripts/package-build/linux-kernel/build.py uses check=True, so CI can accept an incomplete VPP package set.
Detect the no-match case before copying. Exit on real copy failures, preserve the error output, and quote all paths.
Suggested package handling
-cp ${CWD}/../vpp/*.deb ${CWD} 2>/dev/null || echo "I: No freshly built VPP .deb to copy"
+vpp_package_found=0
+for package in "${CWD}"/../vpp/*.deb; do
+ [ -e "$package" ] || continue
+ vpp_package_found=1
+ cp -- "$package" "$CWD" || exit 1
+done
+if [ "$vpp_package_found" -eq 0 ]; then
+ echo "I: No freshly built VPP .deb to copy"
+fi📝 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.
| # move VPP binaries to linux-kernel dir, CI will get VPP .deb here. | |
| # If the VPP build was skipped above (libraries already present from a | |
| # prior run), there may be no fresh .deb here to copy - that's fine. | |
| cp ${CWD}/../vpp/*.deb ${CWD} 2>/dev/null || echo "I: No freshly built VPP .deb to copy" | |
| # move VPP binaries to linux-kernel dir, CI will get VPP .deb here. | |
| # If the VPP build was skipped above (libraries already present from a | |
| # prior run), there may be no fresh .deb here to copy - that's fine. | |
| vpp_package_found=0 | |
| for package in "${CWD}"/../vpp/*.deb; do | |
| [ -e "$package" ] || continue | |
| vpp_package_found=1 | |
| cp -- "$package" "$CWD" || exit 1 | |
| done | |
| if [ "$vpp_package_found" -eq 0 ]; then | |
| echo "I: No freshly built VPP .deb to copy" | |
| fi |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 81-81: mv/cp is invoked with an unquoted variable argument (e.g. mv $src $dst). Unquoted expansions undergo word splitting and pathname (glob) expansion, so a value containing spaces, */?/[, or leading - can split into extra arguments, expand to unintended files, or be parsed as an option — leading to files being moved/overwritten/clobbered at attacker-controlled paths. Always double-quote the expansion and terminate options with --: mv -- "$src" "$dst".
Context: cp ${CWD}/../vpp/*.deb ${CWD}
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(mv-cp-unquoted-variable-bash)
🪛 Shellcheck (0.11.0)
[info] 82-82: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 82-82: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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/linux-kernel/build-accel-ppp-ng.sh` around lines 79 -
82, Update the VPP package-copy logic in build-accel-ppp-ng.sh to detect whether
any matching .deb files exist before invoking cp; skip the copy only when none
are present. When packages exist, quote all paths, run cp without masking its
status, and preserve its stderr so permission, disk, missing-directory, or
partial-copy failures cause the script—and build.py’s check=True caller—to fail.
Source: Linters/SAST tools
| fi | ||
| curl -L -o ${SOURCES_ARCHIVE} ${SOURCES_URL} | ||
|
|
||
| debmake -e support@vyos.io -f "VyOS Support" -p ${PACKAGE_NAME} -u ${PACKAGE_VERSION} -a ${SOURCES_ARCHIVE} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
The three converted shell builders call debmake interactively. build-intel-nic.sh, build-intel-qat.sh, build-ipt-netflow.sh, build-nat-rtsp.sh, and build-linux-firmware.sh all pass -y. The three new archive-based builders do not, so debmake waits for confirmation. build.py hides this for Jool only, by piping echo y through a shell; the two realtek scripts get no stdin at all.
scripts/package-build/linux-kernel/build-jool.sh#L24: add-yto thedebmakeinvocation.scripts/package-build/linux-kernel/build-realtek-r8126.sh#L24: add-yto thedebmakeinvocation.scripts/package-build/linux-kernel/build-realtek-r8152.sh#L24: add-yto thedebmakeinvocation.scripts/package-build/linux-kernel/build.py#L255-L257: after the scripts are non-interactive, changebuild_jooltorun(['./build-jool.sh'], check=True). This removes theshell=Trueworkaround flagged by Ruff S602/S607 and OpenGrep.
📍 Affects 4 files
scripts/package-build/linux-kernel/build-jool.sh#L24-L24(this comment)scripts/package-build/linux-kernel/build-realtek-r8126.sh#L24-L24scripts/package-build/linux-kernel/build-realtek-r8152.sh#L24-L24scripts/package-build/linux-kernel/build.py#L255-L257
🤖 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/linux-kernel/build-jool.sh` at line 24, Make all three
archive-based builders non-interactive by adding debmake’s confirmation flag to
the invocations in scripts/package-build/linux-kernel/build-jool.sh:24-24,
scripts/package-build/linux-kernel/build-realtek-r8126.sh:24-24, and
scripts/package-build/linux-kernel/build-realtek-r8152.sh:24-24. Then update
build_jool in scripts/package-build/linux-kernel/build.py:255-257 to invoke
build-jool.sh directly with run and check enabled, removing the shell=True and
piped-stdin workaround.
Source: Linters/SAST tools
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add -y to debmake.
Without -y, debmake waits for interactive confirmation. build.py works around this by piping echo y through a shell. Make the script self-contained.
♻️ Proposed change
-debmake -e support@vyos.io -f "VyOS Support" -p ${PACKAGE_NAME} -u ${PACKAGE_VERSION} -a ${SOURCES_ARCHIVE}
+debmake -y -e support@vyos.io -f "VyOS Support" -p ${PACKAGE_NAME} -u ${PACKAGE_VERSION} -a ${SOURCES_ARCHIVE}📝 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.
| debmake -e support@vyos.io -f "VyOS Support" -p ${PACKAGE_NAME} -u ${PACKAGE_VERSION} -a ${SOURCES_ARCHIVE} | |
| debmake -y -e support@vyos.io -f "VyOS Support" -p ${PACKAGE_NAME} -u ${PACKAGE_VERSION} -a ${SOURCES_ARCHIVE} |
🤖 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/linux-kernel/build-jool.sh` at line 24, Update the
debmake invocation in the build script to include the non-interactive
confirmation option -y, keeping the existing package metadata and source archive
arguments unchanged so the script no longer requires piped shell input.
| override_dh_auto_build: | ||
| dh_auto_build \$@ | ||
| make -C \${KERNEL_DIR} M=\$\$PWD/src/mod/common modules | ||
| make -C \${KERNEL_DIR} M=\$\$PWD/src/mod/nat64 modules | ||
| make -C \${KERNEL_DIR} M=\$\$PWD/src/mod/siit modules | ||
|
|
||
| override_dh_auto_install: | ||
| dh_auto_install \$@ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any other generated or committed debian/rules in the repo passes $@ to dh_auto_* helpers
rg -n --hidden 'dh_auto_(build|install|configure|clean)\s+\\?\$@' -g '!.git' .
# Confirm no other builder relies on this pattern
rg -n 'override_dh_auto_(build|install)' -A2 scripts/package-buildRepository: vyos/vyos-build
Length of output: 7382
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- build-jool.sh ---'
cat -n scripts/package-build/linux-kernel/build-jool.sh
printf '%s\n' '--- debhelper invocation patterns and generator context ---'
rg -n -C 4 'debmake|debuild|debian/rules|override_dh_auto|dh_auto_' scripts python 2>/dev/null || true
printf '%s\n' '--- related package-build scripts ---'
rg -l 'override_dh_auto_(build|install)' scripts/package-build/linux-kernel scripts/package-build | sort | while read -r f; do
printf '\n### %s\n' "$f"
sed -n '/override_dh_auto_build:/,/^[^[:space:]]/p' "$f" | head -n 20
sed -n '/override_dh_auto_install:/,/^[^[:space:]]/p' "$f" | head -n 20
doneRepository: vyos/vyos-build
Length of output: 50371
🌐 Web query:
Debian debhelper dh_auto_build positional arguments target syntax dh_auto_install
💡 Result:
In Debian debhelper tools like dh_auto_build and dh_auto_install, there are no traditional positional arguments for specifying build or install targets [1][2][3][4]. Instead, these tools utilize a separator syntax (--) to pass custom parameters to the underlying build system (e.g., make, cmake, setup.py) [1][5][3]. The standard command syntax for these tools is: dh_auto_build [build system options] [debhelper options] [-- params] [1][2] dh_auto_install [build system options] [debhelper options] [-- params] [3][4] Key points regarding the syntax: 1. Separator (--): Anything provided after the double dash (--) is passed directly to the underlying program (such as make) after the parameters that debhelper would normally pass [1][5][4]. For example, if you need to pass a specific argument to make that dh_auto_build does not handle, you would append it after the -- [1][3]. 2. Build System Targets: These tools automatically detect the build system (e.g., Makefile, setup.py) and run the appropriate commands [5][3]. They do not accept a "target" argument (like a specific make target) as a positional parameter [1][2][3]. 3. Overriding Behavior: If you need to run a specific target or execute entirely different commands because the automatic detection is insufficient, the recommended approach is to use the dh(1) override system in your debian/rules file [6][7]. You can define override targets to replace or augment the default behavior: # Example of overriding in debian/rules override_dh_auto_build: make universe-explode-in-delight override_dh_auto_install: $(MAKE) -C docs install By defining these override targets, you gain full control over the command execution, bypassing the limitations of the automated helper tools [6][7].
Citations:
- 1: https://manpages.debian.org/bookworm/debhelper/dh_auto_build.1.en.html
- 2: https://man7.org/linux/man-pages/man1/dh_auto_build.1.html
- 3: https://manpages.debian.org/testing/debhelper/dh_auto_install.1.en.html
- 4: https://man7.org/linux/man-pages/man1/dh_auto_install.1.html
- 5: https://github.com/Debian/debhelper/blob/master/dh_auto_install
- 6: https://manpages.debian.org/bookworm-backports/debhelper/dh.1.en.html
- 7: https://manpages.debian.org/bookworm/debhelper/dh.1.en.html
🏁 Script executed (no clone):
#!/bin/bash
set -eu
python3 - <<'PY'
import re
from pathlib import Path
text = Path("/tmp/jool-rules-template").read_text() if Path("/tmp/jool-rules-template").exists() else r'''
override_dh_auto_build:
dh_auto_build \$@
override_dh_auto_install:
dh_auto_install \$@
'''
# Model the two expansion stages used by build-jool.sh:
# 1. The unquoted heredoc turns \$@ into literal $@.
# 2. make expands the automatic variable $@ in each recipe.
rules = text.replace(r'\$@', '$@')
for target in ("override_dh_auto_build", "override_dh_auto_install"):
recipe = re.search(
rf"^{target}:\n((?:\t.*\n)+)", rules, re.MULTILINE
).group(1)
print(target)
for line in recipe.splitlines():
print(" make recipe:", line)
print(" shell argv:", line.replace("$@", target).split())
PY
printf '%s\n' '--- installed debhelper tools, if available ---'
command -v dh_auto_build || true
command -v dh_auto_install || true
if command -v dh_auto_build >/dev/null 2>&1; then
dh_auto_build --help 2>&1 | head -n 80
fi
if command -v dh_auto_install >/dev/null 2>&1; then
dh_auto_install --help 2>&1 | head -n 80
fi
Length of output: 409
🏁 Script executed (no clone):
#!/bin/bash
set -eu
for url in \
https://manpages.debian.org/bookworm/debhelper/dh_auto_build.1.en.gz \
https://manpages.debian.org/bookworm/debhelper/dh_auto_install.1.en.gz \
https://raw.githubusercontent.com/Debian/debhelper/master/dh_auto_build \
https://raw.githubusercontent.com/Debian/debhelper/master/dh_auto_install
do
printf '\n--- %s ---\n' "$url"
curl -fsSL "$url" | gzip -cd 2>/dev/null | grep -E -A8 -B4 '^\s*dh_auto_(build|install)|--\s+params|parameters passed|`@ARGV`|Unknown|extra' | head -n 80 || \
curl -fsSL "$url" | grep -E -A8 -B4 '^\s*dh_auto_(build|install)|--\s+params|parameters passed|`@ARGV`|Unknown|extra' | head -n 80
done
Length of output: 447
🏁 Script executed (no clone):
#!/bin/bash
set -eu
for url in \
https://manpages.debian.org/bookworm/debhelper/dh_auto_build.1.en.html \
https://manpages.debian.org/bookworm/debhelper/dh_auto_install.1.en.html \
https://raw.githubusercontent.com/Debian/debhelper/debhelper-13.25/dh_auto_build \
https://raw.githubusercontent.com/Debian/debhelper/debhelper-13.25/dh_auto_install
do
printf '\n--- %s ---\n' "$url"
curl -fsSL "$url" | tr '\n' ' ' | sed 's/<[^>]*>/ /g' | \
grep -oE '.{0,160}(Usage|-- params|parameters passed|Unknown option|extra arguments|ARGV).{0,300}' | head -n 12 || true
done
Length of output: 567
🏁 Script executed (no clone):
#!/bin/bash
set -eu
curl -fsSL https://api.github.com/repos/Debian/debhelper/contents \
| jq -r '.[].name' \
| grep -E '^(dh_auto_(build|install)|lib)$' || true
printf '%s\n' '--- debhelper source references ---'
for path in dh_auto_build dh_auto_install; do
url="https://raw.githubusercontent.com/Debian/debhelper/master/$path"
printf '\n### %s\n' "$path"
curl -fsSL "$url" | head -n 40 || true
done
printf '%s\n' '--- Debian manpage text via cgit ---'
for path in dh_auto_build.1 dh_auto_install.1; do
url="https://salsa.debian.org/debian/debhelper/-/raw/master/doc/$path"
printf '\n### %s\n' "$path"
curl -fsSL "$url" | grep -E -A8 -B5 'additional parameters|-- params|arguments|Usage' | head -n 80 || true
done
Length of output: 2923
🏁 Script executed (no clone):
#!/bin/bash
set -eu
for path in dh_auto_build dh_auto_install; do
printf '\n--- %s parameter handling ---\n' "$path"
curl -fsSL "https://raw.githubusercontent.com/Debian/debhelper/master/$path" |
grep -n -A18 -B4 -E 'item B<--> I<params>|params.*passed|build\(`@ARGV`\)|install\(`@ARGV`\)|`@ARGV`'
done
Length of output: 1266
Remove $@ from the dh_auto_* override targets.
Make expands $@ to the target name, so these recipes pass override_dh_auto_build and override_dh_auto_install as unsupported positional arguments. Only pass build-system parameters after --.
🐛 Proposed fix
override_dh_auto_build:
- dh_auto_build \$@
+ dh_auto_build
make -C \${KERNEL_DIR} M=\$\$PWD/src/mod/common modules
make -C \${KERNEL_DIR} M=\$\$PWD/src/mod/nat64 modules
make -C \${KERNEL_DIR} M=\$\$PWD/src/mod/siit modules
override_dh_auto_install:
- dh_auto_install \$@
+ dh_auto_install📝 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.
| override_dh_auto_build: | |
| dh_auto_build \$@ | |
| make -C \${KERNEL_DIR} M=\$\$PWD/src/mod/common modules | |
| make -C \${KERNEL_DIR} M=\$\$PWD/src/mod/nat64 modules | |
| make -C \${KERNEL_DIR} M=\$\$PWD/src/mod/siit modules | |
| override_dh_auto_install: | |
| dh_auto_install \$@ | |
| override_dh_auto_build: | |
| dh_auto_build | |
| make -C \${KERNEL_DIR} M=\$\$PWD/src/mod/common modules | |
| make -C \${KERNEL_DIR} M=\$\$PWD/src/mod/nat64 modules | |
| make -C \${KERNEL_DIR} M=\$\$PWD/src/mod/siit modules | |
| override_dh_auto_install: | |
| dh_auto_install |
🤖 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/linux-kernel/build-jool.sh` around lines 46 - 53,
Remove the $@ arguments from the dh_auto_build and dh_auto_install invocations
in the override_dh_auto_build and override_dh_auto_install targets. Leave the
kernel module make commands unchanged, and pass only supported build-system
parameters after -- if any are needed.
| mkdir -p ${DEBIAN_DIR} | ||
| cp nf_conntrack_rtsp.ko nf_nat_rtsp.ko ${DEBIAN_DIR} | ||
| PACKAGE_NAME=nat-rtsp | ||
| PACKAGE_VERSION=$(debian_version "$(git describe --tags --always)") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Strip a leading v from the tag.
build-intel-nic.sh line 40 and build-ipt-netflow.sh line 35 both apply sed s/^v//. This call does not. If nat-rtsp tags use a v prefix, debian_version returns 0~v<tag>, which sorts lower than any previously published <tag> version and blocks the upgrade path.
🐛 Proposed fix
-PACKAGE_VERSION=$(debian_version "$(git describe --tags --always)")
+PACKAGE_VERSION=$(debian_version "$(git describe --tags --always | sed s/^v//)")📝 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.
| PACKAGE_VERSION=$(debian_version "$(git describe --tags --always)") | |
| PACKAGE_VERSION=$(debian_version "$(git describe --tags --always | sed s/^v//)") |
🤖 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/linux-kernel/build-nat-rtsp.sh` at line 25, Update the
VERSION assignment using debian_version in the nat-rtsp build script to remove
one leading “v” from git describe output before conversion, matching the
normalization used by build-intel-nic.sh and build-ipt-netflow.sh.
| def build_jool(): | ||
| """Build Jool""" | ||
| run(['echo y | ./build-jool.py'], check=True, shell=True) | ||
| run(['echo y | ./build-jool.sh'], check=True, shell=True) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Drop shell=True and the echo y pipe.
The pipe only exists because build-jool.sh calls debmake without -y. Add -y in the shell script and invoke it like the other builders. This also clears Ruff S602/S607 and the OpenGrep command-injection hint.
♻️ Proposed change
def build_jool():
"""Build Jool"""
- run(['echo y | ./build-jool.sh'], check=True, shell=True)
+ run(['./build-jool.sh'], check=True)🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 257-257: Dynamic command passed to subprocess with shell=True. Use a command list without shell=True, or use shlex.quote() to sanitize input.
(coderabbit.command-injection.python-shell-true)
🪛 Ruff (0.16.1)
[error] 257-257: subprocess call with shell=True identified, security issue
(S602)
[error] 257-257: Starting a process with a partial executable path
(S607)
🤖 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/linux-kernel/build.py` around lines 255 - 257, Update
build_jool() to invoke build-jool.sh without shell=True or the echo y pipe,
matching the other builder functions. Modify build-jool.sh so its debmake
invocation includes -y, preserving non-interactive behavior while clearing the
command-injection and subprocess lint findings.
Source: Linters/SAST tools
asklymenko
left a comment
asklymenko
left a comment
There was a problem hiding this comment.
Enhance kernel build scripts.
|
Tick the box to add this pull request to the merge queue (same as
|
vyos-bot
Bot
added
mirror-initiated
2 participants
Change summary
The kernel sub-package build scripts are in an inconsistent state. Some packages are still built with a legacy packaging tool while others already use real Debian packaging, and the legacy path tolerates things Debian tooling doesn't; malformed package names, non-conforming version strings, and cleanup steps that run regardless of build failure, masking real errors.
A few sub-package scripts are written in a different language than the rest of the directory, breaking convention. Kernel flavor configuration is parsed redundantly in two places from the same config file.
Accel-PPP sub-build triggers a full, uncached rebuild of a large dependency (VPP) on every run even though only a few of its libraries are needed.
Types of changes
Related Task(s)
Related PR(s)
How to test / Smoketest result
Kernel compiled and added to custom ISO build.
All smoketests passed
Checklist: