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

Jump to bottom

autojump: new program.autojump.enable flag to automatically load autojump #47334

Merged
merged 3 commits into from Feb 20, 2019
Merged

autojump: new program.autojump.enable flag to automatically load autojump #47334

merged 3 commits into from Feb 20, 2019

Conversation

bfortz
Copy link
Contributor

@bfortz bfortz commented Sep 25, 2018 •
edited

Motivation for this change

The new programs.autojump.enable option forces the creation of links needed by the oh-my-zsh autojump plugin and automatically sources the necessary scripts in bash, zsh and fish. This also fixes problems due to the removal of autojump-share (#40725)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@bfortz
autojump: creates links required by oh-my-zsh for autojump.
0d749e5 
The autojump plugin in oh-my-zsh assumes autojump.zsh resides in
/run/current-system/sw/share/autojump/ but these links are not created
by default.

The new programs.autojump.enable option forces the creation of these
links.
@bfortz bfortz mentioned this pull request Sep 25, 2018
Merged
8 tasks
@bfortz
Copy link
Contributor Author

bfortz commented Sep 25, 2018

Should definitely go in 18.09 as well...

@bfortz
Copy link
Contributor Author

bfortz commented Sep 26, 2018

Following comments in #40725, programs.autojump.enable now automatically pulls the necessary scripts in bash (tested), zsh (tested) and fish (? @yurrriq could you test, I'm not using fish).

@bfortz bfortz changed the title autojump: creates links required by oh-my-zsh for autojump. autojump: new program.autojump.enable flag to automatically load autojump Sep 26, 2018
@yurrriq
Copy link
Member

yurrriq commented Sep 26, 2018

Will do. It already works for fish, and I don’t think I did anything special on the fish side, but I can at least make sure it doesn’t break.

@cdepillabout
Copy link
Member

cdepillabout commented Oct 27, 2018 •
edited

It is unfortunate that bash support was broken in #40725 and this PR did not make it in to 18.09. It is also unfortunate that the autojump documentation in the nixpkgs manual was not updated.

Any way to hurry this PR up and get it in to master?

@bfortz
Copy link
Contributor Author

bfortz commented Oct 28, 2018

@xeji any hope to see this merged (and back ported to 18.09?)

yurrriq
yurrriq reviewed Oct 29, 2018
View reviewed changes
nixos/modules/programs/autojump.nix

programs.bash.interactiveShellInit = "source ${pkgs.autojump}/share/autojump/autojump.bash";
programs.zsh.interactiveShellInit = mkIf prg.zsh.enable "source ${pkgs.autojump}/share/autojump/autojump.zsh";
programs.fish.interactiveShellInit = mkIf prg.fish.enable "source ${pkgs.autojump}/share/autojump/autojump.fish";
Copy link
Member

Choose a reason for hiding this comment

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

I can confirm this didn't break anything for me and my fish, but I'm pretty sure this line is redunant and unnecessary. Maybe someone else can chime in.

@matthewbauer
Copy link
Member

Ideally these things wouldn't need a module at all. What happens when you just put autojump into systemPackages? Don't the bash/zsh hooks pick things up. Ideally we could make that process better without needing to introduce an option.

@yurrriq
Copy link
Member

yurrriq commented Nov 16, 2018

@matthewbauer, I thought so too, but according to discussion on #40725, apparently not.

It seems to me it might be preferable to address the root of the problem, at the bash/zsh level, rather than introduce modules like this one. But since I've not reproduced the problem myself, I'm not sure how to approach it.

@bfortz
Copy link
Contributor Author

bfortz commented Nov 16, 2018

There are pros and cons for a module:
Pros:

  • the module should be the responsibility of the plugin maintainer, and does not pollute the zsh/bash/fish builders
  • we can imagine similar situations for other modules (thefuck uses the same approach). If all plugins must be taken care of at the bash/zsh level, it might quickly become a mess to maintain.
    Cons:
  • too many small modules... but is it really a problem?

@yurrriq
Copy link
Member

yurrriq commented Nov 16, 2018

I didn't mean to advocate that we add special handling for autojump or individual plugins at the bash/zsh level, but rather we figure out why it's not picking up share/zsh/site-functions/ etc.

@cdepillabout
Copy link
Member

cdepillabout commented Nov 17, 2018 •
edited

@matthewbauer

Ideally these things wouldn't need a module at all. What happens when you just put autojump into systemPackages? Don't the bash/zsh hooks pick things up.

I think this might be nicer for some people, but I'd argue that it may be somewhat unintuitive.

If I have the autojump package installed system-wide, I wouldn't necessarily want to make my all the users on the system get the autojump hooks run when they login just because they are using bash. I'd prefer if autojump was an opt-in experience. (For instance, on my own system, I don't want root to ever run any autojump code when they run bash.)

Originally, enabling autojump was done on Nix with the autojump-share script, but that was removed in #40725. Now, if you want to use autojump in bash you need to figure out where the autojump hooks are and source them directly.

This "opt-in" style for autojump seems to be how other distros (like Arch) do it as well.

It'd also be great if this PR updated the nixpkgs manual with how to use autojump:

https://nixos.org/nixpkgs/manual/#sec-shell-helpers

I guess I should add that I am in favor of this current PR. It adds an easy way to get autojump added whenever anyone runs bash: programs.bash.interactiveShellInit = "source ${pkgs.autojump}/share/autojump/autojump.bash".

However, this isn't really an option for people not on NixOS.

@domenkozar domenkozar merged commit ae3a807 into NixOS:master Feb 20, 2019
@domenkozar
Copy link
Member

This still doesn't make it easier for non-NixOS users, but at least gets NixOS users back on track. Backporting also to 18.09

@bfortz bfortz deleted the autojump branch February 20, 2019 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurrriq yurrriq yurrriq approved these changes

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
6 participants
@bfortz @yurrriq @cdepillabout @matthewbauer @domenkozar @GrahamcOfBorg