Initial experiments with /opt root for hab#10160
Initial experiments with /opt root for hab#10160agadgil-progress wants to merge 1 commit intomainfrom
/opt root for hab#10160Conversation
Signed-off-by: Abhijit Gadgil <agadgil@progress.com>
✅ Deploy Preview for chef-habitat ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR introduces experimental support for using /opt as the root filesystem path for Habitat on macOS. The changes force macOS to use /opt instead of / or the FS_ROOT environment variable, addressing potential mounting issues on macOS.
Changes:
- Forces FS_ROOT_PATH to
/opton macOS in the core filesystem module - Modifies binlink path handling to accommodate the macOS-specific root path
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| components/core/src/fs.rs | Adds macOS-specific logic to force FS_ROOT_PATH to "/opt" instead of respecting the FS_ROOT environment variable |
| components/hab/src/command/pkg/binlink.rs | Adds macOS-specific path handling for both destination and source paths in the binlink function |
| // For MacOS we just force it to "/opt" as other values can potentially not be | ||
| // mountable. | ||
| PathBuf::from("/opt") |
There was a problem hiding this comment.
Forcing FS_ROOT_PATH to '/opt' on macOS completely ignores the FS_ROOT_ENVVAR environment variable, which breaks the documented behavior for testing and creating self-contained root filesystems. This is inconsistent with Windows and Linux where the environment variable is respected. Consider allowing FS_ROOT_ENVVAR to override the default on macOS, or add a comment explaining why this limitation is necessary for the experimental feature.
| // For MacOS we just force it to "/opt" as other values can potentially not be | |
| // mountable. | |
| PathBuf::from("/opt") | |
| // On macOS, default to "/opt" for mountability reasons, but still allow | |
| // FS_ROOT_ENVVAR to override this default for testing and self-contained | |
| // root filesystems. | |
| match henv::var(FS_ROOT_ENVVAR) { | |
| Ok(root) => PathBuf::from(root), | |
| Err(_) => PathBuf::from("/opt"), | |
| } |
| let dst_path = if cfg!(target_os = "macos") { | ||
| dest_path | ||
| } else { | ||
| &fs_root_path.join(dest_path.strip_prefix("/")?) | ||
| }; |
There was a problem hiding this comment.
When cfg!(target_os = 'macos') is true, dst_path is set to dest_path directly without joining with fs_root_path. However, at line 150, the src path is joined with fs_root_path for macOS. This creates an inconsistency where binlinks will be created in the system directory (e.g., /usr/bin) rather than within the fs_root_path (e.g., /opt/hab/...). This could cause permission errors or unintended system modification. Either both paths should respect fs_root_path, or the logic needs clarification about why destination and source are treated differently.
| let dst_path = if cfg!(target_os = "macos") { | |
| dest_path | |
| } else { | |
| &fs_root_path.join(dest_path.strip_prefix("/")?) | |
| }; | |
| let dst_path = &fs_root_path.join(dest_path.strip_prefix("/")?); |
| if cfg!(target_os = "windows") { | ||
| src = fs_root_path.join(src.strip_prefix("/")?); | ||
| } | ||
| if cfg!(target_os = "macos") { | ||
| src = fs_root_path.join(src.strip_prefix("/")?); | ||
| } |
There was a problem hiding this comment.
This macOS-specific code duplicates the Windows logic from lines 146-148. Consider consolidating these platform-specific path adjustments into a single conditional block or extracting them into a helper function to reduce code duplication. For example: 'if cfg!(any(target_os = "windows", target_os = "macos")) { src = fs_root_path.join(src.strip_prefix("/")?); }'.
| if cfg!(target_os = "windows") { | |
| src = fs_root_path.join(src.strip_prefix("/")?); | |
| } | |
| if cfg!(target_os = "macos") { | |
| src = fs_root_path.join(src.strip_prefix("/")?); | |
| } | |
| if cfg!(any(target_os = "windows", target_os = "macos")) { | |
| src = fs_root_path.join(src.strip_prefix("/")?); | |
| } |
1 participant
No description provided.