macOS: consolidate the duplicated portable-find-executable detection - #3950
Draft
moflwi wants to merge 1 commit into
Draft
macOS: consolidate the duplicated portable-find-executable detection#3950moflwi wants to merge 1 commit into
moflwi wants to merge 1 commit into
Conversation
agent.sh's _otel_find_type_executable and api.sh's _otel_find_executables each independently branched on uname -s Darwin to pick -perm +111 (BSD find) vs -executable (GNU find) — same fact, duplicated, with the two definitions drifting apart in shape as each was fixed in its own branch. Moves the OS check to a single _otel_find_executable_test variable set once in api.sh alongside the existing _otel_shell_home/_otel_shell_bin_home Darwin checks. Both helpers now just splice it into their find invocation. api.sh is unconditionally sourced before agent.sh uses the variable, so ordering is not a concern. Verified under dash (the two branch-specific find commands' actual argv, extracted from the real function bodies) that the unquoted variable word-splits into separate find arguments as intended, and that -perm +111 returns real results on this machine's system find matching the un-refactored behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
|
Hello, thanks for contributing for the first time! |
Open
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR consolidates the platform-specific find “is executable” predicate so macOS (BSD find) vs Linux (GNU find) handling is defined once and reused by both api.sh and agent.sh.
Changes:
- Introduces a shared
_otel_find_executable_testpredicate chosen byuname -sinapi.sh. - Refactors
_otel_find_executablesinapi.shto use the shared predicate. - Refactors
_otel_find_type_executableinagent.shto use the shared predicate.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/usr/share/opentelemetry_shell/api.sh | Defines _otel_find_executable_test once and uses it in _otel_find_executables to avoid duplicated Darwin branching. |
| src/usr/share/opentelemetry_shell/agent.sh | Switches _otel_find_type_executable to use _otel_find_executable_test from api.sh to eliminate duplicated OS checks. |
Suppressed comments (1)
src/usr/share/opentelemetry_shell/api.sh:1
uname -sis now executed three times during initialization. Consider caching once (e.g.,_otel_uname_s=$(uname -s)) and reusing it for the subsequent conditionals to reduce process forks and ensure consistent evaluation.
#!/bin/false
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # $_otel_find_executable_test (set by api.sh) is -executable on Linux and the portable -perm +111 | ||
| # on Darwin, where BSD find errors out on -executable; that error would otherwise be silently | ||
| # swallowed by the redirect below and leave nothing to instrument | ||
| _otel_find_type_executable() { \find "$1" -maxdepth 1 -type "$2" $_otel_find_executable_test 2> /dev/null || \true; } |
| } | ||
| fi | ||
| _otel_find_executables() { | ||
| \find "$1" $_otel_find_executable_test -iname "$2" |
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
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.
Summary
`agent.sh`'s `_otel_find_type_executable` and `api.sh`'s `_otel_find_executables` each independently branched on `uname -s` Darwin to pick `-perm +111` (BSD find) vs `-executable` (GNU find) — same fact, duplicated, with the two definitions having drifted into slightly different shapes since each was fixed independently.
Moves the OS check to a single `_otel_find_executable_test` variable set once in `api.sh`. Both helpers now just splice it into their `find` invocation. Verified under `dash` that the unquoted-variable word-splitting works as intended (a zsh test run first gave a false negative, since zsh doesn't word-split unquoted variables by default).
Depends on #3947 (base branch, docker mount path fix).
Test plan
🤖 Generated with Claude Code