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

networkmanager: fix dispatcher scripts #24507

Merged
merged 1 commit into from Apr 9, 2017
Merged

networkmanager: fix dispatcher scripts #24507

merged 1 commit into from Apr 9, 2017

Conversation

gkleen
Copy link
Contributor

@gkleen gkleen commented Mar 31, 2017

Motivation for this change

networkmanager.nix used source to mean text and wrote dispatcher scripts with the default mode (0666), which means networkmanager wouldn't call them.

Things done

Adjusted networkmanager.dispatchScripts to more closely match the behaviour of environment.etc, taking source to be a path.

  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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.

@mention-bot
Copy link

@pngwjpgh, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rickynils, @domenkozar and @lethalman to be potential reviewers.

@gkleen
Copy link
Contributor Author

gkleen commented Apr 4, 2017

@rickynils, @domenkozar, @lethalman, bump anything standing in the way of this getting merged?

@joachifm
Copy link
Contributor

joachifm commented Apr 7, 2017

Hm, I wonder what the original intent was here. The latter part seems to suggest that users were supposed to be able to declare the script text directly but that obviously clashes with the use of source.

This change makes sense to me, anyway, so if nobody objects I'll integrate it soon.

@joachifm
Copy link
Contributor

joachifm commented Apr 7, 2017

If I understand correctly, the feature is broken as-is, so perhaps this ought to be picked to the relase branch as well.

@joachifm
Copy link
Contributor

joachifm commented Apr 7, 2017

Perhaps consider changing the subject to something like "networkmanager: fix dispatcher scripts" that more readily communicates that this fixes a bug (and is not just a refactor/cosmetic thing).

@gkleen gkleen changed the title networkmanager: use source as in environment.etc networkmanager: fix dispatcher scripts Apr 7, 2017
@joachifm joachifm merged commit 773c456 into NixOS:master Apr 9, 2017
@joachifm
Copy link
Contributor

joachifm commented Apr 9, 2017

Thank you :)

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
0.kind: bug 6.topic: nixos
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gkleen @mention-bot @joachifm