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

nixos/direnv: init #241528

Merged
merged 1 commit into from
Jul 12, 2023
Merged

nixos/direnv: init #241528

merged 1 commit into from
Jul 12, 2023

Conversation

Gerg-L
Copy link
Contributor

@Gerg-L Gerg-L commented Jul 4, 2023

Closes #192667

Description of changes

There really should be a simple direnv module and #192667 has become quite stale
So I took most of the criticisms and applied them along with a few bits of my own

I would like to make an option to conditionally load based on being in a nix shell,
but alas i couldn't get fish to work correctly

Things done
  • Tested the module

drupol

This comment was marked as outdated.

@2xsaiko
Copy link
Contributor

2xsaiko commented Jul 6, 2023

FWIW I have some extra code in my config's module for direnv to source the user's configuration also (which direnv normally does), might be worth including here. This is what configuration I install (in /etc/xdg/direnv as opposed to /etc/direnv):

    environment.etc = {
      "xdg/direnv/lib/nix-direnv.sh".source = mkIf cfg.enableNixDirenv "${pkgs.nix-direnv}/share/nix-direnv/direnvrc";

      # Also load user's config files
      "xdg/direnv/lib/zz-user.sh".text = ''
        direnv_config_dir_home="''${DIRENV_CONFIG_HOME:-''${XDG_CONFIG_HOME:-$HOME/.config}/direnv}"

        for lib in "$direnv_config_dir_home/lib/"*.sh; do
          source "$lib"
        done

        unset direnv_config_dir_home
      '';

      "xdg/direnv/direnvrc".text = ''
        direnv_config_dir_home="''${DIRENV_CONFIG_HOME:-''${XDG_CONFIG_HOME:-$HOME/.config}/direnv}"

        # load the global ~/.direnvrc if present
        if [[ -f $direnv_config_dir_home/direnvrc ]]; then
          # shellcheck disable=SC1090,SC1091
          source "$direnv_config_dir_home/direnvrc" >&2
        elif [[ -f $HOME/.direnvrc ]]; then
          # shellcheck disable=SC1090,SC1091
          source "$HOME/.direnvrc" >&2
        fi

        unset direnv_config_dir_home
      '';
    };

@Gerg-L
Copy link
Contributor Author

Gerg-L commented Jul 6, 2023

FWIW I have some extra code in my config's module for direnv to source the user's configuration also (which direnv normally does), might be worth including here.

I can't see any negatives to this I'll add it

nixos/modules/programs/direnv.nix Outdated Show resolved Hide resolved
nixos/modules/programs/direnv.nix Outdated Show resolved Hide resolved
nixos/modules/programs/direnv.nix Outdated Show resolved Hide resolved
@fufexan

This comment was marked as resolved.

Copy link
Contributor

@bbenne10 bbenne10 left a comment

Choose a reason for hiding this comment

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

Other than one (admittedly rather nitpicky) concern re a comment: this looks like a very good approach that addresses every problem I have with #192667!

nixos/modules/programs/direnv.nix Outdated Show resolved Hide resolved
nixos/modules/programs/direnv.nix Outdated Show resolved Hide resolved
nixos/modules/programs/direnv.nix Outdated Show resolved Hide resolved
@Gerg-L Gerg-L force-pushed the nixos/direnvrc branch 3 times, most recently from 9e11556 to e1678e6 Compare July 7, 2023 14:42
nixos/modules/programs/direnv.nix Outdated Show resolved Hide resolved
nixos/modules/programs/direnv.nix Outdated Show resolved Hide resolved
nixos/modules/programs/direnv.nix Outdated Show resolved Hide resolved
nixos/modules/programs/direnv.nix Outdated Show resolved Hide resolved
@Gerg-L Gerg-L force-pushed the nixos/direnvrc branch 2 times, most recently from 5a39574 to 57c37f9 Compare July 7, 2023 15:06
Copy link
Contributor

@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.

Now that you added a package attribute in the config, you can use it as such...

nixos/modules/programs/direnv.nix Outdated Show resolved Hide resolved
nixos/modules/programs/direnv.nix Outdated Show resolved Hide resolved
nixos/modules/programs/direnv.nix Outdated Show resolved Hide resolved
@Gerg-L
Copy link
Contributor Author

Gerg-L commented Jul 7, 2023

Now that you added a package attribute in the config, you can use it as such...

Does the package gets added to automatically to systemPackages?
Probably not right?

@drupol
Copy link
Contributor

drupol commented Jul 7, 2023

Does the package gets added to automatically to systemPackages? Probably not right?

No it doesn't but your PR doesn't do it either.

@Gerg-L
Copy link
Contributor Author

Gerg-L commented Jul 7, 2023

Does the package gets added to automatically to systemPackages? Probably not right?

No it doesn't but your PR doesn't do it either.

Yeah removed it with the last force push, brain fart

@drupol
Copy link
Contributor

drupol commented Jul 7, 2023

Yeah removed it with the last force push, brain fart

Then you just have to use cfg.package.

@Gerg-L
Copy link
Contributor Author

Gerg-L commented Jul 7, 2023

Alright pushed changes

@Gerg-L
Copy link
Contributor Author

Gerg-L commented Jul 8, 2023

I finally figured out why fish wasn't working with my conditional loading idea: direnv has a .fish file in it's $out which is automatically sourcing direnv,
I don't know whether this should be a change to the package itself it doesn't seem correct that it's automatically sourcing itself when installed --but only in fish

so I added a loadInNixShell option
It's slightly sketchy as it's just greping $PATH to detect nix shell
nix develop and nix-shell both set an env var so they're easy to detect but it should work the majority of the time and It's not active by default

I also added options to disable and set the package of nix-direnv as drupol suggested

@Gerg-L Gerg-L requested review from drupol and jian-lin July 12, 2023 11:44
Copy link
Contributor

@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.

A minor syntax change and I think it's good to go.

nixos/modules/programs/direnv.nix Outdated Show resolved Hide resolved
@Gerg-L
Copy link
Contributor Author

Gerg-L commented Jul 12, 2023

A minor syntax change and I think it's good to go.

Changes pushed!

@drupol drupol merged commit f3d3147 into NixOS:master Jul 12, 2023
5 checks passed
@Gerg-L Gerg-L deleted the nixos/direnvrc branch July 14, 2023 02:22
@eclairevoyant eclairevoyant added the 9.needs: port to stable A PR needs a backport to the stable release. label Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants