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/{nano,vim,emacs}: move editor specific pathsToLink to their specific module #319910

Closed
wants to merge 3 commits into from

Conversation

jopejoe1
Copy link
Member

Description of changes

No need to link editor specific path into /run/current-system/sw/ if the editor is not enabled.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@jopejoe1 jopejoe1 changed the title nixos{nano,vim.emacs}: move editor specific pathsToLink to their specific module nixos{nano,vim,emacs}: move editor specific pathsToLink to their specific module Jun 14, 2024
@jopejoe1 jopejoe1 changed the title nixos{nano,vim,emacs}: move editor specific pathsToLink to their specific module nixos{nano,vim,emacs}: move editor specific pathsToLink to their specific module Jun 14, 2024
@jopejoe1 jopejoe1 changed the title nixos{nano,vim,emacs}: move editor specific pathsToLink to their specific module nixos/{nano,vim,emacs}: move editor specific pathsToLink to their specific module Jun 14, 2024
Copy link
Contributor

@jian-lin jian-lin left a comment

Choose a reason for hiding this comment

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

It is a good idea to put related things together. However, I do not think the current Nixpkgs is ready for this PR:

  • This PR breaks users of foo with programs.foo.enable = false.
  • We do not even have programs.vim.enable and programs.emacs now.

Comment on lines 21 to +24
config = lib.mkIf cfg.defaultEditor {
environment.systemPackages = [ cfg.package ];
environment.variables = { EDITOR = lib.mkOverride 900 "vim"; };
environment.pathsToLink = [ "/share/vim-plugins" ];
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks users who use vim (environment.systemPackages = [ vim ]) as a non-default editor (programs.vim.defaultEditor = false). A release note is needed at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

The vim module should probably get a refactor, to have a separated enable option.

@@ -85,6 +85,8 @@ in
environment.systemPackages = [ cfg.package editorScript ];

environment.variables.EDITOR = mkIf cfg.defaultEditor (mkOverride 900 "emacseditor");

environment.pathsToLink = [ "/share/emacs" ];
Copy link
Contributor

Choose a reason for hiding this comment

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

For Emacs, this is wrong. This module is not programs.emacs, it is services.emacs which is used to config an Emacs daemon. Not all Emacs users want to use the services.emacs module.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did not realize that, that was only a service for emacs and not emacs itself.

@jopejoe1 jopejoe1 marked this pull request as draft June 15, 2024 08:13
@jopejoe1 jopejoe1 closed this Jul 11, 2024
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.

None yet

2 participants