Add rsync - #568
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis change adds reproducible package definitions and build scripts for rsync 3.4.4 and xxHash 0.8.3. Both packages verify sources, declare dependencies and outputs, record provenance, and install into staged output directories. ChangesRsync package
xxHash package
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/rsync/build.ncl (1)
5-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlphabetize the lowercase import bindings.
Order these bindings as
base,gcc,glibc,make,openssl,toolchain, andxxhash.As per coding guidelines, lowercase imported identifiers must be alphabetized.
Proposed change
let base = import "../base/build.ncl" in let gcc = import "../gcc/build.ncl" in +let glibc = import "../glibc/build.ncl" in let make = import "../make/build.ncl" in -let toolchain = import "../toolchain/build.ncl" in -let glibc = import "../glibc/build.ncl" in -let xxhash = import "../xxhash/build.ncl" in let openssl = import "../openssl/build.ncl" in +let toolchain = import "../toolchain/build.ncl" in +let xxhash = import "../xxhash/build.ncl" in🤖 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 `@packages/rsync/build.ncl` around lines 5 - 11, Reorder the import bindings in the rsync build definition alphabetically by their lowercase identifiers: base, gcc, glibc, make, openssl, toolchain, and xxhash.Source: Coding guidelines
packages/xxhash/build.sh (1)
1-2: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse the POSIX shell convention for package builds.
The repository convention is
#!/bin/shwithset -eorset -eu. This script uses/bin/bashandpipefail, but it has no Bash-only syntax. The package specification does not declare Bash as a build dependency. Use the POSIX form and retain the unset-variable guard.Proposed fix
-#!/bin/bash -set -euo pipefail +#!/bin/sh +set -eu🤖 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 `@packages/xxhash/build.sh` around lines 1 - 2, Update the build script’s shebang to /bin/sh and replace the Bash-specific option combination with the POSIX-compatible set -eu form, preserving both exit-on-error and unset-variable protection.Sources: Coding guidelines, Learnings
🤖 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 `@packages/rsync/build.sh`:
- Around line 11-15: Update the configure.sh patching sed rule to delete the
complete here-document range beginning with the cat >>$CONFIG_STATUS <<_ACEOF
marker and ending at the closing _ACEOF line. Replace the current single-line
N-based matching so configured blocks containing content are removed as well.
In `@packages/xxhash/build.sh`:
- Around line 9-11: Update the make invocation in the build script to pass
ARFLAGS=rcsD explicitly, ensuring libxxhash.a retains its symbol index while
using deterministic archive timestamps; remove or override the ineffective
exported ARFLAGS setting as needed.
- Line 11: Update the staging command in build.sh to validate that OUTPUT_DIR is
non-empty before invoking make, then quote the OUTPUT_DIR expansion and the
nproc job-count substitution so empty, whitespace-containing, or glob-containing
paths cannot alter the install destination.
- Around line 5-8: Update the build environment assignments in the xxhash build
script to preserve the toolchain-selected CC rather than forcing gcc, and append
the existing reproducibility options to the current CFLAGS and LDFLAGS values so
target and sysroot flags remain intact; keep CXXFLAGS derived from the resulting
CFLAGS, following the pattern used by the stress-ng build script.
---
Nitpick comments:
In `@packages/rsync/build.ncl`:
- Around line 5-11: Reorder the import bindings in the rsync build definition
alphabetically by their lowercase identifiers: base, gcc, glibc, make, openssl,
toolchain, and xxhash.
In `@packages/xxhash/build.sh`:
- Around line 1-2: Update the build script’s shebang to /bin/sh and replace the
Bash-specific option combination with the POSIX-compatible set -eu form,
preserving both exit-on-error and unset-variable protection.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ff8f883-c7c9-401a-8dc8-fe05f0769913
📒 Files selected for processing (4)
packages/rsync/build.nclpackages/rsync/build.shpackages/xxhash/build.nclpackages/xxhash/build.sh
| export ARFLAGS=Drc | ||
|
|
||
| make -j$(nproc) DESTDIR=$OUTPUT_DIR PREFIX=/usr install |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
curl -fsSL 'https://raw.githubusercontent.com/Cyan4973/xxHash/v0.8.3/Makefile' |
grep -n -A3 '^libxxhash\.a:'Repository: gominimal/pkgs
Length of output: 276
🏁 Script executed:
#!/bin/sh
set -eu
printf '--- packages/xxhash/build.sh ---\n'
cat -n packages/xxhash/build.sh
printf '\n--- make documentation for ARFLAGS override ---\n'
make --version 2>/dev/null | head -n 1 || true
make --help 2>/dev/null | sed -n '/override/,+3p' || true
printf '\n--- Make behavior probe with Makefile from raw v0.8.3 ---\n'
tmp="$(mktemp -d)"
cd "$tmp"
printf 'AR = echo-ar\nlibxxhash.a: ARFLAGS = rcs\n\t$(AR) $(ARFLAGS) $@\n\n' > Makefile
export ARFLAGS=Drc
printf '-- with explicit ARFLAGS on make command line --\n'
make -f Makefile ARFLAGS=rcsD
printf '-- without explicit ARFLAGS on make command line --\n'
make -f MakefileRepository: gominimal/pkgs
Length of output: 243
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- packages/xxhash/build.sh ---'
cat -n packages/xxhash/build.sh
printf '%s\n' ''
printf '%s\n' '--- make documentation for override/directives ---'
make --version 2>/dev/null | head -n 1 || true
make --help 2>/dev/null | sed -n '/override/,+4p' || true
printf '%s\n' ''
printf '%s\n' '--- Make behavior probe with Makefile from raw v0.8.3 ---'
tmp="$(mktemp -d)"
printf '%s\n' 'AR = echo-ar' > "$tmp/Makefile"
printf '%s\n' 'libxxhash.a: ARFLAGS = rcs' >> "$tmp/Makefile"
printf '%s\n' ' $(AR) $(ARFLAGS) $@' >> "$tmp/Makefile"
printf '%s\n' '' >> "$tmp/Makefile"
printf '%s\n' '-- exported ARFLAGS only --'
ARFLAGS=Drc make -f "$tmp/Makefile"
printf '%s\n' '-- explicit ARFLAGS passed to make command line --'
make -f "$tmp/Makefile" ARFLAGS=rcsD
printf '%s\n' '-- patch the target --'
sed -i 's/^libxxhash\.a: ARFLAGS = rcs/libxxhash.a: ARFLAGS = rcsD/' "$tmp/Makefile"
ARFLAGS=Drc make -f "$tmp/Makefile"Repository: gominimal/pkgs
Length of output: 715
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- upstream targets around archive and install ---'
curl -fsSL 'https://raw.githubusercontent.com/Cyan4973/xxHash/v0.8.3/Makefile' | sed -n '135,185p'
printf '%s\n' ''
printf '%s\n' '--- deterministic archive behavior for GNU ar flags ---'
python3 - <<'PY'
flags = "rcsD"
print("libxxhash.a target ARFLAGS rcs assignment will replace exported ARFLAGS=Drc in GNU make")
print("rcsD preserves the index and adds the --deterministic flag for GNU ar")
print("Drc only would override rcs and replace the current order with deterministic+create+replace, dropping index")
PYRepository: gominimal/pkgs
Length of output: 1834
Preserve the static archive index while enforcing deterministic archives.
The xxHash v0.8.3 libxxhash.a target assigns ARFLAGS = rcs, so the exported ARFLAGS=Drc is replaced and the archive index is dropped. Pass ARFLAGS=rcsD on the make command line, or patch the archive target, so libxxhash.a keeps its symbol index and uses a deterministic archive.
Proposed fix
-export ARFLAGS=Drc
+export ARFLAGS=rcsD
-make -j$(nproc) DESTDIR=$OUTPUT_DIR PREFIX=/usr install
+make -j"$(nproc)" ARFLAGS="$ARFLAGS" DESTDIR="$OUTPUT_DIR" PREFIX=/usr 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.
| export ARFLAGS=Drc | |
| make -j$(nproc) DESTDIR=$OUTPUT_DIR PREFIX=/usr install | |
| export ARFLAGS=rcsD | |
| make -j"$(nproc)" ARFLAGS="$ARFLAGS" DESTDIR="$OUTPUT_DIR" PREFIX=/usr 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 `@packages/xxhash/build.sh` around lines 9 - 11, Update the make invocation in
the build script to pass ARFLAGS=rcsD explicitly, ensuring libxxhash.a retains
its symbol index while using deterministic archive timestamps; remove or
override the ineffective exported ARFLAGS setting as needed.
Source: Coding guidelines
| export LDFLAGS="-Wl,--build-id=none" | ||
| export ARFLAGS=Drc | ||
|
|
||
| make -j$(nproc) DESTDIR=$OUTPUT_DIR PREFIX=/usr install |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching build.sh:"
git ls-files 'packages/*/build.sh' | sed -n '1,80p'
echo
echo "packages/xxhash/build.sh:"
if [ -f packages/xxhash/build.sh ]; then
cat -n packages/xxhash/build.sh
else
echo "missing packages/xxhash/build.sh"
fi
echo
echo "Build script occurrences of OUTPUT_DIR / DESTDIR:"
rg -n 'OUTPUT_DIR|DESTDIR|PREFIX|make -j|nproc' packages/*/build.sh packages/xxhash/build.sh || trueRepository: gominimal/pkgs
Length of output: 50370
🌐 Web query:
xxHash v0.8.3 Makefile DESTDIR install install_bin install_lib install_inc install_man
💡 Result:
In the xxHash v0.8.3 Makefile, the DESTDIR variable is supported and acts as a standard prefix for installation paths, enabling staged installations (e.g., for package building) [1][2]. The project provides modular install targets, which allow users to selectively install components [3]. The primary install target invokes these individual targets sequentially [4][5]. Key installation targets include: - install_libxxhash.a: Installs the static library (libxxhash.a) to the LIBDIR [4][1]. - install_libxxhash: Installs the shared library (libxxhash.so or equivalent) and its symbolic links to the LIBDIR [4][1]. - install_libxxhash.includes: Installs the header files (xxhash.h, xxh3.h, etc.) to the INCLUDEDIR [4][1]. - install_xxhsum: Installs the xxhsum CLI utility and creates symbolic links for associated commands (xxh32sum, xxh64sum, xxh128sum, xxh3sum) in the BINDIR [4][1]. - install_man: Installs the man pages to the MANDIR [4][1]. - install_libxxhash.pc: Installs the pkg-config file to the PKGCONFIGDIR [4][5]. These targets rely on directory variables such as BINDIR, LIBDIR, INCLUDEDIR, and MANDIR, which default to standard locations (e.g., /usr/local/bin) but can be overridden [1]. Directories are created as needed during the install process using the MAKE_DIR variable [4][6]. To install the entire suite, you can run: make install To install specific components, you can call their respective targets directly, for example: make install_xxhsum DESTDIR=/tmp/staging
Citations:
- 1: https://github.com/Cyan4973/xxHash/blob/dev/Makefile
- 2: make install fails when INSTALL_DIR is defined Cyan4973/xxHash#911
- 3: Makefile: add dedicated install targets Cyan4973/xxHash#665
- 4: https://fossies.org/linux/misc/xxHash-0.8.3.tar.gz/xxHash-0.8.3/Makefile
- 5: https://fossies.org/linux/xxHash/Makefile
- 6: Cyan4973/xxHash@e626a72
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import subprocess
import tempfile
def exec_sh(script, env=None):
if env is None:
env = {"PATH": subprocess.os.environ.get("PATH", "/usr/bin:/bin"), "HOME": subprocess.os.path.expanduser("~")}
p = subprocess.Popen(['sh', '-e'], stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env, text=True)
stdout, stderr = p.communicate(script, timeout=10)
return p.returncode, stdout.splitlines(), stderr.splitlines()
cases = [
("unset", {}),
("empty", {"OUTPUT_DIR": ""}),
("space", {"OUTPUT_DIR": "/tmp/staging space"}),
("glob", {"OUTPUT_DIR": "/tmp/staging-[a-z]"}),
]
for name, env in cases:
# Minimal shell script that mirrors the script options and the make command argument construction.
# It uses printf -v so the exact unquoted substitution can be inspected without starting make.
script = """\
set -euo pipefail
export CC=gcc
export CFLAGS="-O3 -pipe -ffile-prefix-map=$(pwd)=/builddir -gno-record-gcc-switches"
export CXXFLAGS="$CFLAGS"
export LDFLAGS="-Wl,--build-id=none"
export ARFLAGS=Drc
printf -v cmd 'make -j%%s DESTDIR=%s PREFIX=/usr install' "$(nproc)" "%s"
printf '%s\n' "$cmd"
exit 0
"""
code, stdout, stderr = exec_sh(script, env)
print(f"\nCASE: {name}")
if code != 0:
print("exit", code)
for line in stderr:
print(line)
else:
print("\n".join(stdout))
print("\nBash shellcheck behavior for double-quote glob expansion would be read-only, but no shellcheck availability check needed.")
PYRepository: gominimal/pkgs
Length of output: 642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check bash printf -v unquoted substitution around spaces/globs:"
bash -c '
set -euo pipefail
prefix=/usr
dest=""
printf -v cmd "make -j%s DESTDIR=%s PREFIX=/usr install" "$(nproc)" "$dest"
printf "cmd=%s\n" "$cmd"
if [ "$dest" = "" ]; then
printf "empty case: DESTDIR becomes empty\n"
fi
'
bash -c '
set -euo pipefail
prefix=/usr
dest="/tmp/staging space"
printf -v cmd "make -j%s DESTDIR=%s PREFIX=/usr install" "$(nproc)" "$dest"
printf "cmd=%s\n" "$cmd"
if printf "%s" "$cmd" | grep -qF "DESTDIR=/tmp/staging space"; then
printf "space case: quoted expansion preserves space\n"
fi
'Repository: gominimal/pkgs
Length of output: 473
Validate and quote $OUTPUT_DIR before staging.
set -euo pipefail does not reject an empty OUTPUT_DIR; DESTDIR=PREFIX=/usr then leaves DESTDIR unset and installs under PREFIX=/usr. Unquoted expansion also breaks paths containing spaces or glob characters. Fail closed and quote both $OUTPUT_DIR and $(nproc).
Proposed fix
+: "${OUTPUT_DIR:?OUTPUT_DIR must be non-empty}"
-make -j$(nproc) DESTDIR=$OUTPUT_DIR PREFIX=/usr install
+make -j"$(nproc)" DESTDIR="$OUTPUT_DIR" PREFIX=/usr 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.
| make -j$(nproc) DESTDIR=$OUTPUT_DIR PREFIX=/usr install | |
| : "${OUTPUT_DIR:?OUTPUT_DIR must be non-empty}" | |
| make -j"$(nproc)" DESTDIR="$OUTPUT_DIR" PREFIX=/usr 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 `@packages/xxhash/build.sh` at line 11, Update the staging command in build.sh
to validate that OUTPUT_DIR is non-empty before invoking make, then quote the
OUTPUT_DIR expansion and the nproc job-count substitution so empty,
whitespace-containing, or glob-containing paths cannot alter the install
destination.
Source: Coding guidelines
|
/build |
|
[package: rsync] enumerate bins amd64/linux
[package: rsync] missing runtime_deps amd64/linux
[package: xxhash] enumerate bins amd64/linux
|
|
/build |
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@twitchyliquid64 how are you getting to these errors? my min check doesn't reveal these. I suppose there is something to build them in place? |
|
If you are in a minimal session which is a checkout of pkgs with your changes, there should be equivalent |
xxx@dsktp:~/pkgs$ git remote add irishi https://github.com/lrishi/gominimal-pkgs.git
xxx@dsktp:~/pkgs$ git fetch irishi && git checkout irishi/add_rsync
<snip>
HEAD is now at 29ac230 Enumerate bins in xxhash
xxx@dsktp:~/pkgs$ mip package build rsync xxhash
2026-08-05T16:19:29.752366Z INFO op::specs: Building package: xxhash
2026-08-05T16:19:32.153830Z INFO op::specs: Building package: rsync
2026-08-05T16:19:40.979630Z INFO mip::cmd_pkg: Build completed successfully!
2026-08-05T16:19:40.979644Z INFO mip::cmd_pkg: Target packages:
2026-08-05T16:19:40.979700Z INFO mip::cmd_pkg: rsync -> /home/xxx/.cache/minimal/built/98/a182d5b9a6625000fd5984d8e5d0fa884b07b9100ca8ba1a9246b8f11bb2ee (built in 8.7s)
2026-08-05T16:19:40.979739Z INFO mip::cmd_pkg: xxhash -> /home/xxx/.cache/minimal/built/3e/8afc9e114ac9152e2278582aa7264f10dc9a9ac6272299dd19ce63fce5c8a1 (built in 1.2s)
xxx@dsktp:~/pkgs$ mip check xxhash rsync
package: xxhash
spec name matches dir...Pass
spec name valid...Pass
cycle breaker naming...Pass
output naming...Pass
enumerate bins...Pass
output types valid...Pass
missing runtime_deps...Pass
build scripts are executable...Pass
build script disallowed-patterns...Pass
standalone tests...Pass
source urls valid...Pass
parse...Pass
import line...Pass
imports...Pass
fmt...Pass
package: rsync
spec name matches dir...Pass
spec name valid...Pass
cycle breaker naming...Pass
output naming...Pass
enumerate bins...Pass
output types valid...Pass
missing runtime_deps...Pass
build scripts are executable...Pass
build script disallowed-patterns...Pass
standalone tests...Pass
source urls valid...Pass
parse...Pass
import line...Pass
imports...Pass
fmt...Pass
xxx@dsktp:~/pkgs$ |
|
/build |
Summary
Add rsync to packages. It's dependency xxhash is also added
Related issues
Changes
Checklist
min checkpasses for the affected packages/harnesses.min patched-build <name>succeeds for any package I added or modified.source_provenancepoints to the canonical upstream and the source builds from source (not a prebuilt release binary) where the required toolchain is available.sha256against the upstream archive.Notes for reviewers
Summary by CodeRabbit
New Features
Build Improvements