-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
zsh: path handling - remove $HOME reliance & make relative/absolute path logic consistent
#6089
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
97862d4 to
8233815
Compare
modules/programs/zsh.nix
Outdated
| # Absolute paths are assigned unmutated, relative paths | ||
| # are prepended with the user's home directory. | ||
| zdotdir = (strings.optionalString (!strings.hasPrefix "/" cfg.dotDir) | ||
| config.home.homeDirectory) + escapeShellArg cfg.dotDir; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had dotDir = ".config/zsh"; in my config and I think it generated a ZDOTDIR value of
echo $ZDOTDIR
/home/teto.config/zsh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@teto thanks for raising this -- looking at the code, yeah, I can see where I missed that. Curiously, it didn't cause the test to fail, which is doubly concerning -- let me made a change and test and I'll push an update 👍
|
Looks like the CI is a bit unhappy with this 🙂 But perhaps not far off from working? |
|
@rycee yeah -- apologies for the delay on this; I started, then realised there was more to the fix than I anticipated and life got in the way a bit. I'm actually doing nothing right now so will see if I can get it boxed off :) |
484998a to
b48c6bf
Compare
|
@rycee @teto I believe this is good to go. Thanks for your patience with it. I've been descriptive in the commit messages, but, in addition to original PR description I found that when setting The first commit removes all reliance on If the second commit isn't desired I can revert it, otherwise, it poses a minor breaking change to those who have set a custom ZDOTDIR but not a custom HISTFILE location. I'm unsure of the best way to communicate/mitigate this (via state version logic?), though I'd wager those installs are few -- why go to the effort of moving most .z* files out of |
$HOME reliance & make relative/absolute path logic consistent
b48c6bf to
d219d9d
Compare
|
Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting. If you are the original author of the PR
If you are not the original author of the PR
|
|
Curious if you're still interested in pursuing this? We've had a bit of refactoring since this was made. |
3041638 to
9962d77
Compare
Previously, `config.programs.zsh.dotDir` prepended strings with `$HOME`. This caused issues like nix-community#5100, where `$HOME` is inconsistently resolved in time for the evaluation of the option. The handling of this variable is also inconsistent with how paths are handled elsewhere, including within the same module, where `config.programs.zsh.history.path` does not mutate the supplied string. To preserve backwards compatibility, this change prepends `config.home.homeDirectory` to relative paths, while assigning absolute paths unchanged. Tests for both cases are added. Signed-off-by: Austin Horstman <[email protected]>
9962d77 to
3b6e4fd
Compare
Previously, a stateVersion check for 20.03 determined whether or not the input to `programs.zsh.history.path` would be prepended with `$HOME`. However, this was not communicated in the documentation, which stated the version check determined whether the default histfile location would be in `programs.zsh.dotDir` or `home.homeDirectory`. The current change simplifies matters and brings path handling in-line with that of the preceding work on dotDir path handling. If a relative path is provided, it is parsed as being relative to `home.homeDirectory`. Both absolute and relative paths are supported, and are cleaned before being passed to other functions. Tests have been rewritten for the new logic, with case handling for reusability. Signed-off-by: Austin Horstman <[email protected]>
3b6e4fd to
3e2c84d
Compare
|
Rebased on master and resolved all the merge conflicts and fixed some issues that were caught in newer tests. |
|
Just FYI, this broke usecases where users had environment variables in |
|
Same, after updating with I agree with this simpler handling, but some warning, release note or hiding it behind a new |
Give a news entry about change for path handling work done in nix-community#6089. Signed-off-by: Austin Horstman <[email protected]>
Give a news entry about change for path handling work done in nix-community#6089. Signed-off-by: Austin Horstman <[email protected]>
Give a news entry about change for path handling work done in nix-community#6089. Signed-off-by: Austin Horstman <[email protected]>
Give a news entry about change for path handling work done in nix-community#6089. Signed-off-by: Austin Horstman <[email protected]>
Give a news entry about change for path handling work done in nix-community#6089. Signed-off-by: Austin Horstman <[email protected]>
|
@andrevmatos @gepbird thanks for identifying those issues, created #7561 to handle that better. Would be helpful to test with your previous config to verify it works, as expected. |
Give a news entry about change for path handling work done in nix-community#6089. Signed-off-by: Austin Horstman <[email protected]>
Give a news entry about change for path handling work done in nix-community#6089. Signed-off-by: Austin Horstman <[email protected]>
ref: nix-community/home-manager#6089 Default zsh config in home-manager creates a `$HOME/.zshenv` now with home-manager session vars initialized and will conflict with file trying to be linked to that location. Add contents of my own `.zshenv` into `envExtra` section. Signed-off-by: Stanley Chan <[email protected]>
Give a news entry about change for path handling work done in nix-community#6089. Signed-off-by: Austin Horstman <[email protected]>
Give a news entry about change for path handling work done in nix-community#6089. Signed-off-by: Austin Horstman <[email protected]>
Give a news entry about change for path handling work done in nix-community#6089. Signed-off-by: Austin Horstman <[email protected]>
Give a news entry about change for path handling work done in nix-community#6089. Signed-off-by: Austin Horstman <[email protected]>
Give a news entry about change for path handling work done in nix-community#6089. Signed-off-by: Austin Horstman <[email protected]>
Give a news entry about change for path handling work done in nix-community#6089. Signed-off-by: Austin Horstman <[email protected]>
Give a news entry about change for path handling work done in #6089. Signed-off-by: Austin Horstman <[email protected]>
|
JFYI a trailing slash in my homeDirectory path caused a zsh initialization failure. My nix-darwin configuration: After updating and rebuilding I got this Removing the trailing slash from the path fixed the issue. |
Give a news entry about change for path handling work done in nix-community#6089. Signed-off-by: Austin Horstman <[email protected]>
This change adapts the zsh configuration to follow the recent updates in home-manager (specifically PR nix-community/home-manager#6089 and nix-community/home-manager#7561). With this update, the `programs.zsh.dotDir` option now primarily expects an absolute path, though relative paths remain supported for the time being for backward compatibility. My hard-coded path for `.config/zsh` has been replaced with the `config.xdg.configHome` value. As a result, the manually defined `zdotdir` variable became redundant and has been removed, simplifying the overall configuration.
Description
Previously,
config.programs.zsh.dotDirprepended strings with$HOME. This caused issues where$HOMEis inconsistently resolved in time for the evaluation of the option. The handling of this variable is also inconsistent with how paths are handled elsewhere, including within the same module, whereconfig.programs.zsh.history.pathdoes not mutate the supplied string.This change prepends
config.home.homeDirectoryto relative paths, while assigning absolute paths unchanged. Tests for both cases are added.Closes #5100
Checklist
Change is backwards compatible.
Code formatted with
./format.Code tested through
nix-shell --pure tests -A run.allornix develop --ignore-environment .#allusing Flakes.Test cases updated/added. See example.
Commit messages are formatted like
See CONTRIBUTING for more information and recent commit messages for examples.
If this PR adds a new module
Maintainer CC