Add function OS::get_real_path()#1272
Conversation
|
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:
WalkthroughAdds a cross-platform ChangesReal path resolution (cross-platform)
Estimated code review effort: 4 (Complex) | ~40 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant OS
participant OS_Windows
participant WinAPI
Caller->>OS: get_executable_path()
OS->>OS_Windows: get_executable_path()
OS_Windows->>WinAPI: GetModuleFileNameW()
OS_Windows->>OS_Windows: get_real_path(module path)
OS_Windows->>WinAPI: CreateFileW(module path)
OS_Windows->>WinAPI: GetFinalPathNameByHandleW(handle)
WinAPI-->>OS_Windows: normalized final path
OS_Windows-->>OS: resolved executable path
OS-->>Caller: executable path
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@platform/windows/os_windows.cpp`:
- Line 2055: The line declaring the local variable "String result;" is indented
with spaces instead of the repository's tab style; run clang-format (or reformat
that block) so the declaration uses tabs to match surrounding code and coding
style, ensuring the "String result" line indentation matches neighboring lines
in os_windows.cpp.
- Around line 2054-2078: The get_real_path function currently returns an empty
string when CreateFileW or GetFinalPathNameByHandleW fails; initialize result
from p_path at the start of OS_Windows::get_real_path so the original path is
preserved, then only overwrite result after a successful
GetFinalPathNameByHandleW call; ensure the existing logic that strips
"\\\\?\\UNC\\" and "\\\\?\\" is only applied when resolution succeeded, and
leave result (used by get_executable_path) untouched on any failure of
CreateFileW or GetFinalPathNameByHandleW.
- Around line 2062-2065: The prefix checks in OS_Windows::get_real_path are
using negation on result.substr(...) (e.g. !result.substr(0, 8) == ...) which is
wrong; replace those comparisons with proper prefix checks such as
result.begins_with("\\\\?\\UNC\\") and result.begins_with("\\\\?\\") (or use !=
for direct string compare), then perform the same stripping logic (keep the
existing result = String("\\") + result.substr(7) and result = result.substr(4)
statements) so the extended-path prefixes are detected correctly and the
UNC/\\?\ stripping runs only when the prefix is present.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dcc9583e-e7e6-44ee-801b-35679c86675b
📒 Files selected for processing (1)
platform/windows/os_windows.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (2)
drivers/unix/os_unix.cpp (2)
1141-1141: 💤 Low valueRemove redundant cast.
The explicit cast to
(const char *)is unnecessary becausep_path.utf8().get_data()already returnsconst char*.♻️ Simplified version
- if (realpath((const char *)p_path.utf8().get_data(), resolved_path)) { + if (realpath(p_path.utf8().get_data(), resolved_path)) {🤖 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 `@drivers/unix/os_unix.cpp` at line 1141, The call to realpath uses a redundant C-style cast—remove the unnecessary cast so it calls realpath(p_path.utf8().get_data(), resolved_path) directly; update the call site in the code that references realpath, p_path.utf8().get_data(), and resolved_path to pass the already-const char* return value without casting.
1138-1145: 💤 Low valueConsider documenting the behavior for non-existent paths.
The function returns an empty string when
realpath()fails, which includes cases where the path doesn't exist. This silent failure might be intentional, but consider whether callers will be able to distinguish between an error and an empty result.This is consistent with other error-handling patterns in this file (e.g.,
get_environmentreturning empty string on missing vars), but a brief comment documenting the empty-string-on-failure behavior would help future maintainers.🤖 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 `@drivers/unix/os_unix.cpp` around lines 1138 - 1145, Document that OS_Unix::get_real_path returns an empty String when realpath() fails (e.g., non-existent path or other errors) so callers cannot distinguish error vs empty result; add a brief inline comment above or inside the get_real_path implementation stating this empty-string-on-failure behavior and that callers must treat an empty return as a failure to resolve the path.
🤖 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 `@drivers/unix/os_unix.cpp`:
- Line 1141: The call to realpath uses a redundant C-style cast—remove the
unnecessary cast so it calls realpath(p_path.utf8().get_data(), resolved_path)
directly; update the call site in the code that references realpath,
p_path.utf8().get_data(), and resolved_path to pass the already-const char*
return value without casting.
- Around line 1138-1145: Document that OS_Unix::get_real_path returns an empty
String when realpath() fails (e.g., non-existent path or other errors) so
callers cannot distinguish error vs empty result; add a brief inline comment
above or inside the get_real_path implementation stating this
empty-string-on-failure behavior and that callers must treat an empty return as
a failure to resolve the path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b7e8696-7d49-4846-add4-7ce448bf8401
📒 Files selected for processing (2)
drivers/unix/os_unix.cppdrivers/unix/os_unix.h
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 `@core/os/os.h`:
- Line 199: The OS base class implementation has a signature mismatch: the
header declares virtual String get_real_path(const String &p_path) const while
the cpp defines String OS::get_real_path() const and returns the unused member
_realpath; fix by updating the implementation to match the header (define String
OS::get_real_path(const String &p_path) const) and have it return p_path (i.e.,
return the input argument) as the default behavior, and remove or delete the
unused member _realpath (or at minimum mark it unused) so there is no
dead/unused state.
- Line 54: The field _realpath and the no-arg OS::get_real_path() const accessor
are unused; either remove them or ensure _realpath is populated whenever a
real-path is resolved. Fix option A: remove the member _realpath and the no-arg
OS::get_real_path() const to avoid dead state and rely only on
OS::get_real_path(const String &p_path). Fix option B: populate _realpath inside
the real-path resolver(s) — in OS::get_real_path(const String &p_path)
implementations (and any platform overrides) assign the resolved path to
_realpath before returning so the no-arg accessor returns the last-resolved
path; update constructors if needed to initialize _realpath. Use the symbols
_realpath, OS::get_real_path() const, and OS::get_real_path(const String
&p_path) to locate and apply the change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89f033b5-8aa8-47ff-b342-ade336846137
📒 Files selected for processing (2)
core/os/os.cppcore/os/os.h
✅ Files skipped from review due to trivial changes (1)
- core/os/os.cpp
|
@coderabbitai resume |
|
@coderabbitai review |
✅ Actions performedReviews resumed. |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/unix/os_unix.cpp (1)
1147-1154:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCompile error:
return b;references an undefined variable.The resolved path is stored in
s(Line 1149), but the success path returnsb, which is never declared. This will not compile on__linux__builds.🐛 Proposed fix
String s = get_real_path("/proc/self/exe"); if (!FileAccess::exists(s)) { WARN_PRINT("Couldn't get executable path from /proc/self/exe, using argv[0]"); return OS::get_executable_path(); } - return b; + return s;🤖 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 `@drivers/unix/os_unix.cpp` around lines 1147 - 1154, In OS_Unix::get_executable_path() replace the incorrect undefined return value by returning the resolved path variable `s` (the result of get_real_path("/proc/self/exe")) instead of `b`; keep the existing check using FileAccess::exists(s) and the fallback call to OS::get_executable_path() when the proc path is not available so the function returns `s` on success.
🤖 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.
Outside diff comments:
In `@drivers/unix/os_unix.cpp`:
- Around line 1147-1154: In OS_Unix::get_executable_path() replace the incorrect
undefined return value by returning the resolved path variable `s` (the result
of get_real_path("/proc/self/exe")) instead of `b`; keep the existing check
using FileAccess::exists(s) and the fallback call to OS::get_executable_path()
when the proc path is not available so the function returns `s` on success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9146983-ec6b-4bc6-b5ec-68fe87ffb2db
📒 Files selected for processing (4)
core/os/os.cppcore/os/os.hdrivers/unix/os_unix.cppplatform/windows/os_windows.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- core/os/os.cpp
- platform/windows/os_windows.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/unix/os_unix.cpp (1)
1138-1145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix portability: avoid unguarded
PATH_MAXinOS_Unix::get_real_path()
drivers/unix/os_unix.cppincludes<climits>only under__OpenBSD__, butOS_Unix::get_real_path()unconditionally declareschar resolved_path[PATH_MAX];, making non-OpenBSD builds depend on transitivePATH_MAXavailability. Switch to the POSIX.1-2008realpath(..., nullptr)form (and free the returned buffer). The current success-only assignment + fallback top_pathis fine.🛠️ Proposed fix using dynamic allocation
String OS_Unix::get_real_path(const String &p_path) const { String result = p_path; - char resolved_path[PATH_MAX]; - if (realpath(p_path.utf8().get_data(), resolved_path)) { - result = String::utf8(resolved_path); - } + char *resolved_path = realpath(p_path.utf8().get_data(), nullptr); + if (resolved_path) { + result = String::utf8(resolved_path); + free(resolved_path); + } return result; }🤖 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 `@drivers/unix/os_unix.cpp` around lines 1138 - 1145, OS_Unix::get_real_path currently declares a stack buffer char resolved_path[PATH_MAX] which is non-portable; change it to call realpath(p_path.utf8().get_data(), nullptr), check the returned char * (e.g., char *rp = realpath(...)), on success set result = String::utf8(rp) and then free(rp), otherwise leave result as p_path; ensure you include <cstdlib> for free and handle nullptr safely so behavior and fallback remain unchanged.
🤖 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.
Outside diff comments:
In `@drivers/unix/os_unix.cpp`:
- Around line 1138-1145: OS_Unix::get_real_path currently declares a stack
buffer char resolved_path[PATH_MAX] which is non-portable; change it to call
realpath(p_path.utf8().get_data(), nullptr), check the returned char * (e.g.,
char *rp = realpath(...)), on success set result = String::utf8(rp) and then
free(rp), otherwise leave result as p_path; ensure you include <cstdlib> for
free and handle nullptr safely so behavior and fallback remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32d6f6ab-ba97-431b-b18c-cf829bff8e9d
📒 Files selected for processing (1)
drivers/unix/os_unix.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/unix/os_unix.cpp (1)
1149-1153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDistinguish
realpath()failure from a successful Linux resolution.Line 1150 only checks whether the returned path exists, but
get_real_path()falls back to the original input on failure. That means this branch can still return"/proc/self/exe"when normalization failed, and downstream callers will then derive the executable directory from/proc/selfinstead of the actual binary location.Suggested fix
- String s = get_real_path("/proc/self/exe"); - if (!FileAccess::exists(s)) { + static const String self_exe = "/proc/self/exe"; + String s = get_real_path(self_exe); + if (s == self_exe || !FileAccess::exists(s)) { WARN_PRINT("Couldn't get executable path from /proc/self/exe, using argv[0]"); return OS::get_executable_path(); } return s;🤖 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 `@drivers/unix/os_unix.cpp` around lines 1149 - 1153, get_real_path("/proc/self/exe") can return the original input on failure, so change the branch in the block using get_real_path, variable s, FileAccess::exists and OS::get_executable_path to detect a failed normalization: after calling get_real_path("/proc/self/exe"), treat it as a failure if s is empty or if s == "/proc/self/exe" (meaning realpath didn't resolve) and fall back to OS::get_executable_path(); only when s differs from "/proc/self/exe" and FileAccess::exists(s) is true should you accept s as the resolved executable path.
🤖 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.
Outside diff comments:
In `@drivers/unix/os_unix.cpp`:
- Around line 1149-1153: get_real_path("/proc/self/exe") can return the original
input on failure, so change the branch in the block using get_real_path,
variable s, FileAccess::exists and OS::get_executable_path to detect a failed
normalization: after calling get_real_path("/proc/self/exe"), treat it as a
failure if s is empty or if s == "/proc/self/exe" (meaning realpath didn't
resolve) and fall back to OS::get_executable_path(); only when s differs from
"/proc/self/exe" and FileAccess::exists(s) is true should you accept s as the
resolved executable path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 272435ba-cacf-4bbe-8b89-3131bc91537b
📒 Files selected for processing (1)
drivers/unix/os_unix.cpp
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 `@platform/windows/os_windows.cpp`:
- Around line 2087-2099: In OS_Windows::get_environment fix the syntax error by
removing the extra closing parenthesis in the first GetEnvironmentVariableW call
(the call that currently reads
GetEnvironmentVariableW((LPCWSTR)(p_var.utf16().get_data()), nullptr, 0))); so
it compiles; ensure the call to
GetEnvironmentVariableW((LPCWSTR)(p_var.utf16().get_data()), nullptr, 0) uses
the same cast pattern as the subsequent call and preserve the rest of the logic
that allocates val, calls GetEnvironmentVariableW again, assigns res, and
deletes val.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 63ccc978-d5ae-490a-8836-fa2ad7bb4e78
📒 Files selected for processing (1)
platform/windows/os_windows.cpp
|
@samuelvenable Is this intentional? If not then we need to add it. More specifically, the method would need to be bound somewhere, so that it gets added to the class DB. |
|
I was planning on exposing it in a follow up pr. this was mainly added for use with OS::get_executable_path(). |
|
It's a bit harder to test properly without exposing it to scripting. |
sounds good, i'll do it in this pr then, All it does is normalize paths and resolve symbolic links, outside of this pr, get_executable_path didn't resolve symlinks on windows. after these changes, it does. on unix, the former logic with realpath remains untouched, its just extracted into a helper function for convenience. |
3 participants
Summary by CodeRabbit
New Features
OS.get_real_path(path)API (exposed via scripting/bindings) to return normalized “real” paths across platforms.Bug Fixes
Refactor