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

lorri: add direnv as service dependency #85144

Closed
wants to merge 1 commit into from
Closed

lorri: add direnv as service dependency #85144

wants to merge 1 commit into from

Conversation

candeira
Copy link
Contributor

The readme at https://github.com/target/lorri says:
Enable the daemon service. Set services.lorri.enable = true; in your NixOS configuration.nix or your home-manager home.nix.
This will automatically install both the lorri command and direnv (both required for the nextsteps).

However, it was not true of direnv. It wasn't getting installed. Now it will.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@candeira
lorri: add direnv as service dependency
58751a5 
The readme at  https://github.com/target/lorri says:
    Enable the daemon service. Set services.lorri.enable = true; in your NixOS configuration.nix or your home-manager home.nix.
    This will automatically install both the lorri command and direnv (both required for the nextsteps).

However, it was not true of direnv. It wasn't getting installed. Now it will.
@candeira
Copy link
Contributor Author

I didn't test this at all: this is only a drive-by PR made on Github.

@Profpatsch said it would be ok.

doronbehar
doronbehar reviewed Apr 13, 2020
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Personally, I'm against this change as some users may wish to use lorri without direnv which should be totally legit. Perhaps lorri should update their docs.

@jonringer
Copy link
Contributor

jonringer commented Apr 13, 2020 •
edited

I have to agree with @doronbehar , although the application has support for direnv, It should not bring the package into the environment. Not to mention this doesn't even correctly install direnv as direnv requires additional steps for it to work https://github.com/direnv/direnv/blob/master/docs/hook.md#bash

@flokli
Copy link
Contributor

flokli commented Apr 13, 2020

Please file a PR upstream at lorri to fix the documentation. direnv is optional for lorri to work.

@flokli flokli closed this Apr 13, 2020
@Profpatsch
Copy link
Member

The primary use-case of lorri is usage with direnv, so it makes sense to add it. However, since direnv requires additional setup (which I didn’t think about), it’s probably best to make users explicitely set it up.

@jonringer
Copy link
Contributor

with home-manager, you can do:

  programs.direnv.enable = true;
  services.lorri.enable = true;

and you get the desired effect

candeira added a commit to candeira/lorri that referenced this pull request Apr 17, 2020
@candeira
Fix README (direnv shouldn't be auto-installed)
b068afe 
Direnv isn't and shouldn't be automatially installed on installing lorri, per NixOS/nixpkgs#85144
@candeira candeira mentioned this pull request Apr 17, 2020
Merged
2 tasks
@candeira
Copy link
Contributor Author

Well, I learnt something! Thanks folks, in particular @flokli. I followed your advice:

target/lorri#381

@candeira candeira deleted the patch-1 branch April 19, 2020 14:11
curiousleo pushed a commit to target/lorri that referenced this pull request Apr 30, 2020
@candeira
Fix README (direnv shouldn't be auto-installed) (#381)
0376794 
Direnv isn't and shouldn't be automatially installed on installing lorri, per NixOS/nixpkgs#85144
@curiousleo curiousleo mentioned this pull request Apr 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doronbehar doronbehar doronbehar left review comments

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@candeira @jonringer @flokli @Profpatsch @doronbehar