Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Additionally source libraries from XDG_CONFIG_DIRS #990

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Additionally source libraries from XDG_CONFIG_DIRS #990

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ce1c286
Additionally source libraries from XDG_CONFIG_DIRS
infinisil Sep 29, 2022
3f6363d
Only load legacy ~/.direnvrc if no other direnvrc loaded
infinisil Oct 3, 2022
f106329
Maintain old behavior regarding DIRENV_CONFIG
infinisil Oct 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 25 additions & 9 deletions stdlib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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} )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Author

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.

Copy link
Member

@zimbatm zimbatm Oct 4, 2022 •
edited

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)

Copy link
Author

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

xdg_config_dirs+=( "${XDG_CONFIG_HOME:-$HOME/.config}" )

declare -a direnv_config_dirs
for dir in "${xdg_config_dirs[@]}"; do
direnv_config_dirs+=( "$dir/direnv" )
done
if [[ -n "$DIRENV_CONFIG" ]]; then
direnv_config_dirs+=( "$DIRENV_CONFIG" )
fi
Copy link
Author

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?

Copy link
Member

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.

Copy link
Author

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


# This variable can be used by programs to detect when they are running inside
# of a .envrc evaluation context. It is ignored by the direnv diffing
Expand Down Expand Up @@ -1371,16 +1380,23 @@ __main__() {
trap __dump_at_exit EXIT

# load direnv libraries
for lib in "$direnv_config_dir/lib/"*.sh; do
# shellcheck disable=SC1090
source "$lib"
for direnv_config_dir in "${direnv_config_dirs[@]}"; do
for lib in "$direnv_config_dir/lib/"*.sh; do
# shellcheck disable=SC1090
source "$lib"
done
done

# 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
Comment on lines -1379 to -1383
Copy link
Member

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.

Copy link
Author

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?

Copy link
Member

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.

Copy link
Author

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?

# load global direnvrc's if present
for direnv_config_dir in "${direnv_config_dirs[@]}"; do
if [[ -f $direnv_config_dir/direnvrc ]]; then
# shellcheck disable=SC1090,SC1091
source "$direnv_config_dir/direnvrc" >&2
fi
done

# Legacy .direnvrc
if [[ -f $HOME/.direnvrc ]]; then
# shellcheck disable=SC1090,SC1091
source "$HOME/.direnvrc" >&2
fi
Expand Down