[ci] Publish package checksums #149#242
Conversation
Add make package/index to generate Packages file with SHA256 checksums. Filter to only include openwisp packages and save as Packages.sha256.checksum following OpenWRT's standard format. Fixes openwisp#149
shubhamk0205
force-pushed
the
issues/149-upload-package-checksums
branch
from
9ef2da4 to
32c5451
Compare
shubhamk0205
force-pushed
the
issues/149-upload-package-checksums
branch
from
4591a21 to
046744a
Compare
Ensure usign tool is compiled and installed before running make package/index. This fixes the 'usign: No such file or directory' error when cache is used. The usign tool is required for package signing during index generation.
Replace invalid tools/usign/compile target with proper tools/install. This ensures all required host tools including usign are built before running make package/index.
shubhamk0205
force-pushed
the
issues/149-upload-package-checksums
branch
4 times, most recently
from
e8daae4 to
ed1f00f
Compare
Add check for usign binary existence before running make package/index. If usign is missing from cache, build it using tools/usign/compile. Also ensures output directory exists before writing checksum file and uses improved awk filtering for better multi-package support. Closes openwisp#149
shubhamk0205
force-pushed
the
issues/149-upload-package-checksums
branch
from
ed1f00f to
49366ad
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe script updates CI defaults (BUILD_DIR, DOWNLOADS_DIR, CORES), preserves test execution unless CI=1, and defines timestamped VERSIONED_DIR and LATEST_LINK. It disables OpenWrt signed package output by clearing CONFIG_SIGNED_PACKAGES in .config, conditionally installs tools/toolchain when CI_CACHE is unset, builds the openwisp package, runs make package/index to produce Packages.manifest (renamed to sha256.manifest after verification), removes generated JSON/Packages artifacts, moves the openwisp packages directory into the VERSIONED_DIR, and recreates the latest symlink. Sequence Diagram(s)sequenceDiagram
participant CI as runbuild script
participant Tools as tools/install
participant Toolchain as toolchain/install
participant OpenWrt as make (OpenWrt build)
participant Storage as Filesystem
CI->>Tools: run if CI_CACHE unset
CI->>Toolchain: run if CI_CACHE unset
CI->>OpenWrt: make package/openwisp-config/compile
OpenWrt->>OpenWrt: make package/index (Packages.manifest)
OpenWrt->>Storage: write Packages.manifest
CI->>Storage: verify & rename to sha256.manifest, clean artifacts
CI->>Storage: move openwisp package dir -> $VERSIONED_DIR
CI->>Storage: remove & recreate $LATEST_LINK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
runbuild (1)
64-70: Consider validating the filtered output.The awk script correctly extracts package stanzas, but there's no verification that:
- The input Packages file exists and is readable
- Any openwisp packages were found and written to the checksum file
While the earlier
make package/indexshould ensure the Packages file exists, adding validation would make the script more robust and help catch unexpected issues during CI runs.🔎 Suggested validation
' "$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/Packages" \ >"$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/openwisp/Packages.sha256.checksum" + +# Verify that checksums were generated +if [ ! -s "$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/openwisp/Packages.sha256.checksum" ]; then + echo "Error: No openwisp packages found in Packages file" + exit 1 +fi
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
runbuild
⏰ 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). (1)
- GitHub Check: Build and Upload Packages
🔇 Additional comments (1)
runbuild (1)
61-61: Remove theSIGNED_PACKAGES=variable; setCONFIG_SIGNED_PACKAGESin.configinstead.The
SIGNED_PACKAGES=make variable does not override theCONFIG_SIGNED_PACKAGESKconfig symbol used by OpenWrt's build system. To generate unsigned package indexes, you must disable signing in the.configfile before invokingmake package/index, either by:
- Setting
# CONFIG_SIGNED_PACKAGES is not setin.config, or- Running
make menuconfigand disabling "Signed packages"The current syntax will not prevent Packages.sig generation if
CONFIG_SIGNED_PACKAGESis enabled.Likely an incorrect or invalid review comment.
- Build tools/install if usign binary is not present in staging_dir/host/bin - Only builds tools when usign is actually missing (efficient check) - Fixes 'usign: No such file or directory' error in CI - Handles both CI_CACHE and non-cache scenarios The error occurred because make package/index requires the usign binary to exist even when SIGNED_PACKAGES= is set to skip signing. This is due to OpenWRT's SHA-512 padding workaround in the Makefile that runs unconditionally. Using tools/install instead of tools/usign/compile ensures all tool dependencies are properly built and avoids 'No rule to make target' errors.
shubhamk0205
force-pushed
the
issues/149-upload-package-checksums
branch
from
a848b45 to
3c69722
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
runbuild (1)
65-72: Consider adding defensive checks for the Packages file and filtered output.The awk filtering logic is correct—it captures entire package stanzas from "Package: openwisp-" until the blank line separator. However, consider adding checks to fail early with clear error messages:
- Verify the source Packages file exists before filtering
- Verify the filtered output is non-empty (at least one openwisp package found)
🔎 Proposed defensive checks
# Filter Packages file to include only openwisp packages and save as checksum file +PACKAGES_FILE="$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/Packages" +if [ ! -f "$PACKAGES_FILE" ]; then + echo "ERROR: Packages file not found at $PACKAGES_FILE" + exit 1 +fi + mkdir -p "$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/openwisp" +CHECKSUM_FILE="$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/openwisp/Packages.sha256.checksum" + awk ' /^Package: openwisp-/ {flag=1} flag {print} /^$/ {flag=0} -' "$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/Packages" \ - >"$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/openwisp/Packages.sha256.checksum" +' "$PACKAGES_FILE" > "$CHECKSUM_FILE" + +if [ ! -s "$CHECKSUM_FILE" ]; then + echo "WARNING: No openwisp packages found in Packages file" + exit 1 +fi
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
runbuild
⏰ 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). (1)
- GitHub Check: QA-Checks and Tests
🔇 Additional comments (3)
runbuild (3)
62-63: LGTM! Package index generation command is correct.The
make package/index SIGNED_PACKAGES=command correctly generates an unsigned package index. The emptySIGNED_PACKAGES=value explicitly requests unsigned index generation, which aligns with the PR objectives.
74-77: LGTM! Move and symlink operations are correct.The operations correctly move the openwisp package directory (containing the checksum file) to the versioned directory and update the latest symlink to point to it. The
|| trueon line 76 safely handles the case where the latest symlink doesn't exist yet.
55-60: Remove the usign build (lines 55–60)—the "SHA-512 padding workaround" claim contradicts both line 62 and OpenWrt documentation.The comment on line 56 claims usign is required for a "SHA-512 padding workaround" even for unsigned package indices, but this contradicts the comment on line 62 which explicitly states "(unsigned - no usign needed)". OpenWrt documentation confirms that unsigned package index generation (with
SIGNED_PACKAGES=) does not require usign; usign is only needed if you want to sign the index with a detached.sigfile.Since the index is generated unsigned and the comment on line 62 acknowledges no usign is needed, lines 55–60 waste build time. Either remove this unnecessary tool build, or clarify in the code comments why usign is actually required for your use case.
Likely an incorrect or invalid review comment.
|
I like the approach, I think this is the right way to do it, but I am not sure why unsign is not available and can't get around to finding the time to test this as there's many other more urgent PRs open waiting to be merged. I hope somebody can help us with this. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
@shubhamk0205 Overall aproach is good , LGTM. But there are some changes that can help the pr progress I have mentioned them Have a look and tell me what do you think
| # Ensure usign tool is available (required for package index generation) | ||
| # Even when generating unsigned indexes, OpenWRT's Makefile needs usign for SHA-512 padding workaround | ||
| if [ ! -f staging_dir/host/bin/usign ]; then | ||
| echo "usign not found, building tools..." | ||
| make -j"$CORES" tools/install || make -j1 V=s tools/install | ||
| fi | ||
|
|
||
| # Generate package index with checksums (unsigned - no usign needed) | ||
| make package/index SIGNED_PACKAGES= V=s | ||
|
|
||
| # Filter Packages file to include only openwisp packages and save as checksum file | ||
| mkdir -p "$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/openwisp" | ||
| awk ' | ||
| /^Package: openwisp-/ {flag=1} | ||
| flag {print} | ||
| /^$/ {flag=0} | ||
| ' "$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/Packages" \ | ||
| >"$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/openwisp/Packages.sha256.checksum" |
There was a problem hiding this comment.
The entire usign block and SIGNED_PACKAGES= can be removed once
the CONFIG_SIGNED_PACKAGES fix (that was mentioned in the next one comment by me) is applied.
Also adding defensive checks as suggested by CodeRabbit:
| # Ensure usign tool is available (required for package index generation) | |
| # Even when generating unsigned indexes, OpenWRT's Makefile needs usign for SHA-512 padding workaround | |
| if [ ! -f staging_dir/host/bin/usign ]; then | |
| echo "usign not found, building tools..." | |
| make -j"$CORES" tools/install || make -j1 V=s tools/install | |
| fi | |
| # Generate package index with checksums (unsigned - no usign needed) | |
| make package/index SIGNED_PACKAGES= V=s | |
| # Filter Packages file to include only openwisp packages and save as checksum file | |
| mkdir -p "$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/openwisp" | |
| awk ' | |
| /^Package: openwisp-/ {flag=1} | |
| flag {print} | |
| /^$/ {flag=0} | |
| ' "$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/Packages" \ | |
| >"$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/openwisp/Packages.sha256.checksum" | |
| # Generate package index with checksums (unsigned) | |
| make package/index V=s | |
| # Filter Packages file to include only openwisp packages | |
| PACKAGES_FILE="$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/Packages" | |
| if [ ! -f "$PACKAGES_FILE" ]; then | |
| echo "ERROR: Packages file not found at $PACKAGES_FILE" | |
| exit 1 | |
| fi | |
| mkdir -p "$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/openwisp" | |
| CHECKSUM_FILE="$BUILD_DIR/openwrt/bin/packages/$COMPILE_TARGET/openwisp/Packages.sha256.checksum" | |
| awk ' | |
| /^Package: openwisp-/ {flag=1} | |
| flag {print} | |
| /^$/ {flag=0} | |
| ' "$PACKAGES_FILE" > "$CHECKSUM_FILE" | |
| if [ ! -s "$CHECKSUM_FILE" ]; then | |
| echo "ERROR: No openwisp packages found in Packages file" | |
| exit 1 | |
| fi |
| sed -i '/routing/d' feeds.conf | ||
| ./scripts/feeds update -a | ||
| ./scripts/feeds install -a | ||
| echo "CONFIG_PACKAGE_openwisp-config=y" >>.config |
There was a problem hiding this comment.
Have a look at this , SIGNED_PACKAGES= on line 63 does not disable signing "make defconfig" sets the CONFIG_SIGNED_PACKAGES=y ,and the openwrt modules checks that value however that is not being changed by your changes . So signing is always triggered regardless of the runbuild changes.What i want to suggest is try using this
| echo "CONFIG_PACKAGE_openwisp-config=y" >>.config | |
| echo "# CONFIG_SIGNED_PACKAGES is not set" >> .config |
Sure , let me see and get back to you |
|
Hey @nemesifier , I went through the existing PR (#242) and the review comments. I noticed that the main issue seems to be around package signing not being properly disabled ( I’d like to try fixing this by:
If this approach sounds good, I can work on updating the PR or creating a clean fix. Let me know your thoughts. |
nemesifier
added
the
github_actions
nemesifier
force-pushed
the
issues/149-upload-package-checksums
branch
from
d07945a to
9eb5f7b
Compare
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 `@runbuild`:
- Around line 68-69: The two rm invocations using the PACKAGES_DIR variable (rm
"$PACKAGES_DIR"/*.json and rm "$PACKAGES_DIR"/Packages*) can fail when globs are
empty under set -e; change them to use rm -f (e.g., rm -f "$PACKAGES_DIR"/*.json
and rm -f "$PACKAGES_DIR"/Packages*) so missing matches are ignored and the
script won't abort; keep existing quoting of "$PACKAGES_DIR" intact.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d12df23b-0945-4580-a2a7-d7238e2bbf1c
📒 Files selected for processing (1)
runbuild
📜 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). (1)
- GitHub Check: QA-Checks and Tests
🔇 Additional comments (4)
runbuild (4)
7-7: LGTM!Also applies to: 15-16, 21-21
48-51: LGTM!
59-67: LGTM!
71-75: LGTM!
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I disabled signing as in openwisp/openwrt-openwisp-monitoring#187 and applied the same changes done to openwisp/openwrt-openwisp-monitoring#188.
4 participants
Checklist
Reference to Existing Issue
Closes #149
Description of Changes
Implements package checksum publishing to enable package verification.
Changes:
make package/indexin runbuild to generatePackagesfile with SHA256 checksumsawkPackages.sha256.checksumfollowing OpenWRT's standard formatResult:
Users can now verify downloaded packages using the published SHA256 checksums.
Note: No tests added as this is a build script change only.
Screenshot
N/A