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

services.xserver.imwheel: Fix default extraOptions #76054

Merged
merged 1 commit into from Dec 20, 2019

Conversation

turboMaCk
Copy link
Member

@turboMaCk turboMaCk commented Dec 19, 2019

Motivation for this change

related to #71052 it seems there was a mistake made during refactoring. escapeShellArgs wraps flags to '' which results in imwheel not being able to parse them.

Things done

Replace escapeShellArgs with concatStringsSep " " which fixes the problem.
Change default extraOptions to be compatible with escapeShellArgs.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @infinisil sorry that this sliped.

@infinisil
Copy link
Member

escapeShellArgs should be preferrable because it escapes things properly. If you need to pass --buttons 45 you can do so with either [ "--buttons" "45" ] or [ "--buttons=45" ]

@turboMaCk turboMaCk changed the title services.xserver.imwheel: Fix argument escaping services.xserver.imwheel: Fix default extraOptions Dec 20, 2019
@turboMaCk
Copy link
Member Author

@infinisil changed inlast commit though I have a feeling it might be quite unobvious behaviour for the user.

@infinisil
Copy link
Member

Hm yeah I know what you mean. Maybe it doesn't matter here since I don't think imwheel has any options that could need escaping (as of now anyways), but I think we should strive for proper escaping by default.

@turboMaCk
Copy link
Member Author

turboMaCk commented Dec 20, 2019

I think in cases like this (and especially for service that won't be that widely used probably) the consistency is the most important thing. So if this is standard for services in nixpkgs then I believe it's right thing to do. I was more just making excuse why I instinctively removed the escaping rather than changed the defaults. Thanks for the feedback 💯

@turboMaCk
Copy link
Member Author

turboMaCk commented Dec 20, 2019

Maybe it doesn't matter here since I don't think imwheel has any options that could need escaping (as of now anyways)

I think there is still valid argument that allowing this will teach folks to expect this format to work across services. Then in cases where service really does need to have arguments escaped they would shoot their foot expecting this format to work. I would rather guide towards single consistent behaviour apliable to all cases and avoid fragmentation of different behaviour which I belive is already a bit complicated to manage in nixpkgs.

TL;DR: after thinking about it I think I was wrong and you're right.

@infinisil infinisil merged commit 8c39d69 into NixOS:master Dec 20, 2019
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

2 participants