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

nixos/zsh: move zsh setopt #58012

Merged
merged 1 commit into from Jun 8, 2019
Merged

nixos/zsh: move zsh setopt #58012

merged 1 commit into from Jun 8, 2019

Conversation

Izorkin
Copy link
Contributor

@Izorkin Izorkin commented Mar 21, 2019 •
edited

Motivation for this change

When installing the oh-my-zsh plugin, it additionally sets its setopt parameters. This PR transfers the setopt parameter after the plug-in is loaded so that you can reassign the parameters. For example, delete the parameters that are loaded plugin oh-my-zsh.

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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Izorkin Izorkin requested a review from infinisil as a code owner March 21, 2019 06:52
@Izorkin
Copy link
Contributor Author

Izorkin commented Mar 21, 2019

cc @Mic92

@Mic92
Copy link
Member

Mic92 commented Mar 21, 2019

I think I have to skip on this one, because I don't know what other consequences moving setopt down could be. I could imagine that some other plugins rely on having setopt set upfront.

@Mic92
Copy link
Member

Mic92 commented Mar 21, 2019

cc @Ma27

@Ma27
Copy link
Member

Ma27 commented Jun 4, 2019

hmm sorry, totally missed that one... I'll try to have a look at this tonight :)

@Ma27
Copy link
Member

Ma27 commented Jun 7, 2019

I think that the change itself seems reasonable. IMHO the setopt option is mainly intended to configure options that will be available when actually using zsh.

However this may break setups where scripts e.g. in interactiveShellInitrely on several options being set, so I'd add a changelog entry (probably even in the section for breaking changes) and then this should be mergable IMHO.

@Izorkin
Copy link
Contributor Author

Izorkin commented Jun 8, 2019

@Ma27 i ask you to help compile the changelog, I have a bad english.

@Ma27
Copy link
Member

Ma27 commented Jun 8, 2019

It should be sufficient to add a <listitem> in the Backward Incompatibilities section which says that the setopt declarations will be evaluated at the end of zshrc, so any code in interactiveShellInit, loginShellInit and promptInit may break if it relies on those options being set.

All options can be referenced using <xref linkend="opt-programs.zsh.optionname" />.

The changelog can be build by running nix-build nixos/release.nix -A manual --arg supportedSystems '[ builtins.currentSystem ]'.

A pretty helpful guide for docbook can be found here: https://tdg.docbook.org/tdg/4.5/part2.html

@Izorkin
Copy link
Contributor Author

Izorkin commented Jun 8, 2019

@Ma27 thanks. PR updated.

Ma27
Ma27 reviewed Jun 8, 2019
View reviewed changes
nixos/doc/manual/release-notes/rl-1909.xml Outdated Show resolved Hide resolved
nixos/doc/manual/release-notes/rl-1909.xml Outdated Show resolved Hide resolved
nixos/doc/manual/release-notes/rl-1909.xml Outdated Show resolved Hide resolved
Ma27
Ma27 approved these changes Jun 8, 2019
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Updated the changelog. I guess this should be fine now. Let's wait for ofBorg to finish and merge then 👍

@Ma27
Copy link
Member

Ma27 commented Jun 8, 2019

With a changelog entry which warns about the possible compatibility break this should be fine.
For the record, I've done something similar for ZSH aliases in #28378.

@Ma27 Ma27 merged commit de4ab3b into NixOS:master Jun 8, 2019
@Ma27
Copy link
Member

Ma27 commented Jun 8, 2019

@Izorkin thanks!

@Izorkin Izorkin deleted the zsh-options2 branch June 9, 2019 07:25
aszlig added a commit to openlab-aux/vuizvui that referenced this pull request Sep 23, 2019
@aszlig
modules/zsh: Fix options and clean up a bit
972808b 
Since a while ago[1], the setting of ZSH options is now done after
interactiveShellInit, so using unsetopt SHARE_HISTORY doesn't work
anymore because it is set *afterwards*.

Instead of setting these options, we now use the setOptions option
instead and override it with exactly the options I want to be set.

Additionally, compinit is also no longer necessary, because it is done
by default and invoking it on our own is just redundant.

[1]: NixOS/nixpkgs#58012

Signed-off-by: aszlig <aszlig@nix.build>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ma27 Ma27 Ma27 approved these changes

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 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
4 participants
@Izorkin @Mic92 @Ma27 @GrahamcOfBorg