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

desktop-managers: do not leak feh to PATH #30286

Merged
merged 1 commit into from Oct 13, 2017
Merged

desktop-managers: do not leak feh to PATH #30286

merged 1 commit into from Oct 13, 2017

Conversation

lukateras
Copy link
Member

@lukateras lukateras commented Oct 10, 2017 •
edited

Motivation for this change

feh is used to set background image for desktop managers that do not support it directly, however there is no need to include it in PATH.

Fixes #17450.

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.

@lukateras
desktop-managers: do not leak feh to PATH
f9415cb 
feh is used to set background image for desktop managers that do not support it directly, however there is no need to include it in PATH.

Fixes NixOS#17450.
@peterhoeg peterhoeg merged commit f7ba92b into NixOS:master Oct 13, 2017
@adisbladis adisbladis mentioned this pull request Oct 13, 2017
Closed
8 tasks
@Profpatsch Profpatsch mentioned this pull request Oct 13, 2017
Merged
@Profpatsch
Copy link
Member

I’ve reverted the change, it’s not possible to assign something not called config or options to a module.

Please test changes before you PR.

@peterhoeg
Copy link
Member

My bad for merging without a local test on my side. It looked innocent enough (famous last words I guess).

@lukateras
Copy link
Member Author

Sorry, I too thought there should be no problem with doing that... I'll open a new pull request after I test the change.

@Profpatsch
Copy link
Member

Profpatsch commented Oct 13, 2017 •
edited

My bad for merging without a local test on my side. It looked innocent enough

Yeah, no problem.

@yegortimoshenko Thanks for the PR, will merge it when tested. :)

@michalrus
Copy link
Member

@peterhoeg, reopen #17450?…

@lukateras lukateras mentioned this pull request Oct 16, 2017
Closed
8 tasks
@lukateras lukateras deleted the patch-2 branch December 18, 2017 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lukateras @Profpatsch @peterhoeg @michalrus