Supporting a minimal install for local context 3rd party tools#2620
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces two centralized make macros ( ChangesCentralized PMDA Config Generation with local.conf Support
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 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 `@src/include/builddefs.in`:
- Around line 532-545: The grep commands in both PMCDCONF_MAKERULE and
LOCALCONF_MAKERULE (lines 534 and 541) currently use regex-based pattern
matching, which is unsafe if configuration line values ever contain regex
metacharacters. Fix this by adding the -F flag to both grep commands to enable
fixed-string exact-line matching. Specifically, change grep -c to grep -F -c in
the conditional checks that validate whether PMCDCONF_LINE already exists in
../pmcd.conf and whether LOCALCONF_LINE already exists in ../local.conf.
In `@src/pmdas/darwin_proc/GNUmakefile`:
- Line 2: The copyright year range on line 2 of the GNUmakefile contains a typo
where "2025-20926" should be "2025-2026". Correct this typo by updating the
copyright header to use the proper year range format. This is a simple text
correction in the comment header to fix the accidental digit transposition.
🪄 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: cfaec185-09ba-49ef-9cb5-09ea892c3852
📒 Files selected for processing (25)
debian/pcp-conf.installsrc/include/builddefs.insrc/pmdas/.gitignoresrc/pmdas/GNUmakefilesrc/pmdas/aix/GNUmakefilesrc/pmdas/btrfs/GNUmakefilesrc/pmdas/cifs/GNUmakefilesrc/pmdas/darwin/GNUmakefilesrc/pmdas/darwin_proc/GNUmakefilesrc/pmdas/denki/GNUmakefilesrc/pmdas/dm/GNUmakefilesrc/pmdas/etw/GNUmakefilesrc/pmdas/freebsd/GNUmakefilesrc/pmdas/jbd2/GNUmakefilesrc/pmdas/kvm/GNUmakefilesrc/pmdas/linux/GNUmakefilesrc/pmdas/linux_proc/GNUmakefilesrc/pmdas/linux_xfs/GNUmakefilesrc/pmdas/mmv/GNUmakefilesrc/pmdas/netbsd/GNUmakefilesrc/pmdas/openbsd/GNUmakefilesrc/pmdas/pmcd/GNUmakefilesrc/pmdas/root/GNUmakefilesrc/pmdas/solaris/GNUmakefilesrc/pmdas/windows/GNUmakefile
natoscott
force-pushed
the
local-config
branch
2 times, most recently
from
9cdb54b to
6fe2546
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pmdas/GNUmakefile (1)
74-93: 💤 Low valueConsider creating an empty root.local as fallback for robustness.
If no PMDAs produce
root_*files,root.localwon't be created, butinstall_pcp(line 104) unconditionally tries to install it. While this is unlikely in practice given the expected PMDA set, adding a fallback would prevent build failures in minimal or unusual configurations.🛡️ Suggested fallback
> root.local; \ - fi + else \ + echo "root {" > root.local && echo "}" >> root.local; \ + fi🤖 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/pmdas/GNUmakefile` around lines 74 - 93, The root.local file is only created when PMDAs with root_* files are found, but the install_pcp target (line 104) unconditionally attempts to install it, which will fail if no such files exist. Add a fallback mechanism after the existing conditional block that creates an empty root.local file if it wasn't already generated, ensuring the build succeeds in minimal configurations where no PMDAs produce root_* files.
🤖 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.
Nitpick comments:
In `@src/pmdas/GNUmakefile`:
- Around line 74-93: The root.local file is only created when PMDAs with root_*
files are found, but the install_pcp target (line 104) unconditionally attempts
to install it, which will fail if no such files exist. Add a fallback mechanism
after the existing conditional block that creates an empty root.local file if it
wasn't already generated, ensuring the build succeeds in minimal configurations
where no PMDAs produce root_* files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 87a2fe5d-e52f-4475-b914-e516e8a2c04b
📒 Files selected for processing (3)
debian/pcp-conf.installsrc/pmdas/.gitignoresrc/pmdas/GNUmakefile
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pmdas/.gitignore
- debian/pcp-conf.install
Member
Author
This is unnecessary - all platforms provide a kernel and MMV DSO PMDA at the very least. |
Contributor
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 `@src/pmdas/GNUmakefile`:
- Line 104: The install command in the GNUmakefile is incorrectly using the `-t`
flag with a file path instead of a directory. The `-t` flag expects a directory
destination, but $(PCP_PMNS_DIR)/root.local is a file path. Replace this single
install command with separate install commands that install root.local to each
of its two intended destinations: $(PCP_PMNS_DIR)/root.local and
$(PCP_PMNSADM_DIR)/root.local, without using the `-t` flag and instead
specifying the complete destination file paths directly.
🪄 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: 6941bc9e-e6c0-43a2-9c18-a91c5ad4feda
📒 Files selected for processing (3)
debian/pcp-conf.installsrc/pmdas/.gitignoresrc/pmdas/GNUmakefile
✅ Files skipped from review due to trivial changes (1)
- src/pmdas/.gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- debian/pcp-conf.install
natoscott
force-pushed
the
local-config
branch
3 times, most recently
from
8b6ea65 to
8fe087a
Compare
kmcdonell
requested changes
Jun 17, 2026
Arrange for the build to produce /etc/pcp/local.conf for use with pmNewContext(PM_CONTEXT_LOCAL,...) so that PMAPI clients can use DSO PMDAs for sampling using just pcp-libs installed, and no pmcd installed/running if they wish. Related to performancecopilot#2615.
Arrange for the root_* namespace files to be installed via the pcp-conf package, so that pmNewContext(PM_CONTEXT_LOCAL,...) in PMAPI clients can use DSO PMDAs for sampling using just pcp-conf and pcp-libs installed. The final step is to add a "root.local" without cpp-directives (there is no pmcpp(1) binary is available in this minimal install situation). Related to performancecopilot#2615.
During the build, generate a merged PMNS root.local containing the
combined namespace for all DSO PMDAs listed in local.conf. Uses
pmcpp.static to resolve domain macros and awk to merge the separate
root{} blocks from each PMDA's root_* file into a single namespace.
Install root.local to PCP_PMNS_DIR as part of pcp-conf, alongside
the existing per-PMDA root_* files. Clients using PM_CONTEXT_LOCAL
can select this namespace via PMNS_DEFAULT.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
PMDAs contributing root_* files to pcp-conf were installing a real file copy to both $(PMDAADMDIR) and $(PCP_PMNSADM_DIR). Make the $(PCP_PMNSADM_DIR) and $(PCP_PMNS_DIR) entries symlinks back to the canonical copy in $(PMDAADMDIR) instead. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
QA test 1742 exercises PM_CONTEXT_LOCAL using PCP_PMCDCONF_PATH=local.conf and PMNS_DEFAULT=root.local: verifies hinv.ncpu is fetchable, mmv wildcard resolution works, and pmcd.* metrics are absent from the local namespace. Document PCP_PMCDCONF_PATH and PMNS_DEFAULT usage for local context in pcpintro(1), including the recommended pairing of local.conf + root.local. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
natoscott
force-pushed
the
local-config
branch
2 times, most recently
from
6ec5d73 to
2cd0754
Compare
Rename the generated local context PMNS file from root.local to local.root throughout (GNUmakefile, .gitignore, pcp-conf.install, QA test 1742, pcpintro.1). Also fix pcp-conf.install to use usr/lib/pcp/ rather than usr/libexec/pcp/ for the new entries - Debian sets libexecdir=/usr/lib so $(PCP_PMNSADM_DIR) resolves correctly in the makefiles but the static paths in the install manifest must match. Addresses review feedback from Kenj. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Related to #2615