-
Notifications
You must be signed in to change notification settings - Fork 634
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
Additionally source libraries from XDG_CONFIG_DIRS #990
base: master
Are you sure you want to change the base?
Conversation
Allows the system administrator to configure direnv libraries and rc files using by default /etc/xdg/direnv/lib/*.sh and /etc/xdg/direnv/direnvrc
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.
thanks, it's moving in the right direction
# load the global ~/.direnvrc if present | ||
if [[ -f $direnv_config_dir/direnvrc ]]; then | ||
# shellcheck disable=SC1090,SC1091 | ||
source "$direnv_config_dir/direnvrc" >&2 | ||
elif [[ -f $HOME/.direnvrc ]]; then |
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.
It's probably enough just to extend the above block. I would rather not change this logic as now it's loading both the ~/.config/direnv/direnvrc and the ~/.direnvrc.
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.
We don't want to load ~/.direnvrc
multiple times though, so a variable is needed, check out the new commit: 3f6363d
Is this alright?
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.
It's not quite there yet. It should only load the first direnvrc, starting with $direnv_config_dir/direnvrc
, then (maybe) the rest of the XDG config dirs, and finally the legacy one. But I think this is best left untouched as it doesn't solve a specific use-case.
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'm not sure I understand, can you show me how you'd change the code?
@@ -19,7 +19,16 @@ direnv="$(command -v direnv)" | |||
DIRENV_LOG_FORMAT="${DIRENV_LOG_FORMAT-direnv: %s}" | |||
|
|||
# Where direnv configuration should be stored | |||
direnv_config_dir="${DIRENV_CONFIG:-${XDG_CONFIG_HOME:-$HOME/.config}/direnv}" | |||
IFS=: xdg_config_dirs=( ${XDG_CONFIG_DIRS:-/etc/xdg} ) |
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.
IFS=: xdg_config_dirs=( ${XDG_CONFIG_DIRS:-/etc/xdg} ) | |
direnv_config_dir="${DIRENV_CONFIG:-${XDG_CONFIG_HOME:-$HOME/.config}/direnv}" | |
IFS=: xdg_data_dirs=( ${XDG_DATA_DIRS:-/usr/local/share/:/usr/share} ) |
I find the XDG spec confusing. I think in this scenario, the ~/.config/direnv/lib/*.sh
can be considered "user" configuration, but the shared libraries are probably part of XDG_DATA_DIRS? Or it should have been XDG_DATA_HOME already.
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.
The spec says this:
$XDG_DATA_HOME
defines the base directory relative to which user-specific data files should be stored
It doesn't explain what "user-specific data files" are, but from my experience I'd say it's "files that contain persistent user-specific data but which are created and managed by the program". I don't think such direnv libraries fit in there, since they're neither user-specific, nor created or managed by the program. And $XDG_DATA_DIRS
is just the extension of $XDG_DATA_HOME
to multiple additional directories.
I really think $XDG_CONFIG_HOME
(and its extension to $XDG_CONFIG_DIRS
) is the right place for these libraries, they really are configuration: They don't store user-specific data, are reusable between arbitrarily many installation, they're kind of like plugins.
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 agree for ~/.config/direnv/lib/*.sh
, but as soon as you start scanning outside of the user's home, it becomes re-distributable code. For nix-direnv, you wouldn't expect the user to change the implementation themselves but rather to propose the changes upstream.
This also shows here where the code is in $out/share: NixOS/nixpkgs#192667 (comment)
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 don't understand, are you saying that because libraries outside ~
are redistributable (though I'd argue libraries in ~
are equally as redestributable), XDG_DATA_DIRS
is the correct variable to read from? That doesn't make sense to me, XDG_DATA_DIRS
like XDG_DATA_HOME
is for data, not configuration or libraries or redistributable things. For GDPR compliance for example, if somebody asks me to wipe all user data, I'd wipe all XDG_DATA_DIRS
and XDG_DATA_HOME
, the applications should all continue working with the same configuration though
b017310
to
ce1c286
Compare
stdlib.sh
Outdated
if [[ -n "$DIRENV_CONFIG" ]]; then | ||
direnv_config_dirs+=( "$DIRENV_CONFIG" ) | ||
fi |
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.
@zimbatm Btw this also slightly changes behavior: Before when DIRENV_CONFIG
was set, $XDG_CONFIG_HOME
/~/.config
was ignored, but now it's loaded regardless. I can easily change the behavior to work as before, what do you think?
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.
Given how widely direnv is deployed, it's best to keep as much back-compat as possible. DIRENV_CONFIG is for when you want to override the default behaviour.
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 fixed this in f106329 now, it has the old behavior again
Only use ~/.config/direnv if DIRENV_CONFIG is not set
@infinisil @zimbatm Maybe my PR comes in handy here? See #1038 … I discovered this PR after creating my PR, so maybe I'll first take a look at your conversation (especially those regarding |
Is there any movement on this PR? I am waiting on this PR to approve and merge NixOS/nixpkgs#192667, which would be extremely helpful in integrating nix-direnv (and direnv itself) into nix installs natively. |
I'm waiting on clarifications of @zimbatm's comments, to me this PR would be good to merge. |
Allows the system administrator to configure direnv libraries and rc files using by default /etc/xdg/direnv/lib/*.sh and /etc/xdg/direnv/direnvrc
See NixOS/nixpkgs#192667 (comment) for motivation