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

sway: do not use pkgs.sway when cfg.package = null #5309

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

amarshall
Copy link
Contributor

Description

  • Implicitly disable checkConfig when cfg.package = null as we don’t
    have any exe to use for the check
  • Implicitly disable swaymsg reload on activation, since we have no
    exe to use for running it

See #5307

Checklist

Need to think about if there is a good test to write for this…I did run existing Sway tests and all OK.

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

- Implicitly disable checkConfig when `cfg.package = null` as we don’t
  have any exe to use for the check
- Implicitly disable `swaymsg reload` on activation, since we have no
  exe to use for running it

See nix-community#5307
@ElysaSrc
Copy link

ElysaSrc commented Apr 21, 2024

Why couldn't we use the osConfig to get the value ?
Locally, to keep both features I've done this:

{
  config,
  lib,
  pkgs,
  customPkgs,
  osConfig,
  ...
}: {
  wayland.windowManager.sway = {
    enable = true;
    package = osConfig.programs.sway.package;
    extraSessionCommands = osConfig.programs.sway.extraSessionCommands;
    # ...
  };
};

@amarshall
Copy link
Contributor Author

IMO, that should be something the user decides to do themselves, not something implicit in the default config. There’s also no precedent for consuming osConfig in HM.

@ElysaSrc
Copy link

IMO, that should be something the user decides to do themselves, not something implicit in the default config. There’s also no precedent for consuming osConfig in HM.

I was suggesting it because the documentation says:

Sway package to use. Will override the options ‘wrapperFeatures’, ‘extraSessionCommands’, and ‘extraOptions’. Set to null to not add any Sway package to your path. This should be done if you want to use the NixOS Sway module to install Sway.

But I do understand why we shouldn't consume it then.

I haven't had time to check the PR, I'll check it later. For now, I just use osConfig.

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

3 participants