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.

Sign up for GitHub

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

Jump to bottom

nixos/ydotool: module init #191911

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

nixos/ydotool: module init #191911

wants to merge 1 commit into from

Conversation

kraem
Copy link
Member

@kraem kraem commented Sep 19, 2022 •
edited

Description of changes

Resolves #183659

As ydotool supports text consoles i put wantedBy = "default.target" instead of "graphical-session.target"

I don't think this is realated but nixpkgs-review fails:
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD

nixos-install-tools is blacklisted in nixpkgs-review

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.

@kraem
Copy link
Member Author

kraem commented Sep 20, 2022

Sorry i didn't check existing PR;s before but found #107210 just now.

@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/1724

@kraem
Copy link
Member Author

kraem commented Jan 18, 2023

Removed the wrapping of the binaries with YDOTOOL_SOCK=$XDG_RUNTIME_DIR as it's patched upstream: ReimuNotMoe/ydotool@36cc6db

@kraem kraem force-pushed the kraem/ydotool_module_init branch 3 times, most recently from 3413f38 to 95733fa Compare January 18, 2023 21:32
SuperSandro2000
SuperSandro2000 reviewed Jan 18, 2023
View reviewed changes
nixos/doc/manual/release-notes/rl-2305.section.md
@@ -26,6 +26,8 @@ In addition to numerous new and upgraded packages, this release has the followin

- [fzf](https://github.com/junegunn/fzf), a command line fuzzyfinder. Available as [programs.fzf](#opt-programs.fzf.fuzzyCompletion).

- [ydotool](https://github.com/ReimuNotMoe/ydotool), Generic command-line automation tool. Available as [programs.ydotool](#opt-programs.ydotool.enable).
Copy link
Member

Choose a reason for hiding this comment

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

What does Generic command-line automation tool mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've taken the description from upstream. I think what they're trying to convey is that it works under wayland, xorg, fbdev, ..

I can of course change it if it's not adequate

SuperSandro2000
SuperSandro2000 reviewed Jan 18, 2023
View reviewed changes
nixos/modules/programs/ydotool.nix Outdated

systemd.user.services.ydotoold = {
description = "Starts ydotoold service";
wantedBy = [ "defaul.target" ];
Copy link
Member

Choose a reason for hiding this comment

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

This should be multi-user.target

Copy link
Member Author

@kraem kraem Jan 19, 2023 •
edited

Choose a reason for hiding this comment

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

From what i've gathered it should be default.target for user services?

edit: i fixed the typo in the meantime

Copy link
Contributor

Choose a reason for hiding this comment

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

default.target should be correct, multi-user.target is only for system services.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it hit me just now that this depends on my user being in the group input. Maybe this should be changed to a system wide service and be ran as root instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

As the ydotool client uses XDG_RUNTIME_DIR i didn't change it back to a system service, but added documentation about the user needing to be in the input group.

@kraem kraem force-pushed the kraem/ydotool_module_init branch 3 times, most recently from aa07634 to e0f8aaa Compare January 19, 2023 18:17
@kraem kraem force-pushed the kraem/ydotool_module_init branch from e0f8aaa to a6a1683 Compare March 4, 2023 17:17
talyz
talyz reviewed Mar 6, 2023
View reviewed changes
Copy link
Contributor

@talyz talyz left a comment

Choose a reason for hiding this comment

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

Looks great! I've tried it in a VM and it works well!

In addition to adding the options we discussed offline, I only have two very minor nitpicks: as it's technically a service, it should be in the services options category and the service description should be a short description of the program it's running.

nixos/modules/programs/ydotool.nix
{

options = {
programs.ydotool = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
programs.ydotool = {
services.ydotool = {

nixos/doc/manual/release-notes/rl-2305.section.md
@@ -34,6 +34,8 @@ In addition to numerous new and upgraded packages, this release has the followin

- [fzf](https://github.com/junegunn/fzf), a command line fuzzyfinder. Available as [programs.fzf](#opt-programs.fzf.fuzzyCompletion).

- [ydotool](https://github.com/ReimuNotMoe/ydotool), Generic command-line automation tool. Available as [programs.ydotool](#opt-programs.ydotool.enable).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [ydotool](https://github.com/ReimuNotMoe/ydotool), Generic command-line automation tool. Available as [programs.ydotool](#opt-programs.ydotool.enable).
- [ydotool](https://github.com/ReimuNotMoe/ydotool), Generic command-line automation tool. Available as [services.ydotool](#opt-services.ydotool.enable).

nixos/modules/programs/ydotool.nix
environment.systemPackages = [ pkgs.ydotool ];

systemd.user.services.ydotoold = {
description = "Starts ydotoold service";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description = "Starts ydotoold service";
description = "Daemon for ydotool";

@wentasah
Copy link
Contributor

For this to work, I had to add hardware.uinput.enable = true; and put myself into uinput group rather than into input.

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@OPNA2608 OPNA2608 mentioned this pull request Apr 13, 2024
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@talyz talyz talyz left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add programs.ydotool.enable config option to run required ydotoold daemon
6 participants
@kraem @nixos-discourse @wentasah @talyz @SuperSandro2000 @wegank