T8599: Make source packages required and fix them - #1175
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)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (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). (3)
🧰 Additional context used🔍 Remote MCP Context7Summary of Additional Technical Context for PR ReviewBased on the documentation retrieved, here are relevant technical details that validate and clarify aspects of the PR: Python subprocess.CalledProcessError HandlingThe PR's approach to error handling aligns with standard Python subprocess patterns:
TOML Configuration FormatThe PR's addition of
Key Validation PointsThe technical documentation confirms that:
The previous research context combined with this technical validation provides a strong foundation for reviewing the PR's implementation. 📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesDebian package build improvements
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify 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 |
|
👍 |
There was a problem hiding this comment.
Pull request overview
Updates the package build helper to fail fast when dpkg-buildpackage (including dpkg-source patch application) errors, instead of continuing with a binaries-only build that can hide patch failures in logs.
Changes:
- Stop ignoring source package build failures and exit the script with an error.
- Update console output to surface the failure as an error.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| print("I: Source packages build failed, ignoring - building binaries only") | ||
| build_cmd = package.get('build_cmd', 'dpkg-buildpackage -uc -us -tc -b') | ||
| run(build_cmd, cwd=repo_dir, check=True, shell=True) | ||
| print("E: Source packages build failed, possibly patches problem") |
There was a problem hiding this comment.
The message "Source packages build failed, possibly patches problem" will be printed for any failure of build_cmd (e.g., compile/test failures too), so it may be misleading. Recommend making the message generic (include repo_name/build_cmd), and/or only mention patches when the failure is specifically detected as a dpkg-source/patch application error.
| print("E: Source packages build failed, possibly patches problem") | |
| print(f'E: Source package build failed for "{repo_name}" while running: {build_cmd}') |
0d1f1dd to
0042227
Compare
hedrok
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/package-build/build.py (1)
159-185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent error handling: tarball failure silently continues.
Per PR objectives, source package build errors should fail the build. However, if tarball creation at line 159 fails (
check=TrueraisesCalledProcessError), it's caught by line 184, which prints the error but does not exit. The script continues to the next package.This contradicts the explicit
sys.exit(1)at line 182 fordpkg-buildpackagefailures.Proposed fix: unify exit behavior
except CalledProcessError as e: - print(f"Failed to build package {repo_name}: {e}") + print(f"E: Failed to build package {repo_name}: {e}") + sys.exit(1) finally:Alternatively, move the outer
try/exceptto only wrap non-critical sections if partial failures are intentional.🤖 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/build.py` around lines 159 - 185, The tarball creation error is currently caught by the outer CalledProcessError handler and only logged, allowing the script to continue; change behavior so a failing tar creation aborts the build like dpkg-buildpackage failures: when run(['tar', ...], check=True) raises CalledProcessError, call sys.exit(1) (or re-raise) instead of merely printing, or narrow the outer try/except to exclude the tar creation block so that CalledProcessError from the tar command propagates; reference the tarball creation call (tarball_name, repo_name, repo_dir), the CalledProcessError handler and sys.exit usage near the dpkg-buildpackage handling to implement the unified exit behavior.
🤖 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/build.py`:
- Around line 140-155: The dpkg-parsechangelog invocations assign to
package_version and package_name but only check stdout; instead verify the
CompletedProcess.returncode is 0 before trusting stdout (i.e., after run([...])
check both result.returncode == 0 and result.stdout) and fall back to
package['commit_id'].replace('/', '_') for package_version or repo_name for
package_name when returncode != 0 or stdout is empty; update the logic around
the run(...) results (the variables named package_version and package_name and
the commands invoking dpkg-parsechangelog with repo_dir) so you only
decode/process stdout when the command succeeded, otherwise use the existing
fallback branches.
---
Outside diff comments:
In `@scripts/package-build/build.py`:
- Around line 159-185: The tarball creation error is currently caught by the
outer CalledProcessError handler and only logged, allowing the script to
continue; change behavior so a failing tar creation aborts the build like
dpkg-buildpackage failures: when run(['tar', ...], check=True) raises
CalledProcessError, call sys.exit(1) (or re-raise) instead of merely printing,
or narrow the outer try/except to exclude the tar creation block so that
CalledProcessError from the tar command propagates; reference the tarball
creation call (tarball_name, repo_name, repo_dir), the CalledProcessError
handler and sys.exit usage near the dpkg-buildpackage handling to implement the
unified exit behavior.
🪄 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: 4770785f-d394-4a2a-98b2-707b3fa52beb
📒 Files selected for processing (1)
scripts/package-build/build.py
📜 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). (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/build.py
🪛 OpenGrep (1.22.0)
scripts/package-build/build.py
[ERROR] 179-179: 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.15.15)
scripts/package-build/build.py
[error] 140-140: subprocess call: check for execution of untrusted input
(S603)
[error] 140-140: Starting a process with a partial executable path
(S607)
[error] 150-150: subprocess call: check for execution of untrusted input
(S603)
[error] 150-150: Starting a process with a partial executable path
(S607)
[error] 179-179: subprocess call with shell=True identified, security issue
(S602)
🔍 Remote MCP
I'll search for relevant information about the Debian packaging tools and practices mentioned in this PR to help with the review.Based on the search results, I have sufficient information about the Debian source packaging conventions and tools mentioned in the PR. Let me compile the relevant findings.
Summary of Relevant Context for PR Review
Debian Source Package Naming Convention
The packaging tools require that the tarball complies with the naming convention: source package name, underscore, upstream version number, followed by .orig.tar.gz. The PR changes the naming scheme for .orig.tar.gz files from using a sanitized commit ID to using proper Debian source package and upstream version values extracted from debian/changelog. The name is as follows: source package name, underscore, upstream version number, followed by .orig.tar.gz. This is a critical standard that if you have renamed the upstream tarfile into _.orig.tar.gz and named the directory like -, dpkg-source -b should be able to build proper source packages.
dpkg-parsechangelog Tool
dpkg-parsechangelog reads and parses the changelog of an unpacked Debian source tree and outputs the information in it to standard output in a machine-readable form. The Source field extracts the source package name, and the Version field extracts the source version number. Its main purpose is to provide machine-readable output for scripts and build tools, enabling automated processes (like dpkg-buildpackage) to determine the current package version and integrate changelog entries into the build and release workflow.
Source Package Build Process and .orig.tar.gz Requirements
Building a non-native package involves extracting the original tarball in a separate ".orig" directory and regenerating the .diff.gz by comparing the source package directory with the .orig directory. The source packaging tools manage the changes between the original and Debian source using diff and patch. Turning the original source tree as included in the .orig.tar.gz into the Debian package source must not involve any changes which cannot be handled by these tools. This highlights why the PR's approach of using proper naming and cleaning build artifacts is important.
Build-deps Cleanup Implications
The PR's cleanup of *build-deps*.{deb,buildinfo,changes} files is relevant because dpkg-source is frequently used and also called from package-independent automated building scripts such as dpkg-buildpackage. Build artifacts left in the source directory would be detected as modifications to upstream sources when building the source package, which is why cleanup before invoking dpkg-source is necessary.
Error Handling in dpkg-buildpackage
dpkg-buildpackage calls dpkg-source -b to generate the source package (if a source build has been requested with --build or equivalent options, and if no .dsc has been specified). The PR's change to make source package build failures fatal (rather than being ignored or retried) aligns with ensuring build failures surface properly during the source package generation step.
[::web_search::]
🔇 Additional comments (2)
scripts/package-build/build.py (2)
178-179: LGTM!
196-198: LGTM!
| package_version = run(['dpkg-parsechangelog', '--show-field', 'Version', '--file', repo_dir / 'debian/changelog'], capture_output=True, check=False) | ||
| if package_version.stdout: | ||
| package_version = package_version.stdout.decode().strip() | ||
| package_version = package_version.rsplit('-', maxsplit=1)[0] | ||
| if ':' in package_version: | ||
| package_version = package_version.split(':', maxsplit=1)[1] | ||
| else: | ||
| # On failure fallback to sanitized commit ID | ||
| package_version = package['commit_id'].replace('/', '_') | ||
|
|
||
| package_name = run(['dpkg-parsechangelog', '--show-field', 'Source', '--file', repo_dir / 'debian/changelog'], capture_output=True, check=False) | ||
| if package_name.stdout: | ||
| package_name = package_name.stdout.decode().strip() | ||
| else: | ||
| # On failure fallback to repo name | ||
| package_name = repo_name |
There was a problem hiding this comment.
Check returncode alongside stdout for robustness.
Both dpkg-parsechangelog calls use check=False and test if package_version.stdout:. If the command fails but writes partial/unexpected data to stdout, you'd use garbage as the version or source name. Validate success explicitly:
- package_version = run(['dpkg-parsechangelog', '--show-field', 'Version', '--file', repo_dir / 'debian/changelog'], capture_output=True, check=False)
- if package_version.stdout:
+ result = run(['dpkg-parsechangelog', '--show-field', 'Version', '--file', repo_dir / 'debian/changelog'], capture_output=True, check=False)
+ if result.returncode == 0 and result.stdout:
+ package_version = result.stdout.decode().strip()Same pattern applies to the Source field extraction at line 150.
🧰 Tools
🪛 Ruff (0.15.15)
[error] 140-140: subprocess call: check for execution of untrusted input
(S603)
[error] 140-140: Starting a process with a partial executable path
(S607)
[error] 150-150: subprocess call: check for execution of untrusted input
(S603)
[error] 150-150: 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/build.py` around lines 140 - 155, The
dpkg-parsechangelog invocations assign to package_version and package_name but
only check stdout; instead verify the CompletedProcess.returncode is 0 before
trusting stdout (i.e., after run([...]) check both result.returncode == 0 and
result.stdout) and fall back to package['commit_id'].replace('/', '_') for
package_version or repo_name for package_name when returncode != 0 or stdout is
empty; update the logic around the run(...) results (the variables named
package_version and package_name and the commands invoking dpkg-parsechangelog
with repo_dir) so you only decode/process stdout when the command succeeded,
otherwise use the existing fallback branches.
| # Check if the 'patches' directory exists in the repository | ||
| if (repo_dir / 'patches'): | ||
| apply_patches(repo_dir, patch_dir / repo_name) | ||
|
|
||
| # Sanitize the commit ID and build a tarball for the package | ||
| commit_id_sanitized = package['commit_id'].replace('/', '_') | ||
| tarball_name = f"{repo_name}_{commit_id_sanitized}.tar.gz" | ||
| # Create original tarball for dpkg-source to be happy |
| # Create original tarball for dpkg-source to be happy | ||
| package_version = run(['dpkg-parsechangelog', '--show-field', 'Version', '--file', repo_dir / 'debian/changelog'], capture_output=True, check=False) | ||
| if package_version.stdout: | ||
| package_version = package_version.stdout.decode().strip() | ||
| package_version = package_version.rsplit('-', maxsplit=1)[0] | ||
| if ':' in package_version: | ||
| package_version = package_version.split(':', maxsplit=1)[1] | ||
| else: | ||
| # On failure fallback to sanitized commit ID | ||
| package_version = package['commit_id'].replace('/', '_') | ||
|
|
||
| package_name = run(['dpkg-parsechangelog', '--show-field', 'Source', '--file', repo_dir / 'debian/changelog'], capture_output=True, check=False) | ||
| if package_name.stdout: | ||
| package_name = package_name.stdout.decode().strip() | ||
| else: | ||
| # On failure fallback to repo name | ||
| package_name = repo_name |
| except CalledProcessError as e: | ||
| print(f"Failed to build package {repo_name}: {e}") |
| for suffix in ('deb', 'buildinfo', 'changes'): | ||
| for file in glob.glob(str(repo_dir / f'*build-deps*.{suffix}')): | ||
| os.remove(file) |
sever-sever
left a comment
sever-sever
left a comment
There was a problem hiding this comment.
Fix the source packages' build
Fix ignoring errors during package build
Tested and seems to work as expected.
Thanks @hedrok
|
Updated PR with new commit as Copilot noticed that some errors are ignored anyway. Moving PR to draft till I rerun |
alexandr-san4ez
left a comment
alexandr-san4ez
left a comment
There was a problem hiding this comment.
I tested the build process with several packages:
- amazon-cloudwatch-agent
- udp-broadcast-relay
- waagent
- wide-dhcpv6
- xen-guest-agent
- zerotier-one
No objections.
A lot of packages failed to build source package. Stop ignoring source package build errors and fix it. To fix source packages: * Use <source_package>_<upstream_version>.orig.tar.gz naming for source archive. * Get `source_package` and `upstream_version` from changelog using dpkg-parsechangelog utility * Clean build-deps after usage or dpkg-source sees these files as changes relative to upstream. * Add '.github' to --diff-ignore source option
Exit with error for other failures: * Creating/installing dependency package should be error: e.g. debugging why strongswan cannot find systemd could be easier if build.py failed when it couldn't install systemd and not when `configure` couldn't find it. * Creating tarball * pre_hook run
Patch modifies debian/control and debian/rules, there is no easy way to fix source package creation in such case. As quick solution disable source package build for this package (previously it failed silently)
bb73f42 to
845f9c5
Compare
|
I have rechecked all packages (except Linux kernel as I have some problems with connecting to packages.vyos.net, but it should not be affected - there are custom build commands) Additional fixes/notes: dropbear Our patch changes net-snmp Same in both strongswan Now build failure is obvious because of additional checks, in vyos-1x Uses 1.0 source format + has no correct version, it seems this combination cannot work with '.orig' source format that I've added. I've read documentation a little bit (https://www.man7.org/linux/man-pages/man1/dpkg-source.1.html), 3.0 source format requires '.orig', but 1.0 - no '.orig' in tarball filename. Added check of source format for that. Also in both rolling and my PR I currently get an error: |
|
Tick the box to add this pull request to the merge queue (same as
|
1.0 source format version needs tarball without '.orig' suffix as described in https://www.man7.org/linux/man-pages/man1/dpkg-source.1.html. Only if debian/source/format exists and contains 3.0, use '.orig'
845f9c5 to
c596d7e
Compare
vyos-bot
Bot
added
mirror-initiated
Change summary
T8599: Make source packages required and fix them
A lot of packages failed to build source package.
Stop ignoring source package build errors and fix it.
Also removes rerun when
build_cmdwas used - because of this patch application problems were hidden.To fix source packages:
source_packageandupstream_versionfrom changelog using dpkg-parsechangelog utilityWhen patch fails to apply exit with error instead of ignoring it.
Types of changes
Related Task(s)
Related PR(s)
How to test / Smoketest result
Source packages problem:
Try to build
ddclient, there is message that source package build failed.After PR: source package build succeeds.
Problem with applying patches in FRR:
Add failing patch, try to build. Result before this PR:
Inside of build log - package builds successfully, it is extremely easy not to see this.
With PR:
With failure.
Checklist: