nix-community / home-manager

Manage a user environment using Nix [maintainer=@rycee]
https://nix-community.github.io/home-manager/
MIT License
6.88k stars 1.79k forks source link

bug: `programs.tmux` options can override tmux-sensible, `/etc/tmux.conf` inadvertently #2541

Open lilyball opened 2 years ago

lilyball commented 2 years ago

Is there an existing issue for this?

Issue description

programs.tmux has various options such as terminal, aggressiveResize, historyLimit, etc. These default to the current default values that tmux uses, so the assumption is that if you don't touch them, it doesn't matter as they're just setting the options to the values they already have.

Unfortunately, this is not true. The easiest way to make this not true is to set programs.tmux.sensibleOnTop = true, which will load tmux-sensible prior to any other configuration. tmux-sensible sets a bunch of these options, and the default home-manager module configuration will then immediately reset some of them back to the original tmux defaults. But even with sensibleOnTop off, you can still run into this problem if you have an /etc/tmux.conf that sets up system-wide configuration (I believe nix-darwin's tmux support will create this file, and I assume—though haven't checked—that nixos will too).

My suggestion would be to have these options have no default at all but use defaultText to describe the tmux default, and then to have the actual configuration check if a value has been set. Alternatively they could allow null and use that as the default (as prefix does) or check if the value is different than the default (as shortcut does), but those feel like hackier solutions (especially the shortcut one; that really should just switch over to having no default and checking if the option is defined).

Maintainer CC

@marsam @toonn @expipiplus1

System information

- system: `"x86_64-darwin"`
 - host os: `Darwin 21.1.0, macOS 10.16`
 - multi-user?: `yes`
 - sandbox: `relaxed`
 - version: `nix-env (Nix) 2.4`
 - channels(lily): `"darwin, home-manager"`
 - channels(root): `"nixpkgs-21.11pre329221.c11d08f0239"`
 - nixpkgs: `/etc/nix/inputs/nixpkgs`
lilyball commented 2 years ago

To clarify, I think the following configuration should produce something very minimal:

{
    programs.tmux.enabled = true;
}

Some of the options, such as keyMode, it seems reasonable to set unconditionally. That's not something that really should be set at the system level. But anything that tmux-sensible touches (or might plausibly touch in the future) and currently defaults to the tmux default should not end up in the generated configuration file if I haven't touched the value.

toonn commented 2 years ago

One concern here is that it's not quite backwards compatible to change this. I do think null should be a legal value to say "Don't mess with this setting." I also agree it's a bit unfortunate to always override with the program's defaults but as long as a way is introduced to disable the overriding behavior it's not terrible and IMO it would violate POLA if the module documentation shows defaults that don't actually apply unless you set them explicitly.

lilyball commented 2 years ago

It's very nearly backwards-compatible to change this when sensibleOnTop is off (AFAIK it only differs if /etc/tmux.conf exists, and if it is setting stuff that programs.tmux is overwriting by accident then it's probably good to fix that). With sensibleOnTop on, changing this is backwards-incompatible, but again, the current behavior is rather buggy in this scenario, so again it seems like a good fix to make. In theory you could deprecate sensibleOnTop, preserve existing behavior if it's on but otherwise fix it, and use a new variable that does the same thing without existing behavior, but that seems overly complicated.

As it is, I'm currently just dealing with this by leaving sensibleOnTop off and putting sensible as the first entry in tmux.plugins, but this does leave me vulnerable to ordering issues if multiple modules set plugins. The real solution is to use multiple modules explicitly to force ordering of sensible to the top while leaving the rest of the plugins at the default behavior, but that's annoying to write and so I haven't. I just hope this doesn't bite me later.

IMO it would violate POLA if the module documentation shows defaults that don't actually apply unless you set them explicitly.

With defaultText you can write it to make it clear that this is the tmux default, as opposed to the actual value in Nix. Or just skip that and list the tmux default in the description.

EDIT: Having written this, I now felt compelled to actually fix my config to do plugins = mkMerge [ (mkBefore [ sensible ]) [ other plugins ] ]. Before this I didn't realize mkMerge actually worked on individual non-attrset attributes.

stale[bot] commented 2 years ago

Thank you for your contribution! I marked this issue as stale due to inactivity. If this remains inactive for another 7 days, I will close this issue. Please read the relevant sections below before commenting.

If you are the original author of the issue

* If this is resolved, please consider closing it so that the maintainers know not to focus on this. * If this might still be an issue, but you are not interested in promoting its resolution, please consider closing it while encouraging others to take over and reopen an issue if they care enough. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

If you are not the original author of the issue

* If you are also experiencing this issue, please add details of your situation to help with the debugging process. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

Memorandum on closing issues

If you have nothing of substance to add, please refrain from commenting and allow the bot to close the issue. Also, don't be afraid to manually close an issue, even if it holds valuable information.

Closed issues stay in the system for people to search, read, cross-reference, or even reopen – nothing is lost! Closing obsolete issues is an important way to help maintainers focus their time and effort.

lilyball commented 2 years ago

This still bothers me.

stale[bot] commented 2 years ago

Thank you for your contribution! I marked this issue as stale due to inactivity. Please be considerate of people watching this issue and receiving notifications before commenting 'I have this issue too'. We welcome additional information that will help resolve this issue. Please read the relevant sections below before commenting.

If you are the original author of the issue

* If this is resolved, please consider closing it so that the maintainers know not to focus on this. * If this might still be an issue, but you are not interested in promoting its resolution, please consider closing it while encouraging others to take over and reopen an issue if they care enough. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

If you are not the original author of the issue

* If you are also experiencing this issue, please add details of your situation to help with the debugging process. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

Memorandum on closing issues

Don't be afraid to manually close an issue, even if it holds valuable information. Closed issues stay in the system for people to search, read, cross-reference, or even reopen – nothing is lost! Closing obsolete issues is an important way to help maintainers focus their time and effort.

pimvh commented 1 year ago

Hi! I am loving home-manager so far, but I also find the default tmux configuration quite opinionated, which is frankly also an opinion 😄. Would you be open to a PR that wraps all the default configuration in a boolean flag that is set to true for backward compatibility?

EDIT: This statement is outright false. If you override the config file in xdg.configFile you can change anything your heart desires.

stale[bot] commented 11 months ago

Thank you for your contribution! I marked this issue as stale due to inactivity. Please be considerate of people watching this issue and receiving notifications before commenting 'I have this issue too'. We welcome additional information that will help resolve this issue. Please read the relevant sections below before commenting.

If you are the original author of the issue

* If this is resolved, please consider closing it so that the maintainers know not to focus on this. * If this might still be an issue, but you are not interested in promoting its resolution, please consider closing it while encouraging others to take over and reopen an issue if they care enough. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

If you are not the original author of the issue

* If you are also experiencing this issue, please add details of your situation to help with the debugging process. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

Memorandum on closing issues

Don't be afraid to manually close an issue, even if it holds valuable information. Closed issues stay in the system for people to search, read, cross-reference, or even reopen – nothing is lost! Closing obsolete issues is an important way to help maintainers focus their time and effort.

toonn commented 11 months ago

@pimvh, this is about the tmux module. In principle you can overwrite anything in $HOME pretty easily with HM. The module's just too opinionated.

pimvh commented 11 months ago

@pimvh, this is about the tmux module. In principle you can overwrite anything in $HOME pretty easily with HM. The module's just too opinionated.

Yes, indeed, they are different. For other people finding this issue the solution to get around this while using the home-manager module is doing this:

    tmux = {
      enable = true;
      sensibleOnTop = false;
      extraConfig = ''
        # Either source another config file, you push as a managed home-manager file (see xdg.Configfile)
        source-file ~/.config/tmux/your-config-file.conf

        # our just specify your tmux config here, if you like embedding config in nix.
        # .....
      ''

      # more tmux options here
      # ...
   };

As explained in text by @lilyball above.

stale[bot] commented 8 months ago

Thank you for your contribution! I marked this issue as stale due to inactivity. Please be considerate of people watching this issue and receiving notifications before commenting 'I have this issue too'. We welcome additional information that will help resolve this issue. Please read the relevant sections below before commenting.

If you are the original author of the issue

* If this is resolved, please consider closing it so that the maintainers know not to focus on this. * If this might still be an issue, but you are not interested in promoting its resolution, please consider closing it while encouraging others to take over and reopen an issue if they care enough. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

If you are not the original author of the issue

* If you are also experiencing this issue, please add details of your situation to help with the debugging process. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

Memorandum on closing issues

Don't be afraid to manually close an issue, even if it holds valuable information. Closed issues stay in the system for people to search, read, cross-reference, or even reopen – nothing is lost! Closing obsolete issues is an important way to help maintainers focus their time and effort.