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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/espanso: add wayland and package option #285138

Merged
merged 1 commit into from May 19, 2024

Conversation

michaelBelsanti
Copy link
Contributor

This adds two options to the espanso module. One that enables wayland support by using the espanso-wayland package, and a package option.

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.05 Release Notes (or backporting 23.05 and 23.11 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.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3370

@SuperSandro2000
Copy link
Member

We cannot compile x11 and wayland at the same time? :(

Comment on lines 12 to 13
wayland = mkOption {
type = types.bool;
default = false;
description = lib.mdDoc "Use the Wayland build of Espanso.";
};
Copy link
Member

Choose a reason for hiding this comment

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

It would probably cleaner to add the wayland variant to the related options of the package option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So create a services.espanso.package.wayland option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would probably cleaner to add the wayland variant to the related options of the package option

@SuperSandro2000 could you explain exactly what you meant here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to use a second variable (cfg.wayland) as opposed to having users specify package = pkgs.espanso-wayland? It seems like the latter is more common and a little less complex.

Copy link
Member

Choose a reason for hiding this comment

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

what @n8henrie said

mkPackageOption takes a related packages argument and we can write that there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to have a wayland option so it would be a bit more discoverable for users when enabling espanso, but a package option with the example set is good enough.

@michaelBelsanti
Copy link
Contributor Author

We cannot compile x11 and wayland at the same time? :(

Unfortunately does not seem possible with espanso atm.

@michaelBelsanti
Copy link
Contributor Author

dropped the mainProgram commit, looks like it got merged already

@SuperSandro2000 SuperSandro2000 merged commit 8036f1f into NixOS:master May 19, 2024
21 checks passed
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

5 participants