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

openldap: load configuration files from global sysconfdir /etc #21648

Merged
merged 1 commit into from Jan 10, 2017
Merged

openldap: load configuration files from global sysconfdir /etc #21648

merged 1 commit into from Jan 10, 2017

Conversation

outergod
Copy link
Contributor

@outergod outergod commented Jan 4, 2017 •
edited

Motivation for this change

openldap is currently built so that the client programs only ever read the sample configuration file in $out/etc/openldap/ldap.conf. The path is fixed and can't be changed via runtime parameters, while some options can only be set through that file. This simple change solves the problem by separating the install and runtime sysconfdirs so that environment.etc."openldap/ldap.conf" can be used to configure openldap.

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
    • 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

@e-user, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @chaoflow and @DerTim1 to be potential reviewers.

DerTim1
DerTim1 approved these changes Jan 5, 2017
View reviewed changes
Copy link

@DerTim1 DerTim1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with NixOS and a configuration for an openldap-server.

Why is out in brackets (see installflags)?

@outergod
Copy link
Contributor Author

outergod commented Jan 5, 2017

$(foo) is expanded late, by Make. It seems to be convention to use in case of Make flags in nixpkgs, just by looking at the number of matches in the package tree.

I wonder why the build fails, though, I can't reproduce it.

@DerTim1
Copy link

DerTim1 commented Jan 5, 2017

You mean the travis-ci builds? Openldap is dependency for a huge amount of packages. I think the check will try to build all of them and something different went wrong.

@outergod
Copy link
Contributor Author

outergod commented Jan 5, 2017 •
edited

/tmp/nix-build-samba-4.5.3.drv-0/samba-4.5.3/librpc/idl/auth.idl:107: error: Unable to determine origin of type struct cli_credentials

AFAICT this is pretty much unrelated to my change.

@outergod
Copy link
Contributor Author

outergod commented Jan 7, 2017

Any idea what I can do?

@DerTim1
Copy link

DerTim1 commented Jan 9, 2017

The travis-ci tests failed because of "No disk space left" and "size of logfile exceeded". So I think we don't need to worry about these failures.

@DerTim1
Copy link

DerTim1 commented Jan 9, 2017 •
edited

You can try to use nox-review to build all dependent packages. This failed on my virtual machine with 16 GB diskspace. I think you need much more space to test the huge amount of packages with openldap as dependencies.
nix-shell -p nox --run "nox-review pr 21648"

Nevertheless I think your PR is ok.

fpletz
fpletz approved these changes Jan 9, 2017
View reviewed changes
Copy link
Member

@fpletz fpletz left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 but not sure if we should push this to staging first?

@Mic92
Copy link
Member

Mic92 commented Jan 9, 2017

@vcunat ^ staging?

@Mic92
Copy link
Member

Mic92 commented Jan 9, 2017 •
edited

Should be merged together with: #21764

@Mic92 Mic92 mentioned this pull request Jan 9, 2017
Closed
7 tasks
@grahamc
Copy link
Member

grahamc commented Jan 10, 2017

Out of curiosity, why load it from /etc, instead of a configuration in the nix store? This seems a bit antithetical to NixOS.

@Mic92
Copy link
Member

Mic92 commented Jan 10, 2017 •
edited

@grahamc see pull request description.

@grahamc
Copy link
Member

grahamc commented Jan 10, 2017

Sorry :/ I missed The path is fixed and can't be changed via runtime parameters!

@vcunat
Copy link
Member

vcunat commented Jan 10, 2017

Yes, better staging – my tool shows >4k rebuilds just on x86_64-linux.

@vcunat vcunat changed the base branch from master to staging January 10, 2017 16:39
@vcunat vcunat merged commit db90ac6 into NixOS:staging Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpletz fpletz fpletz approved these changes

@DerTim1 DerTim1 DerTim1 approved these changes

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
7 participants
@outergod @mention-bot @DerTim1 @Mic92 @grahamc @vcunat @fpletz