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

transmission: refactor preStart into script; add test #37329

Closed
wants to merge 3 commits into from
Closed

transmission: refactor preStart into script; add test #37329

wants to merge 3 commits into from

Conversation

coreyoconnor
Copy link
Contributor

@coreyoconnor coreyoconnor commented Mar 18, 2018 •
edited

Motivation for this change
  1. preStart script exceeded reasonable line length.
  2. no logging from preStart script made debugging permission issues difficult.
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.

jtojnar
jtojnar reviewed Mar 18, 2018
View reviewed changes
nixos/modules/services/torrent/transmission.nix Outdated
@@ -21,6 +21,16 @@ let

# for users in group "transmission" to have access to torrents
fullSettings = { umask = 2; download-dir = downloadDir; incomplete-dir = incompleteDir; } // cfg.settings;

preStart = pkgs.writeScript "transmission-pre-start" ''
#!${pkgs.runtimeShell}
Copy link
Contributor

@jtojnar jtojnar Mar 18, 2018 •
edited

Choose a reason for hiding this comment

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

Could you indent this? '' strings will correctly remove the Nix indentation.

bjornfor
bjornfor reviewed Mar 18, 2018
View reviewed changes
nixos/modules/services/torrent/transmission.nix Outdated
preStart = pkgs.writeScript "transmission-pre-start" ''
#!${pkgs.runtimeShell}
set -ex
for DIR in ${homeDir} ${settingsDir} ${fullSettings.download-dir} ${fullSettings.incomplete-dir}; do
Copy link
Contributor

Choose a reason for hiding this comment

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

To make this future proof against overlong lines, the list of DIRs can be placed on separate lines :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Though I think I did one better (and easier actually): represent the list in nix. Reflect to shell.

Also added a test for even extra future proofing haha.

@coreyoconnor coreyoconnor changed the title transmission: refactor preStart into script. transmission: refactor preStart into script; add test Mar 19, 2018
@coreyoconnor
Copy link
Contributor Author

Example of output from test, showing logging of pre-start commands.

I'm a supporter of the verbose logging but not sure if it's standard.

machine# [   12.136757] systemd[1]: Starting Transmission BitTorrent Service...
machine# [   12.188112] 9pdd8kpa2p2i7rl1v63whjnxv93n2qqc-transmission-pre-start[651]: + for DIR in '/var/lib/transmission' '/var/lib/transmission/.config/transmission-daemon' '/var/lib/transmission/Downloads' '/var/lib/transmission/.incomplete'
machine# [   12.209280] 9pdd8kpa2p2i7rl1v63whjnxv93n2qqc-transmission-pre-start[651]: + mkdir -p /var/lib/transmission
machine# [   12.227633] 9pdd8kpa2p2i7rl1v63whjnxv93n2qqc-transmission-pre-start[651]: + chmod 770 /var/lib/transmission
machine# [   12.242598] 9pdd8kpa2p2i7rl1v63whjnxv93n2qqc-transmission-pre-start[651]: + for DIR in '/var/lib/transmission' '/var/lib/transmission/.config/transmission-daemon' '/var/lib/transmission/Downloads' '/var/lib/transmission/.incomplete'
machine# [   12.262423] 9pdd8kpa2p2i7rl1v63whjnxv93n2qqc-transmission-pre-start[651]: + mkdir -p /var/lib/transmission/.config/transmission-daemon
machine# [   12.285629] dhcpcd[653]: dev: loaded udev
machine# [   12.297089] systemd[1]: Started Permit User Sessions.
machine# [   12.307783] 9pdd8kpa2p2i7rl1v63whjnxv93n2qqc-transmission-pre-start[651]: + chmod 770 /var/lib/transmission/.config/transmission-daemon
machine# [   12.326262] 9pdd8kpa2p2i7rl1v63whjnxv93n2qqc-transmission-pre-start[651]: + for DIR in '/var/lib/transmission' '/var/lib/transmission/.config/transmission-daemon' '/var/lib/transmission/Downloads' '/var/lib/transmission/.incomplete'
machine# [   12.342075] 9pdd8kpa2p2i7rl1v63whjnxv93n2qqc-transmission-pre-start[651]: + mkdir -p /var/lib/transmission/Downloads
machine# [   12.353131] 9pdd8kpa2p2i7rl1v63whjnxv93n2qqc-transmission-pre-start[651]: + chmod 770 /var/lib/transmission/Downloads
machine# [   12.366193] 9pdd8kpa2p2i7rl1v63whjnxv93n2qqc-transmission-pre-start[651]: + for DIR in '/var/lib/transmission' '/var/lib/transmission/.config/transmission-daemon' '/var/lib/transmission/Downloads' '/var/lib/transmission/.incomplete'
machine# [   12.374452] 9pdd8kpa2p2i7rl1v63whjnxv93n2qqc-transmission-pre-start[651]: + mkdir -p /var/lib/transmission/.incomplete
machine# [   12.378337] 9pdd8kpa2p2i7rl1v63whjnxv93n2qqc-transmission-pre-start[651]: + chmod 770 /var/lib/transmission/.incomplete
machine# [   12.383151] 9pdd8kpa2p2i7rl1v63whjnxv93n2qqc-transmission-pre-start[651]: + cp -f /nix/store/zchiknkjz1r9yah6jy8lafbk8qndj0k3-settings.json /var/lib/transmission/.config/transmission-daemon/settings.json
 Prompts.
machine# [   12.409179] systemd[1]: Started Transmission BitTorrent Service.

@coreyoconnor
Copy link
Contributor Author

ping.

@bjornfor
Copy link
Contributor

Some nitpicks:

  • The 2nd commit is logically a fixup. Please squash.
  • Commits touching NixOS should have "nixos" prefix. E.g. "nixos/transmission: refactor".
  • Commit subject lines should not end with a dot.

@coreyoconnor
Copy link
Contributor Author

Updated.

I'm curious: what's the reasoning behind "subject lines should not end with a dot"?

@bjornfor
Copy link
Contributor

bjornfor commented Apr 5, 2018

It's just common practice. Similar to email subject lines and chapter titles in books.

Also it saves one char :-)

@bjornfor
Copy link
Contributor

bjornfor commented Apr 5, 2018

Applied to master (9eec034, c0de245, d0d0502). Thanks!

@bjornfor bjornfor closed this Apr 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar left review comments

@bjornfor bjornfor bjornfor approved these changes

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@coreyoconnor @bjornfor @jtojnar @GrahamcOfBorg