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

ckb: fix missing modprobe dependency #33287

Merged
merged 1 commit into from Jan 22, 2018
Merged

ckb: fix missing modprobe dependency #33287

merged 1 commit into from Jan 22, 2018

Conversation

Nadrieril
Copy link
Member

Motivation for this change

When plugging a new device, the service tries to run modprobe and fails. This PR adds modprobe to the path of the service to fix this.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

bjornfor
bjornfor requested changes Jan 1, 2018
View reviewed changes
Copy link
Contributor

@bjornfor bjornfor left a comment

Choose a reason for hiding this comment

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

Please change commit prefix from "ckb service:" to "nixos/ckb:".

@kierdavis
Copy link
Contributor

modprobe is called by the ckb package, not the service that manages it, so the change should be made there. In other words, this PR fixes the issue only when ckb-daemon is run by systemd rather than if it is run by any other means.

There only seems to be one occurrence of modprobe in the ckb source tree:

$ grep -r modprobe
src/ckb-daemon/usb_linux.c:    if(system("modprobe uinput") != 0)

It looks like this can be fixed with a simple patch to the source, changing modprobe to the absolute pathname of the binary.

@Nadrieril Nadrieril changed the title ckb service: fix missing modprobe dependency ckb: fix missing modprobe dependency Jan 9, 2018
@Nadrieril
Copy link
Member Author

You're right. I patched the package instead.

@kierdavis
Copy link
Contributor

Thanks. The diff looks fine; I'll test this later tonight when I'm in front of a computer with the appropriate hardware.

@kierdavis
Copy link
Contributor

Looks good to me - with this change no sh: modprobe: command not found message is logged by ckb-daemon when it starts up.

@kierdavis
Copy link
Contributor

This is also a bug in the current release, so it should probably be cherry-picked into release-17.09.

@Nadrieril
Copy link
Member Author

I don't know who has the rights to merge, but this is mergeable

@Nadrieril Nadrieril mentioned this pull request Jan 22, 2018
Merged
8 tasks
@7c6f434c 7c6f434c merged commit 9593ead into NixOS:master Jan 22, 2018
@Nadrieril Nadrieril deleted the fix-ckb-service branch February 10, 2018 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjornfor bjornfor bjornfor requested changes

@kierdavis kierdavis kierdavis approved these changes

Assignees
No one assigned
Labels
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.

None yet
5 participants
@Nadrieril @kierdavis @bjornfor @7c6f434c @GrahamcOfBorg