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

Jump to bottom

msmtp: Configure sysconfdir to point to /etc #54316

Merged
merged 1 commit into from Jan 23, 2019
Merged

msmtp: Configure sysconfdir to point to /etc #54316

merged 1 commit into from Jan 23, 2019

Conversation

kampka
Copy link
Contributor

@kampka kampka commented Jan 19, 2019

Motivation for this change

The current build lets the SYSCONFDIR of msmtp point to the nix store /nix/.../msmtp-1.81/etc, which is not very useful.
This change will allow for system wide configuration to be placed in /etc instead.

I only tested this one using an overlay because I believe it to be trivial, can do more tests if requested.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@peterhoeg
Copy link
Member

And make install doesn't try to put something in there?

@kampka
Copy link
Contributor Author

kampka commented Jan 19, 2019

msmtp does not ship a default config or something that would be put there if that is what you are asking about, so no, nothing is installed into /etc, the path is just compiled into the binary.

@hedning
Copy link
Contributor

hedning commented Jan 19, 2019

This is a nit pick, but it's good to suffix commit messages with the package attribute path, ie. mstmp: in this case.

@kampka kampka changed the title Configure sysconfdir to point to /etc msmtp: Configure sysconfdir to point to /etc Jan 20, 2019
@kampka
msmtp: Configure sysconfdir to point to /etc
25acd14 
The current build lets the SYSCONFDIR of msmtp point to the nix store /nix/.../msmtp-1.81/etc, which is not very useful.
This change will allow for system wide configuration to be placed in /etc instead.
@kampka
Copy link
Contributor Author

kampka commented Jan 20, 2019

@hedning valid point, fixed

@hedning
Copy link
Contributor

hedning commented Jan 20, 2019 •
edited

@GrahamcOfBorg build msmtp

@hedning
Copy link
Contributor

hedning commented Jan 20, 2019

--sysconfdir=/etc seems like a common pattern 👍

@peterhoeg
Copy link
Member

--sysconfdir=/etc seems like a common pattern

It blows up spectacularly if the installPhase tries to put something in there of course which is the reason it's not part of the standard builder.

@infinisil infinisil merged commit a96ff61 into NixOS:master Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterhoeg peterhoeg Awaiting requested review from peterhoeg

@garbas garbas Awaiting requested review from garbas

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kampka @peterhoeg @hedning @infinisil @GrahamcOfBorg