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.

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

[FEATURE] additional (and optional) system-wide configuration path PREFIX/etc/direnv #1038

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brandung-sjorek
Copy link

The PR implements additional (and optional) system-wide configuration path PREFIX/etc/direnv.

Motivation

My original motivation was to distribute some direnenv-extensions (for use in our company), with package-managers like dpkg, Homebrew brew, MacPorts port and the beloved nix. Currently the situation with direnv-extensions is cumbersome and difficult, as installation of files into ~/.config/direnv/lib/*.sh is not possible, and alternatives like creating symlinks to system-wide provided extensions appear to be somehow error-prone.

After glimpsing into direnv's sources I thought, it might be a good idea to implement this posix-like behaviour for the whole configuration.

I could elaborate in depth on the PR, but I think the actual changes enlighten you more.

Summary of changes

  • configure SYSCONFDIR (=PREFIX/etc/direnv) at build-time in GNUmakefile and integrate it into the direnv-binary
  • replace build-time configured SYSCONFDIR in direnv stdlib
  • implement additional loading of direnv.toml from SYSCONFDIR
  • implement additional SYSCONFDIR-related runtime-configuration source …-inclusions to stdlib.sh
  • provide some example SYSCONFDIR runtime-configuration files, add them to make install
  • update documentation accordingly

I'd be glad to get your opinion and feedback on this PR. And I'd be really happy if the PR or something similar gets merged (or if someone proposes another solution to my original motivation).

Cheers and
Happy new year!

@brandung-sjorek brandung-sjorek force-pushed the feature/system-wide-configuration-path branch 3 times, most recently from a154be8 to 9795a99 Compare December 27, 2022 21:08
@brandung-sjorek brandung-sjorek force-pushed the feature/system-wide-configuration-path branch 2 times, most recently from fda644a to 9799e7f Compare December 27, 2022 21:59
@brandung-sjorek brandung-sjorek force-pushed the feature/system-wide-configuration-path branch 3 times, most recently from d277508 to 6bba2bc Compare February 12, 2023 10:20
@brandung-sjorek
Copy link
Author

@zimbatm
Hey Jonas,

I'm sorry to bother you, but I'd really appreciate any feedback on this PR. The change might not be of much value in the Nix world, but it would really help me to initiate a transition to it in my company.

Thanks for your work so far, anyway …
Stephan

Copy link

@drupol drupol left a comment

Choose a reason for hiding this comment

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

I'm eager to review this PR, but I've noticed several changes that seem unrelated to the new feature, as well as numerous code style modifications. In the future, it might be best to keep such changes separate from new feature introductions for the sake of clarity and easier review. Don't you think?

@brandung-sjorek
Copy link
Author

brandung-sjorek commented Jun 6, 2023

@drupol Sorry, but I don't think that there are unrelated changes or code-style modifications. Please, take a look at my summary of changes in the PR description and tell me what to drop from the PR. I'm curious - what make's you think there are unrelated or style changes ? Could you please give me an example?

Nevertheless, thanks for your feedback!

Copy link
Author

@brandung-sjorek brandung-sjorek left a comment

Choose a reason for hiding this comment

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

@drupol Are these the unrelated changes you're talking about?

GNUmakefile Outdated Show resolved Hide resolved
internal/cmd/const.go Outdated Show resolved Hide resolved
internal/cmd/mod.go Outdated Show resolved Hide resolved
Copy link

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Sure, here are some places where the styling was modified.

GNUmakefile Outdated Show resolved Hide resolved
internal/cmd/const.go Outdated Show resolved Hide resolved
internal/cmd/mod.go Outdated Show resolved Hide resolved
@brandung-sjorek
Copy link
Author

brandung-sjorek commented Jun 6, 2023

@drupol Ok, I understand and agree - I'll change that this evening - 08:00 PM CEST

@drupol
Copy link

drupol commented Jun 6, 2023

Take your time and thanks for your collaboration.

…EFIX/etc/direnv

Signed-off-by: Stephan Jorek <stephan.jorek@brandung.de>
@brandung-sjorek brandung-sjorek force-pushed the feature/system-wide-configuration-path branch from 6bba2bc to e180c33 Compare June 10, 2023 07:17
@brandung-sjorek
Copy link
Author

brandung-sjorek commented Jun 10, 2023

@drupol I removed the code-style changes from the PR. Now, I ask myself what to do next? There are still changes in there, that are technically not needed, for example …

  • the update of the man-pages, mentioning the system-wide configuration
  • the example configuration-files in /etc
  • the gnused package-addition in the *.nix recipes, used in the GNUmakefile for the building configuration-files and man-pages with correct paths inside

… and maybe more. How do we want to deal with those changes. To my mind I would keep them.

direnv_config_dir="${DIRENV_CONFIG:-${XDG_CONFIG_HOME:-$HOME/.config}/direnv}"

# Where system-wide direnv configuration is stored
direnv_sysconfig_dir="${DIRENV_SYSCONFIG:-/etc/direnv}"
Copy link
Contributor

Choose a reason for hiding this comment

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

My gut feeling tells me that this should be XDG spec compliant, eg.: /usr/share/direnv or /usr/local/share/direnv, but essentially $prefix/share/direnv.
Definitely XDG_DATA_DIRS compliant. /etc/direnv is definitely not XDG spec.

@@ -138,8 +138,10 @@ things.
Exporting variables by hand is a bit repetitive so direnv provides a set of
utility functions that are made available in the context of the `.envrc` file.
Check the direnv-stdlib(1) man page for more details. You can also define your
own extensions inside `$XDG_CONFIG_HOME/direnv/direnvrc` or
`$XDG_CONFIG_HOME/direnv/lib/*.sh` files.
user-specific extensions inside `$DIRENV_CONFIG/direnvrc` or
Copy link
Contributor

Choose a reason for hiding this comment

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

Should still respect XDG spec and refer to XDG spec IMHO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants