feat: Use ZDOTDIR if set and fallback to HOME#3933
feat: Use ZDOTDIR if set and fallback to HOME#3933ickc wants to merge 5 commits intomamba-org:mainfrom
Conversation
| fs::u8path zshrc_path = home / ".zshrc"; | ||
| fs::u8path zshrc_path | ||
| // use ZDOTDIR if set and fallback to HOME | ||
| const char* zdotdir = std::getenv("ZDOTDIR"); |
There was a problem hiding this comment.
We have util::get_env to properly read from the environment.
| fs::u8path zshrc_path | ||
| // use ZDOTDIR if set and fallback to HOME | ||
| const char* zdotdir = std::getenv("ZDOTDIR"); | ||
| if (zdotdir != nullptr && zdotdir[0] != '\0') | ||
| { | ||
| zshrc_path = fs::u8path(zdotdir) / ".zshrc"; | ||
| } else { | ||
| zshrc_path = home / ".zshrc"; | ||
| } |
There was a problem hiding this comment.
Looks like the repeated logic could be factored into a simple function.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3933 +/- ##
=======================================
Coverage 50.83% 50.84%
=======================================
Files 237 237
Lines 28228 28235 +7
Branches 2919 2920 +1
=======================================
+ Hits 14351 14356 +5
- Misses 13874 13876 +2
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3b7b065 to
0ea3710
Compare
|
@AntoinePrv, do you have any pointers to fix the windows build and also the integration tests on UNIX platforms? |
which return ZDOTDIR if set else HOME
There was a problem hiding this comment.
Pull request overview
This PR adds support for the ZDOTDIR environment variable in zsh shell initialization, allowing users to specify a custom location for zsh configuration files. If ZDOTDIR is not set, the code falls back to the user's home directory, which is the standard zsh behavior.
Changes:
- Added
zsh_home_dir()function to checkZDOTDIRenvironment variable and fall back toHOME - Updated zsh shell initialization code to use
zsh_home_dir()instead ofuser_home_dir() - Added test coverage for the new
zsh_home_dir()function
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| libmamba/include/mamba/util/environment.hpp | Declares zsh_home_dir() function with platform guard for non-Windows systems |
| libmamba/src/util/environment.cpp | Implements zsh_home_dir() to check ZDOTDIR and fallback to user_home_dir() |
| libmamba/src/core/shell_init.cpp | Updates three functions (init_shell, deinit_shell, config_path_for_shell) to use zsh_home_dir() for zsh configuration |
| libmamba/tests/src/util/test_environment.cpp | Adds test cases for zsh_home_dir() covering both ZDOTDIR set and not set scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| else if (shell == "zsh") | ||
| { | ||
| home = util::zsh_home_dir(); | ||
| fs::u8path zshrc_path = home / ".zshrc"; | ||
| modify_rc_file(context, zshrc_path, conda_prefix, shell, mamba_exe); | ||
| } |
There was a problem hiding this comment.
The function util::zsh_home_dir() is only declared for non-Windows platforms (guarded by #ifndef _WIN32 in the header file), but it's being called here without platform guards. This will cause a compilation error on Windows. The call should be wrapped in #ifndef _WIN32 preprocessor guards, or the entire zsh block should be conditionally compiled for non-Windows platforms only.
| } | |
| else if (shell == "zsh") | |
| { | |
| home = util::zsh_home_dir(); | |
| fs::u8path zshrc_path = home / ".zshrc"; | |
| modify_rc_file(context, zshrc_path, conda_prefix, shell, mamba_exe); | |
| } | |
| } | |
| #ifndef _WIN32 | |
| else if (shell == "zsh") | |
| { | |
| home = util::zsh_home_dir(); | |
| fs::u8path zshrc_path = home / ".zshrc"; | |
| modify_rc_file(context, zshrc_path, conda_prefix, shell, mamba_exe); | |
| } | |
| #endif |
| { | ||
| home = util::zsh_home_dir(); |
There was a problem hiding this comment.
The function util::zsh_home_dir() is only declared for non-Windows platforms (guarded by #ifndef _WIN32 in the header file), but it's being called here without platform guards. This will cause a compilation error on Windows. The call should be wrapped in #ifndef _WIN32 preprocessor guards, or the entire zsh block should be conditionally compiled for non-Windows platforms only.
| { | |
| home = util::zsh_home_dir(); | |
| { | |
| #ifndef _WIN32 | |
| home = util::zsh_home_dir(); | |
| #endif |
| { | ||
| home = util::zsh_home_dir(); |
There was a problem hiding this comment.
The function util::zsh_home_dir() is only declared for non-Windows platforms (guarded by #ifndef _WIN32 in the header file), but it's being called here without platform guards. This will cause a compilation error on Windows. The call should be wrapped in #ifndef _WIN32 preprocessor guards, or the entire zsh block should be conditionally compiled for non-Windows platforms only.
| { | |
| home = util::zsh_home_dir(); | |
| { | |
| #ifndef _WIN32 | |
| home = util::zsh_home_dir(); | |
| #endif |
| TEST_CASE("zsh_home_dir", "[mamba::util]") | ||
| { | ||
| if (on_win) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| const auto restore = mambatests::EnvironmentCleaner(); | ||
|
|
||
| SECTION("ZDOTDIR set") | ||
| { | ||
| set_env("ZDOTDIR", "/user/mamba/.zsh"); | ||
| REQUIRE(zsh_home_dir() == "/user/mamba/.zsh"); | ||
| } | ||
|
|
||
| SECTION("ZDOTDIR not set") | ||
| { | ||
| unset_env("ZDOTDIR"); | ||
| set_env("HOME", "/user/mamba"); | ||
| REQUIRE(zsh_home_dir() == user_home_dir()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The test case calls zsh_home_dir() which is only declared for non-Windows platforms (guarded by #ifndef _WIN32 in the header file). While there's a runtime check if (on_win) { return; } to skip the test on Windows, the test will still fail to compile on Windows because the compiler sees the calls to zsh_home_dir() regardless of the runtime check. The entire test case should be wrapped in #ifndef _WIN32 preprocessor guards to prevent compilation errors on Windows.
No description provided.