packaging (Debian and tarball) and multiarch path fixups (Debian)#2656
Merged
Conversation
On FreeBSD 14.3 (vm06), ldconfig '-?' reports ... usage: ldconfig [-32] [-BRimr] [-f hints_file][directory | file ...] which needs a smarter grep to find the -m option.
Only for developer use to overlay "sudo make install" on top of a PCP installation. Needed some extra tweaks to handle path rewriting for multiarch on Debian ... package installs and make install were not putting the libpcp<foo>.so.N file in the same directory.
1. mv rather than cp, so we don't end up with turds in /usr/lib that are
not part of any PCP package
2. add -d (debug flag) to report key variable assignments
3. expand coverage to include pkgconfig files that are also not where
"sudo make install" puts them
Note: all the 9 files are hard links to the same script, so realy only
one set of changes here.
Revert parts of commit 10f01ad. Conditionally installing based on $DEB_HOST_MULTIARCH was wrong. Install into $PCP_LIB_DIR (e.g. /usr/lib) and let the Debian build ecosystem deal with the path renaming to install libraries below $PCP_LIB_DIR/$DEB_HOST_MULTIARCH.
The Debian packaging ecosystem sees the zbxpcp.so DSO's below /usr/lib and wants to move 'em to the correct multipath directory. Make the pathnames glob-match the renamed paths, same as was done for all the libpcp<foo> libraries.
Correct usr/share/pcp/htop entries were already present.
Needs to refer to libpcp-import1-dev.
- /run/pmimport installed via tmpfiles, not packaged - pcp-dstat lintian changes
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes update Debian packaging manifests, standardize pkg-config installation paths, add a shared developer overlay installation script with symlinked entry points, and broaden post-install ChangesInstallation and packaging
Sequence Diagram(s)sequenceDiagram
participant Developer
participant install-dev
participant make
participant pkg-config
participant Filesystem
Developer->>install-dev: Run developer overlay install
install-dev->>make: Build and install library
make-->>install-dev: Return versioned shared object
install-dev->>pkg-config: Query libdir
pkg-config-->>install-dev: Return target directory
install-dev->>Filesystem: Reconcile library and pkg-config artifacts
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/libpcp_gui/src/install-dev (1)
1-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIdentical script duplicated per library. These four
install-devscripts (and likely the others in the same cohort) are byte-for-byte identical aside from the cwd-derived$iam; the copy-pasted error-text bug above is direct evidence of the drift risk this creates.
src/libpcp_gui/src/install-dev#L1-L94: extract shared logic into one script (e.g. undersrc/include/) parameterized by library name, sourced/invoked from eachsrc/dir.src/libpcp_import/src/install-dev#L1-L94: same consolidation.src/libpcp_mmv/src/install-dev#L1-L94: same consolidation.src/libpcp_pmda/src/install-dev#L1-L94: same consolidation.🤖 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 `@src/libpcp_gui/src/install-dev` around lines 1 - 94, Consolidate the duplicated install-dev logic into one shared parameterized script, deriving or accepting the library name through the existing iam flow and preserving the current build, install, pkg-config, relocation, and debug behavior. Update src/libpcp_gui/src/install-dev (lines 1-94), src/libpcp_import/src/install-dev (lines 1-94), src/libpcp_mmv/src/install-dev (lines 1-94), and src/libpcp_pmda/src/install-dev (lines 1-94) to invoke the shared implementation rather than duplicate it; place the shared script under src/include/ as suggested.
🤖 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 `@build/tar/postinstall.tail`:
- Line 528: Update the ldconfig capability probe to use a token-aware regular
expression that matches only a standalone short `-m` option, excluding long
options such as `--format`. Preserve the existing probe and merge-branch
behavior while preventing unsupported `$LDCONFIG -m` invocation on Linux.
In `@src/libpcp_gui/src/install-dev`:
- Around line 21-25: The `/etc/pcp.env` checks report the wrong missing-file
path. Update the error message in `src/libpcp_gui/src/install-dev` lines 21-25,
`src/libpcp_import/src/install-dev` lines 21-25,
`src/libpcp_mmv/src/install-dev` lines 21-25, and
`src/libpcp_pmda/src/install-dev` lines 21-25 to identify `/etc/pcp.env`.
- Around line 63-91: Ensure the sudo rm and sudo mv operations in the
install-dev file handling loop fail the script when unsuccessful by adding
explicit nonzero-exit handling. Apply this change at the corresponding lines
63-91 in src/libpcp_gui/src/install-dev, src/libpcp_import/src/install-dev,
src/libpcp_mmv/src/install-dev, and src/libpcp_pmda/src/install-dev; both the
duplicate-file removal branch and updated-file move branch must propagate
command failures instead of allowing a successful exit.
In `@src/libpcp/src/install-dev`:
- Around line 63-91: Check the exit status of both sudo rm -f and sudo mv in the
artifact-reconciliation loop, exiting with an “Arrgh” error if either operation
fails. Apply this change in src/libpcp/src/install-dev,
src/libpcp_archive/src/install-dev, src/libpcp_fault/src/install-dev,
src/libpcp_trace/src/install-dev, and src/libpcp_web/src/install-dev within the
reconciliation branches, preserving the existing success messages and behavior.
- Around line 21-27: Update the /etc/pcp.env existence-check error message in
the install-dev scripts at src/libpcp/src/install-dev lines 21-27,
src/libpcp_archive/src/install-dev lines 21-27, src/libpcp_fault/src/install-dev
lines 21-27, src/libpcp_trace/src/install-dev lines 21-27, and
src/libpcp_web/src/install-dev lines 21-27 to identify /etc/pcp.env as the
missing file.
- Around line 1-94: Replace the duplicated install-dev bodies with one shared
script, preserving the existing dynamic iam derivation and installation
behavior. Update src/libpcp/src/install-dev lines 1-94 to delegate to the shared
script, and make the same delegation change in
src/libpcp_archive/src/install-dev lines 1-94, src/libpcp_fault/src/install-dev
lines 1-94, src/libpcp_trace/src/install-dev lines 1-94, and
src/libpcp_web/src/install-dev lines 1-94; use a thin wrapper or symlink that
executes or sources the common implementation.
---
Nitpick comments:
In `@src/libpcp_gui/src/install-dev`:
- Around line 1-94: Consolidate the duplicated install-dev logic into one shared
parameterized script, deriving or accepting the library name through the
existing iam flow and preserving the current build, install, pkg-config,
relocation, and debug behavior. Update src/libpcp_gui/src/install-dev (lines
1-94), src/libpcp_import/src/install-dev (lines 1-94),
src/libpcp_mmv/src/install-dev (lines 1-94), and src/libpcp_pmda/src/install-dev
(lines 1-94) to invoke the shared implementation rather than duplicate it; place
the shared script under src/include/ as suggested.
🪄 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), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 27f764a4-d690-4bd6-89e2-3587d5d92236
📒 Files selected for processing (20)
build/tar/postinstall.taildebian/control.pcpdebian/libpcp-import2.installdebian/pcp-dstat.installdebian/pcp-dstat.lintiandebian/pcp-export-zabbix-agent.installdebian/pcp-htop.installsrc/libpcp/src/GNUmakefilesrc/libpcp/src/install-devsrc/libpcp_archive/src/GNUmakefilesrc/libpcp_archive/src/install-devsrc/libpcp_fault/src/install-devsrc/libpcp_gui/src/install-devsrc/libpcp_import/src/GNUmakefilesrc/libpcp_import/src/install-devsrc/libpcp_mmv/src/install-devsrc/libpcp_pmda/src/GNUmakefilesrc/libpcp_pmda/src/install-devsrc/libpcp_trace/src/install-devsrc/libpcp_web/src/install-dev
💤 Files with no reviewable changes (5)
- src/libpcp_archive/src/GNUmakefile
- src/libpcp/src/GNUmakefile
- src/libpcp_pmda/src/GNUmakefile
- src/libpcp_import/src/GNUmakefile
- debian/pcp-htop.install
Comment on lines
+21
to
+25
| if [ ! -f /etc/pcp.env ] | ||
| then | ||
| echo "Arrgh: ../../../src/include/pcp.env missing" | ||
| exit 1 | ||
| fi |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wrong error text for the /etc/pcp.env check, copy-pasted across all four scripts. All four scripts check for /etc/pcp.env but print "Arrgh: ../../../src/include/pcp.env missing", copy-pasted from the earlier pcp.conf check without updating the path.
src/libpcp_gui/src/install-dev#L21-L25: change the message to"Arrgh: /etc/pcp.env missing".src/libpcp_import/src/install-dev#L21-L25: same fix.src/libpcp_mmv/src/install-dev#L21-L25: same fix.src/libpcp_pmda/src/install-dev#L21-L25: same fix.
📍 Affects 4 files
src/libpcp_gui/src/install-dev#L21-L25(this comment)src/libpcp_import/src/install-dev#L21-L25src/libpcp_mmv/src/install-dev#L21-L25src/libpcp_pmda/src/install-dev#L21-L25
🤖 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 `@src/libpcp_gui/src/install-dev` around lines 21 - 25, The `/etc/pcp.env`
checks report the wrong missing-file path. Update the error message in
`src/libpcp_gui/src/install-dev` lines 21-25,
`src/libpcp_import/src/install-dev` lines 21-25,
`src/libpcp_mmv/src/install-dev` lines 21-25, and
`src/libpcp_pmda/src/install-dev` lines 21-25 to identify `/etc/pcp.env`.
Comment on lines
+63
to
+91
| if [ "$pkg_dir" != "$PCP_LIB_DIR" ] | ||
| then | ||
| $debug && echo "Debug: moving $iam files from $PCP_LIB_DIR to $pkg_dir" | ||
| for file in ${iam}.so ${iam}.so.$dso_ver ${iam}.a pkgconfig/${iam}.pc | ||
| do | ||
| if [ -f "$PCP_LIB_DIR/$file" ] | ||
| then | ||
| if [ -f "$pkg_dir/$file" ] | ||
| then | ||
| # both exist | ||
| if cmp "$PCP_LIB_DIR/$file" "$pkg_dir/$file" >/dev/null | ||
| then | ||
| echo "Warning: removing $PCP_LIB_DIR/$file (already the same as $pkg_dir/$file)" | ||
| sudo rm -f "$PCP_LIB_DIR/$file" | ||
| else | ||
| echo "Info: updated $pkg_dir/$file with $PCP_LIB_DIR/$file" | ||
| sudo mv "$PCP_LIB_DIR/$file" "$pkg_dir/$file" | ||
| fi | ||
| else | ||
| echo "Arrgh: $PCP_LIB_DIR/$file present, but no $pkg_dir/$file" | ||
| exit 1 | ||
| fi | ||
| else | ||
| echo "Warning: $PCP_LIB_DIR/$file: not found" | ||
| fi | ||
| done | ||
| else | ||
| $debug && echo "Debug: nothing else to do" | ||
| fi |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in src/libpcp_gui/src/install-dev src/libpcp_import/src/install-dev src/libpcp_mmv/src/install-dev src/libpcp_pmda/src/install-dev; do
echo "===== $f ====="
wc -l "$f"
sed -n '1,180p' "$f" | nl -ba | sed -n '1,180p'
echo
doneRepository: performancecopilot/pcp
Length of output: 277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find whether these scripts run under a shell with errexit/pipefail or create target dirs elsewhere.
rg -n --hidden --glob '!*node_modules*' --glob '!*.git*' \
'pkgconfig|install-dev|mkdir -p|set -e|set -u|set -o errexit|sudo mv|sudo rm|\.so\.[0-9]+' \
src/libpcp_gui/src/install-dev src/libpcp_import/src/install-dev src/libpcp_mmv/src/install-dev src/libpcp_pmda/src/install-dev \
| sed -n '1,220p'Repository: performancecopilot/pcp
Length of output: 1476
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in src/libpcp_gui/src/install-dev src/libpcp_import/src/install-dev src/libpcp_mmv/src/install-dev src/libpcp_pmda/src/install-dev; do
echo "===== $f ====="
awk 'NR>=1 && NR<=180 {print NR ":" $0}' "$f" | sed -n '1,180p'
echo
doneRepository: performancecopilot/pcp
Length of output: 9380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "dso_ver|dso version|SONAME|pkgconfig" src/libpcp_gui src/libpcp_import src/libpcp_mmv src/libpcp_pmda | sed -n '1,220p'Repository: performancecopilot/pcp
Length of output: 1492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in src/libpcp_gui/src/GNUmakefile src/libpcp_import/src/GNUmakefile src/libpcp_mmv/src/GNUmakefile src/libpcp_pmda/src/GNUmakefile; do
echo "===== $f ====="
sed -n '1,220p' "$f" | awk '{print NR ":" $0}' | sed -n '1,220p'
echo
doneRepository: performancecopilot/pcp
Length of output: 10164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'pkgconfig|mkdir -p|install -d|SONAME|\.so\.[0-9]+|\.so\$|\.so\.' \
src/libpcp_gui src/libpcp_import src/libpcp_mmv src/libpcp_pmda \
| sed -n '1,260p'Repository: performancecopilot/pcp
Length of output: 1740
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
paths = [
Path("src/libpcp_gui/src/GNUmakefile"),
Path("src/libpcp_import/src/GNUmakefile"),
Path("src/libpcp_mmv/src/GNUmakefile"),
Path("src/libpcp_pmda/src/GNUmakefile"),
]
for p in paths:
print(f"===== {p} =====")
text = p.read_text().splitlines()
for i, line in enumerate(text, 1):
if any(k in line for k in ("pkgconfig", "install", ".so.", "SONAME", "LIBCONFIG", "PCP_LIB_DIR")):
print(f"{i}:{line}")
print()
PYRepository: performancecopilot/pcp
Length of output: 2002
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in src/libpcp_gui/src/GNUmakefile src/libpcp_import/src/GNUmakefile src/libpcp_mmv/src/GNUmakefile src/libpcp_pmda/src/GNUmakefile; do
echo "===== $f ====="
wc -l "$f"
sed -n '1,220p' "$f" | nl -ba | sed -n '1,220p'
echo
doneRepository: performancecopilot/pcp
Length of output: 277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --glob '!*.git*' 'soname|version-info|shared|SHLIB|LDFLAGS|CFLAGS|\.so\.[0-9]+' src/libpcp_gui src/libpcp_import src/libpcp_mmv src/libpcp_pmda | sed -n '1,260p'Repository: performancecopilot/pcp
Length of output: 710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in src/libpcp_gui/src/GNUmakefile src/libpcp_import/src/GNUmakefile src/libpcp_mmv/src/GNUmakefile src/libpcp_pmda/src/GNUmakefile; do
echo "===== $f ====="
rg -n -C 4 '\.so|install.*pkgconfig|install.*LIBCONFIG|soname|version-info|shared' "$f"
echo
doneRepository: performancecopilot/pcp
Length of output: 203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in src/libpcp_gui/src/GNUmakefile src/libpcp_import/src/GNUmakefile src/libpcp_mmv/src/GNUmakefile src/libpcp_pmda/src/GNUmakefile; do
echo "===== $f ====="
rg -n -C 3 'ln -s|install -m|\.so\.|\.a|LIBCONFIG|pkgconfig' "$f"
echo
doneRepository: performancecopilot/pcp
Length of output: 2742
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'PCP_LIB_DIR.*/pkgconfig|mkdir -p .*pkgconfig|install -d .*pkgconfig|pkgconfig/.*mkdir|INSTALL.*pkgconfig' \
src src/libpcp_gui src/libpcp_import src/libpcp_mmv src/libpcp_pmda \
| sed -n '1,260p'Repository: performancecopilot/pcp
Length of output: 793
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'define .*INSTALL|INSTALL =|INSTALL\)|mkdir -p|install -d|pkgconfig' src/include src/include/builddefs src/include/*.mk src/include/* 2>/dev/null | sed -n '1,260p'Repository: performancecopilot/pcp
Length of output: 5376
Check sudo mv/sudo rm failures. In src/libpcp_gui/src/install-dev, src/libpcp_import/src/install-dev, src/libpcp_mmv/src/install-dev, and src/libpcp_pmda/src/install-dev, a failed move/remove can be ignored and the script still exits 0. Add explicit failure handling (|| exit 1 or similar) around those commands.
📍 Affects 4 files
src/libpcp_gui/src/install-dev#L63-L91(this comment)src/libpcp_import/src/install-dev#L63-L91src/libpcp_mmv/src/install-dev#L63-L91src/libpcp_pmda/src/install-dev#L63-L91
🤖 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 `@src/libpcp_gui/src/install-dev` around lines 63 - 91, Ensure the sudo rm and
sudo mv operations in the install-dev file handling loop fail the script when
unsuccessful by adding explicit nonzero-exit handling. Apply this change at the
corresponding lines 63-91 in src/libpcp_gui/src/install-dev,
src/libpcp_import/src/install-dev, src/libpcp_mmv/src/install-dev, and
src/libpcp_pmda/src/install-dev; both the duplicate-file removal branch and
updated-file move branch must propagate command failures instead of allowing a
successful exit.
Comment on lines
+1
to
+94
| #!/bin/sh | ||
| # | ||
| # Developer overlay install for libpcp<foo> | ||
| # | ||
| # Usage: install-dev [-d] | ||
| # | ||
|
|
||
| debug=false | ||
| [ $# -eq 1 -a "X$1" = X-d ] && debug=true | ||
|
|
||
| # need to get the pcp.conf used in the build, not the | ||
| # installed one | ||
| # | ||
| if [ ! -f ../../../src/include/pcp.conf ] | ||
| then | ||
| echo "Arrgh: ../../../src/include/pcp.conf missing" | ||
| exit 1 | ||
| fi | ||
| PCP_CONF=../../../src/include/pcp.conf | ||
|
|
||
| if [ ! -f /etc/pcp.env ] | ||
| then | ||
| echo "Arrgh: ../../../src/include/pcp.env missing" | ||
| exit 1 | ||
| fi | ||
|
|
||
| . /etc/pcp.env | ||
|
|
||
| iam="`pwd | sed -e 's;/src$;;' -e's;.*/\(.*\);\1;'`" | ||
|
|
||
| if ! make | ||
| then | ||
| echo "Arrgh: make failed" | ||
| exit 1 | ||
| fi | ||
|
|
||
| case "`echo $iam.so.*`" | ||
| in | ||
| *\ *) | ||
| echo "Arrgh: more than one .so.*: `echo $iam.so.*`" | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| dso_ver=`echo "$iam".so.* | sed -e 's/.*\.so\.//'` | ||
|
|
||
| if ! sudo make install | ||
| then | ||
| echo "Arrgh: sudo make install failed" | ||
| exit 1 | ||
| fi | ||
|
|
||
| $debug && echo "Debug: iam=$iam dso_ver=$dso_ver" | ||
| $debug && echo "Debug: PCP build libdir: $PCP_LIB_DIR" | ||
|
|
||
| pkg_dir="`pkg-config --variable=libdir $iam 2>/dev/null`" | ||
| $debug && echo "Debug: pkg-config libdir: $pkg_dir" | ||
| if [ -z "$pkg_dir" ] | ||
| then | ||
| echo "Arrgh: no libdir from pkg-config" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [ "$pkg_dir" != "$PCP_LIB_DIR" ] | ||
| then | ||
| $debug && echo "Debug: moving $iam files from $PCP_LIB_DIR to $pkg_dir" | ||
| for file in ${iam}.so ${iam}.so.$dso_ver ${iam}.a pkgconfig/${iam}.pc | ||
| do | ||
| if [ -f "$PCP_LIB_DIR/$file" ] | ||
| then | ||
| if [ -f "$pkg_dir/$file" ] | ||
| then | ||
| # both exist | ||
| if cmp "$PCP_LIB_DIR/$file" "$pkg_dir/$file" >/dev/null | ||
| then | ||
| echo "Warning: removing $PCP_LIB_DIR/$file (already the same as $pkg_dir/$file)" | ||
| sudo rm -f "$PCP_LIB_DIR/$file" | ||
| else | ||
| echo "Info: updated $pkg_dir/$file with $PCP_LIB_DIR/$file" | ||
| sudo mv "$PCP_LIB_DIR/$file" "$pkg_dir/$file" | ||
| fi | ||
| else | ||
| echo "Arrgh: $PCP_LIB_DIR/$file present, but no $pkg_dir/$file" | ||
| exit 1 | ||
| fi | ||
| else | ||
| echo "Warning: $PCP_LIB_DIR/$file: not found" | ||
| fi | ||
| done | ||
| else | ||
| $debug && echo "Debug: nothing else to do" | ||
| fi | ||
|
|
||
| exit 0 | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fully duplicated install-dev script across libraries. All 5 scripts are byte-identical 94-line copies (and the PR stack indicates this extends to libpcp_gui, libpcp_mmv, libpcp_pmda, libpcp_import too). Because iam is already derived dynamically from pwd, the script needs no per-library customization, so this is a pure copy/paste — the two bugs above demonstrate the maintenance cost of fixing the same logic in N places.
src/libpcp/src/install-dev#L1-L94: extract the shared body into a common script (e.g. undersrc/include/) and have this file become a thin wrapper/symlink that sources or execs it.src/libpcp_archive/src/install-dev#L1-L94: same, delegate to the shared script.src/libpcp_fault/src/install-dev#L1-L94: same, delegate to the shared script.src/libpcp_trace/src/install-dev#L1-L94: same, delegate to the shared script.src/libpcp_web/src/install-dev#L1-L94: same, delegate to the shared script.
📍 Affects 5 files
src/libpcp/src/install-dev#L1-L94(this comment)src/libpcp_archive/src/install-dev#L1-L94src/libpcp_fault/src/install-dev#L1-L94src/libpcp_trace/src/install-dev#L1-L94src/libpcp_web/src/install-dev#L1-L94
🤖 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 `@src/libpcp/src/install-dev` around lines 1 - 94, Replace the duplicated
install-dev bodies with one shared script, preserving the existing dynamic iam
derivation and installation behavior. Update src/libpcp/src/install-dev lines
1-94 to delegate to the shared script, and make the same delegation change in
src/libpcp_archive/src/install-dev lines 1-94, src/libpcp_fault/src/install-dev
lines 1-94, src/libpcp_trace/src/install-dev lines 1-94, and
src/libpcp_web/src/install-dev lines 1-94; use a thin wrapper or symlink that
executes or sources the common implementation.
Comment on lines
+21
to
+27
| if [ ! -f /etc/pcp.env ] | ||
| then | ||
| echo "Arrgh: ../../../src/include/pcp.env missing" | ||
| exit 1 | ||
| fi | ||
|
|
||
| . /etc/pcp.env |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wrong error message copy-pasted across all 5 install-dev scripts. Each script's /etc/pcp.env existence check reuses the error text from the pcp.conf check above it, so the message names the wrong file/path when this check actually fails.
src/libpcp/src/install-dev#L21-L27: change the message to"Arrgh: /etc/pcp.env missing".src/libpcp_archive/src/install-dev#L21-L27: same fix.src/libpcp_fault/src/install-dev#L21-L27: same fix.src/libpcp_trace/src/install-dev#L21-L27: same fix.src/libpcp_web/src/install-dev#L21-L27: same fix.
📍 Affects 5 files
src/libpcp/src/install-dev#L21-L27(this comment)src/libpcp_archive/src/install-dev#L21-L27src/libpcp_fault/src/install-dev#L21-L27src/libpcp_trace/src/install-dev#L21-L27src/libpcp_web/src/install-dev#L21-L27
🤖 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 `@src/libpcp/src/install-dev` around lines 21 - 27, Update the /etc/pcp.env
existence-check error message in the install-dev scripts at
src/libpcp/src/install-dev lines 21-27, src/libpcp_archive/src/install-dev lines
21-27, src/libpcp_fault/src/install-dev lines 21-27,
src/libpcp_trace/src/install-dev lines 21-27, and src/libpcp_web/src/install-dev
lines 21-27 to identify /etc/pcp.env as the missing file.
Comment on lines
+63
to
+91
| if [ "$pkg_dir" != "$PCP_LIB_DIR" ] | ||
| then | ||
| $debug && echo "Debug: moving $iam files from $PCP_LIB_DIR to $pkg_dir" | ||
| for file in ${iam}.so ${iam}.so.$dso_ver ${iam}.a pkgconfig/${iam}.pc | ||
| do | ||
| if [ -f "$PCP_LIB_DIR/$file" ] | ||
| then | ||
| if [ -f "$pkg_dir/$file" ] | ||
| then | ||
| # both exist | ||
| if cmp "$PCP_LIB_DIR/$file" "$pkg_dir/$file" >/dev/null | ||
| then | ||
| echo "Warning: removing $PCP_LIB_DIR/$file (already the same as $pkg_dir/$file)" | ||
| sudo rm -f "$PCP_LIB_DIR/$file" | ||
| else | ||
| echo "Info: updated $pkg_dir/$file with $PCP_LIB_DIR/$file" | ||
| sudo mv "$PCP_LIB_DIR/$file" "$pkg_dir/$file" | ||
| fi | ||
| else | ||
| echo "Arrgh: $PCP_LIB_DIR/$file present, but no $pkg_dir/$file" | ||
| exit 1 | ||
| fi | ||
| else | ||
| echo "Warning: $PCP_LIB_DIR/$file: not found" | ||
| fi | ||
| done | ||
| else | ||
| $debug && echo "Debug: nothing else to do" | ||
| fi |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unchecked sudo rm/sudo mv exit status across all 5 reconciliation loops. Unlike make and sudo make install, which are checked and abort the script on failure, the sudo rm -f/sudo mv calls inside the artifact-reconciliation branch are not checked, so a failed remove/move still reports success and leaves $PCP_LIB_DIR/$pkg_dir inconsistent.
src/libpcp/src/install-dev#L63-L91: wrap thesudo rm -f/sudo mvcalls (lines 76, 79) inif ! ...; then echo Arrgh...; exit 1; fi.src/libpcp_archive/src/install-dev#L63-L91: same fix.src/libpcp_fault/src/install-dev#L63-L91: same fix.src/libpcp_trace/src/install-dev#L63-L91: same fix.src/libpcp_web/src/install-dev#L63-L91: same fix.
📍 Affects 5 files
src/libpcp/src/install-dev#L63-L91(this comment)src/libpcp_archive/src/install-dev#L63-L91src/libpcp_fault/src/install-dev#L63-L91src/libpcp_trace/src/install-dev#L63-L91src/libpcp_web/src/install-dev#L63-L91
🤖 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 `@src/libpcp/src/install-dev` around lines 63 - 91, Check the exit status of
both sudo rm -f and sudo mv in the artifact-reconciliation loop, exiting with an
“Arrgh” error if either operation fails. Apply this change in
src/libpcp/src/install-dev, src/libpcp_archive/src/install-dev,
src/libpcp_fault/src/install-dev, src/libpcp_trace/src/install-dev, and
src/libpcp_web/src/install-dev within the reconciliation branches, preserving
the existing success messages and behavior.
1. one script and make others symlinks (hard links are not preserved by git apparently) 2. harden the error checking in the script
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.