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
openldap: fix build if openssl or cyrus_sasl are overridden to null, add flag for cyrus_sasl, require openssl #108046
openldap: fix build if openssl or cyrus_sasl are overridden to null, add flag for cyrus_sasl, require openssl #108046
Conversation
da71366
to
865dcae
Compare
Other than that LGTM. |
fc11023
to
52df434
Compare
bbd87c0
to
4a5d3be
Compare
@sternenseemann you did not rebase cleanly and left a merge conflict in. |
Note that compiling without openssl is only a theoretical possibility since we have no alternative crypto backends supported by the package.
4a5d3be
to
ffaac7b
Compare
Should be fine now. |
* instead of cyrus_sasl == null, withCyrusSasl = false is now the proper way to disable sasl support for openldap. * the ability to compile without openssl has been removed, since openssl is required as the package needs it for libcrypto.
ffaac7b
to
d16b556
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than that the diff LGTM. @lovek323
* Quote some shell arguments * Make rm fail if openldap ever ceases to install var to $out
|
Also cross compiles to aarch64-multiplatform. |
rm -r libraries/*/.libs | ||
rm -r contrib/slapd-modules/passwd/*/.libs | ||
for f in "$out/lib/libldap.la" "$out/lib/libldap_r.la"; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for f in "$out/lib/libldap.la" "$out/lib/libldap_r.la"; do | |
for f in $out/lib/libldap.la $out/lib/libldap_r.la; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove quoting? This is really bizarre bikeshedding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it is not required and most parts of nixpkgs is not doing it.
What GitHub merge method did you use? Apparently this is one where it loses all commit messages except for the (in this case slightly out of date) PR title and number? Would be nice to use one which preserves the commit messages even when squashing. |
Squashing. The is no never method to squash and keep the title. I would need to force push that to your branch. |
@SuperSandro2000 Please don’t squash PRs with sensible commits. |
Rebase them instead |
Alternatively, squash them locally to keep all descriptions and then push the squashed commit before merging. All extended descriptions that @sternenseemann wrote just went away. |
git sometimes takes 10 minutes to force push other peoples branches and runs out of memory. I am to lazy to wait for that. |
Motivation for this change
Compiling
openldap.override { cyrus_sasl = null; }
results in an evaluation error.Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)cc @lovek323