fix: resolve executable path when run via PATH#359
fix: resolve executable path when run via PATH#359tsurumi-yizhou wants to merge 2 commits intoclice-io:mainfrom
Conversation
📝 WalkthroughWalkthroughAdded a cross-platform executable search (search_in_path) and updated resolve_self_path to use it when argv[0] lacks path separators; when argv[0] contains a separator it is resolved as an absolute/weakly-canonical path. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Proc as Process (argv[0])
participant Env as Environment (PATH, PATHEXT)
participant FS as Filesystem
Note over Proc: resolve_self_path(argc, argv)
Proc->>Proc: check argv[0] for path separator
alt contains separator
Proc->>FS: compute absolute path
FS-->>Proc: absolute path
Proc->>FS: weakly_canonical(path)
FS-->>Proc: canonical path or error
else no separator
Proc->>Env: query PATH (+PATHEXT on Windows)
Env->>FS: test candidate paths (exec bit / exists)
FS-->>Env: found canonical path or not
Env-->>Proc: return located path or original name
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 (2)
src/clice.cc (2)
12-12: Unused include can be removed.Since the
eventide::serde::config::set_field_rename_policycall is commented out (lines 222-223), this include appears to be unused. Consider removing it until the migration is complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/clice.cc` at line 12, Remove the unused include "eventide/serde/serde/config.h" from src/clice.cc (the include at the top referencing eventide::serde::config) since the only call that would use it, eventide::serde::config::set_field_rename_policy, is currently commented out; alternatively, if you intend to keep that functionality, uncomment and properly use eventide::serde::config::set_field_rename_policy where it was disabled (around the commented lines 222-223) so the include is actually required.
221-223: Consider tracking the serde migration in an issue.Leaving a TODO comment without a linked issue makes it easy to forget. If the eventide serde API migration is non-trivial, consider creating an issue to track it.
Additionally, verify if this rename policy affects any other serde usage in the codebase. Based on the
Optionsstruct inserver/runtime.h, thebuild_options()function manually maps CLI options without serde deserialization, so this may have limited impact—but other code paths might rely on it.Would you like me to open an issue to track the eventide serde API migration?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/clice.cc` around lines 221 - 223, Replace the bare TODO in main() that references the eventide serde migration with a tracked issue: open an issue describing the required API migration for eventide::serde::config::set_field_rename_policy (include needed behavior and any impacts), then update the comment in src/clice.cc main() to reference that issue ID/URL; while creating the issue, search the codebase for uses of eventide::serde and set_field_rename_policy and verify whether server/runtime.h::Options and build_options() or any other serde deserialization paths will be affected, listing affected files in the issue so the migration can be scheduled and implemented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/clice.cc`:
- Around line 90-137: The Windows branch of search_in_path should honor PATHEXT:
read the PATHEXT env var (or use fallback {".exe",".cmd",".bat",".com"}), split
it by ';', and when constructing full_path inside the loop over PATH directories
try the provided name first and then each PATHEXT extension (skip adding an
extension if name already has one) before calling std::filesystem::canonical and
is_executable; update the Windows is_executable lambda to behave like the POSIX
one if necessary and return the first canonical.string() that passes the checks
in search_in_path.
---
Nitpick comments:
In `@src/clice.cc`:
- Line 12: Remove the unused include "eventide/serde/serde/config.h" from
src/clice.cc (the include at the top referencing eventide::serde::config) since
the only call that would use it,
eventide::serde::config::set_field_rename_policy, is currently commented out;
alternatively, if you intend to keep that functionality, uncomment and properly
use eventide::serde::config::set_field_rename_policy where it was disabled
(around the commented lines 222-223) so the include is actually required.
- Around line 221-223: Replace the bare TODO in main() that references the
eventide serde migration with a tracked issue: open an issue describing the
required API migration for eventide::serde::config::set_field_rename_policy
(include needed behavior and any impacts), then update the comment in
src/clice.cc main() to reference that issue ID/URL; while creating the issue,
search the codebase for uses of eventide::serde and set_field_rename_policy and
verify whether server/runtime.h::Options and build_options() or any other serde
deserialization paths will be affected, listing affected files in the issue so
the migration can be scheduled and implemented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6cb5cf7-dad4-478e-89e2-a51c89baf977
📒 Files selected for processing (1)
src/clice.cc
When clice is executed via PATH (e.g., 'clice' instead of '/path/to/clice'), resolve_self_path() incorrectly resolves the path relative to current directory instead of the actual executable location. This causes worker pool to fail with 'no such file or directory' error. Add search_in_path() function to search PATH environment variable when argv[0] contains no path separators. Fixes: worker pool fails to start when clice is in PATH
tsurumi-yizhou
force-pushed
the
fix-resolve-self-path
branch
from
bef643f to
027bb55
Compare
- Read PATHEXT environment variable or use default extensions - Try original name first, then each extension if name has no extension - Update Windows is_executable to use filesystem::status like POSIX
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/clice.cc`:
- Around line 91-101: The file uses std::getenv in clice.cc (see usage around
path_env and pathext_env) but doesn't include <cstdlib>; add an `#include`
<cstdlib> to the top of the translation unit so std::getenv is defined portably
(keep existing includes and place <cstdlib> with the other system headers).
- Around line 144-180: The PATH loop currently skips empty PATH components
(if(!dir.empty())) which ignores the POSIX meaning of empty entries as the
current directory and does not explicitly search the CWD on Windows; update the
loop handling around path_view/path_sep so that when dir is empty you treat it
as the current directory (use "." on POSIX) or, on Windows, explicitly insert
std::filesystem::current_path() into the search before iterating PATH entries;
ensure this logic is applied where full_path is constructed
(std::filesystem::path(dir) / name) and that the existing canonical +
is_executable checks (and the PATHEXT extension loop using extensions and
has_extension) are reused for these inserted current-directory checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa9ba543-078b-46ee-a6ff-f574243c5866
📒 Files selected for processing (1)
src/clice.cc
| const char* path_env = std::getenv("PATH"); | ||
| if(!path_env || name.empty()) { | ||
| return std::string(name); | ||
| } | ||
|
|
||
| #ifdef _WIN32 | ||
| constexpr char path_sep = ';'; | ||
|
|
||
| // Get PATHEXT or use default extensions | ||
| std::vector<std::string> extensions; | ||
| const char* pathext_env = std::getenv("PATHEXT"); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In standard C++, which header declares std::getenv, and is using std::getenvwithout including guaranteed to be portable?
💡 Result:
- Header: In standard C++,
std::getenvis declared in<cstdlib>. [1][2] - Portability: No. Calling
std::getenvwithout including<cstdlib>is not guaranteed portable—it may “work” only because some other header (or your implementation) happens to include/declare it, but the standard doesn’t require that. You should include<cstdlib>yourself. [2][3]
Sources:
[1] cppreference: <cstdlib> header contents (lists getenv) (en.cppreference.com)
[2] cppreference: C++ headers / requirements about what headers provide (en.cppreference.com)
[3] Stack Overflow discussion noting non-portability of relying on indirect declarations (stackoverflow.com)
Citations:
- 1: https://en.cppreference.com/w/cpp/header/cstdlib.html?utm_source=openai
- 2: https://en.cppreference.com/w/cpp/headers.html?utm_source=openai
- 3: https://stackoverflow.com/questions/29551755/why-getenv-can-get-name-resolved-without-a-std?utm_source=openai
Add <cstdlib> for std::getenv portability.
The code uses std::getenv without including <cstdlib>, which is not guaranteed to be portable across standard-compliant implementations.
Suggested fix
+#include <cstdlib>
`#include` <expected>
`#include` <filesystem>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/clice.cc` around lines 91 - 101, The file uses std::getenv in clice.cc
(see usage around path_env and pathext_env) but doesn't include <cstdlib>; add
an `#include` <cstdlib> to the top of the translation unit so std::getenv is
defined portably (keep existing includes and place <cstdlib> with the other
system headers).
| while(start < path_view.size()) { | ||
| size_t end = path_view.find(path_sep, start); | ||
| if(end == std::string_view::npos) { | ||
| end = path_view.size(); | ||
| } | ||
|
|
||
| std::string_view dir = path_view.substr(start, end - start); | ||
| if(!dir.empty()) { | ||
| #ifdef _WIN32 | ||
| // Try the name as-is first | ||
| std::filesystem::path full_path = std::filesystem::path(dir) / name; | ||
| std::error_code ec; | ||
| auto canonical = std::filesystem::canonical(full_path, ec); | ||
| if(!ec && is_executable(canonical)) { | ||
| return canonical.string(); | ||
| } | ||
|
|
||
| // If name doesn't have an extension, try each PATHEXT extension | ||
| if(!has_extension) { | ||
| for(const auto& ext : extensions) { | ||
| std::string name_with_ext = std::string(name) + ext; | ||
| full_path = std::filesystem::path(dir) / name_with_ext; | ||
| canonical = std::filesystem::canonical(full_path, ec); | ||
| if(!ec && is_executable(canonical)) { | ||
| return canonical.string(); | ||
| } | ||
| } | ||
| } | ||
| #else | ||
| std::filesystem::path full_path = std::filesystem::path(dir) / name; | ||
| std::error_code ec; | ||
| auto canonical = std::filesystem::canonical(full_path, ec); | ||
| if(!ec && is_executable(canonical)) { | ||
| return canonical.string(); | ||
| } | ||
| #endif | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
When resolving a bare command name, does Windows search the current directory before PATH, and on POSIX does an empty PATH component represent the current directory?
💡 Result:
-
Windows (Win32 process creation / “bare” executable name): Usually yes. When Windows performs an executable search (e.g., for a relative name), it searches the current working directory before the system PATH when
SafeProcessSearchModeis 0 (the documented default). IfSafeProcessSearchModeis set to 1 (or changed per-process), it searches the system PATH first and the current directory later. [1] -
POSIX: Yes. In
PATH, a zero-length (empty) component means the current working directory (written as::, a leading:, or a trailing:). POSIX notes this is a “legacy feature”; strictly conforming applications should use.explicitly. [2]
Sources: [1] SearchPathA (Microsoft Learn) • [2] POSIX environment variable PATH (The Open Group)
🏁 Script executed:
# Check if the function has any current directory handling
cd src && grep -n "current_path\|current_directory\|\"\.\"\|dir\.empty()" clice.cc | head -30Repository: clice-io/clice
Length of output: 88
🏁 Script executed:
# Find the search_in_path function and display it with context
cd src && grep -n "search_in_path" clice.cc | head -5Repository: clice-io/clice
Length of output: 157
🏁 Script executed:
# Display the complete search_in_path function (approximately lines 90-210)
cd src && sed -n '90,210p' clice.ccRepository: clice-io/clice
Length of output: 4406
Ensure current directory is checked as part of executable search.
The loop skips empty PATH entries (if(!dir.empty())), but on POSIX, an empty PATH component represents the current directory per POSIX specification and should be searched. On Windows, while the operating system searches the current directory before PATH by default, this behavior is not guaranteed—it depends on the SafeProcessSearchMode registry setting. The code should explicitly check the current directory to ensure consistent, predictable behavior across platforms. Treat empty PATH entries as "." on POSIX, and prepend the current directory (via std::filesystem::current_path()) on Windows before searching the PATH.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/clice.cc` around lines 144 - 180, The PATH loop currently skips empty
PATH components (if(!dir.empty())) which ignores the POSIX meaning of empty
entries as the current directory and does not explicitly search the CWD on
Windows; update the loop handling around path_view/path_sep so that when dir is
empty you treat it as the current directory (use "." on POSIX) or, on Windows,
explicitly insert std::filesystem::current_path() into the search before
iterating PATH entries; ensure this logic is applied where full_path is
constructed (std::filesystem::path(dir) / name) and that the existing canonical
+ is_executable checks (and the PATHEXT extension loop using extensions and
has_extension) are reused for these inserted current-directory checks.
1 participant
When clice is executed via PATH (e.g., 'clice' instead of '/path/to/clice'), resolve_self_path() incorrectly resolves the path relative to current directory instead of the actual executable location. This causes worker pool to fail with 'no such file or directory' error.
Add search_in_path() function to search PATH environment variable when argv[0] contains no path separators.
Fixes: worker pool fails to start when clice is in PATH #358
Summary by CodeRabbit
Refactor
Chores