openssl: Enable tests, add missing patch comments#443190
openssl: Enable tests, add missing patch comments#443190infinisil wants to merge 2 commits intoNixOS:masterfrom
Conversation
nixpkgs-ci
bot
added
10.rebuild-darwin: 0
There was a problem hiding this comment.
I don't see doCheck = false in the main derivation though;
so doesn't building it by default do the checks as the test?
There was a problem hiding this comment.
Contrary to popular belief, doCheck is false by default for mkDerivation 🙃
There was a problem hiding this comment.
Oh...
Yes contrary to my belief because I've had to explicitly set it to false at times.. maybe other derivation builders buildPythonPackage have it set to true.
What's the difference between setting doCheck and the approach here in tests for my edification?
There was a problem hiding this comment.
Is it :
Could in theory also enable the tests for all builds, but it makes the build take a lot longer (13 instead of 5 minutes for the default version on my machine)
We run tests not for every build variant?
There was a problem hiding this comment.
Yeah indeed, it's true by default for buildPythonPackage, although it's kind of more doInstallCheck there.
What's the difference between setting doCheck and the approach here in tests for my edification?
It avoids having to run the tests (which almost triple the build time) when doing stdenv rebuilds, while still running the tests in ofborg PRs and allowing to run them on demand. Not a big fan of doing this tbh, but we can still enable them always later if wanted.
There was a problem hiding this comment.
OpenSSL is only part of stdenv on Darwin and LLVM is far more of a serial bottleneck there. If these tests pass in the sandbox I’m fine enabling them in the main derivation.
There was a problem hiding this comment.
Alright then, done now. Confirmed that it gives the same drv hashes as with the .overrideAttrs before.
nixpkgs-ci
bot
added
the
12.approvals: 1
Makes the build take about 13 instead of 5 minutes
infinisil
force-pushed
the
openssl-tests-patches
branch
from
346432f to
a31a53e
Compare
nixpkgs-ci
bot
added
10.rebuild-linux: 501+
infinisil
changed the title
tests.withChecks to run tests, add missing patch comments
| hash = "sha256-39135OobV/86bb3msL3D8x21rJnn/dTq+eH7tuwtuM4="; | ||
|
|
||
| patches = [ | ||
| # Support for NIX_SSL_CERT_FILE, motivation unclear: |
There was a problem hiding this comment.
Motivation for this patch is given here: 942dbf8
|
LGTM after adapting comment for |
|
Thanks for the follow-up PR! |
4 participants
NIX_SSL_CERT_FILEThings done
passthru.tests.Add a 👍 reaction to pull requests you find important.