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

@Infinisil Infinisil commented Dec 19, 2019

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 force-pushed the turboMaCk:imwheel-fixes branch from cd28908 to 66eef90 Dec 20, 2019
@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

@turboMaCk turboMaCk commented Dec 20, 2019

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

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Dec 20, 2019

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 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 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
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.