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

kanata: init at 1.0.5 #182358

Merged
merged 3 commits into from
Jul 24, 2022
Merged

kanata: init at 1.0.5 #182358

merged 3 commits into from
Jul 24, 2022

Conversation

jian-lin
Copy link
Contributor

@jian-lin jian-lin commented Jul 21, 2022

Description of changes

https://github.com/jtroo/kanata

Closes #179096

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@jian-lin
Copy link
Contributor Author

cc @guy-who-googles

pkgs/tools/system/kanata/default.nix Outdated Show resolved Hide resolved
pkgs/tools/system/kanata/default.nix Outdated Show resolved Hide resolved
@jian-lin
Copy link
Contributor Author

About the coding style, let's hear from more people.

@jian-lin
Copy link
Contributor Author

Thanks. Changes have been made.

pkgs/tools/system/kanata/default.nix Outdated Show resolved Hide resolved
@jian-lin
Copy link
Contributor Author

jian-lin commented Jul 22, 2022

Another question: kanata can run command if cmd feature is enabled, which is not by default. Should we enable it at compile time?

Quoting the author from release notes:

(The cmd feature) is restricted behind conditional compilation because I consider the action to be a security risk that should be explicitly opted into and completely forbidden by default

However, even if cmd feature is enabled at compile time, to actually use this feature, a flag needs to be enabled in the config file.
IMO, for simplicity, we can enable the cmd feature at compile time(, which I have not done in this pr).

What do you think?

@azahi
Copy link
Member

azahi commented Jul 22, 2022

Should we enable it at compile time?

Looks like it just creates an additional binary called kanata_cmd_enabled. I see nothing wrong with just adding the flag by default.

You can also add an additional boolean as an input, something like { cmd ? false }, and modify compilation flags depending on its value. That way a user can easily override the package if that functionality is needed.

@SuperSandro2000
Copy link
Member

However, even if cmd feature is enabled at compile time, to actually use this feature, a flag needs to be enabled in the config file.
IMO, for simplicity, we can enable the cmd feature at compile time(, which I have not done in this pr).

Then I would just enable it by default.

@jtroo
Copy link

jtroo commented Jul 22, 2022

I think cmd should be disabled by default and enabled if desired to ensure safe defaults for users.

Here's a hypothetical scenario. A less technical user of kanata may ask for help somewhere random or download a kanata configuration from somewhere random. This untrusted config file could have cmd enabled and have a key do a curl command to do a download and then execute malware. The less technical user probably has no need for the cmd feature and may not know that they should have compiled kanata without the cmd feature.

@jian-lin
Copy link
Contributor Author

jian-lin commented Jul 22, 2022

A less technical user may ... download a kanata configuration from somewhere random

IMHO, nixpkgs users are not that less technical in general. But more security is also good.

I'm fine with both decisions.

Another question: if we gate cmd behind an option like enableCmd, is it worth providing another package in all-packages.nix like kanata-with-cmd = callPackage ../tools/system/kanata { enableCmd = true; }?

@jian-lin
Copy link
Contributor Author

  • license is updated.
  • cmd feature is gated by withCmd and is disabled by default

@azahi
Copy link
Member

azahi commented Jul 22, 2022

Result of nixpkgs-review pr 182358 run on x86_64-linux 1

1 package built:
  • kanata

Copy link
Member

@azahi azahi left a comment

Choose a reason for hiding this comment

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

LGTM but I have one tiny nitpick.

pkgs/tools/system/kanata/default.nix Outdated Show resolved Hide resolved
Co-authored-by: Azat Bahawi <azat+github@bahawi.net>
jian-lin added a commit to linj-fork/nixpkgs that referenced this pull request Jul 24, 2022
Co-authored-by: Azat Bahawi <azat+github@bahawi.net>
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
@jian-lin
Copy link
Contributor Author

I add an nixos module for kanata in #182756, you may be interested.

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.

kanata
4 participants