-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
python3Packages.dlib: Inherit stdenv and build flags from main derivation, fix tests #273665
base: master
Are you sure you want to change the base?
Conversation
# Pass CMake flags through to the build script | ||
preConfigure = '' | ||
setupPyBuildFlags="$setupPyBuildFlags $(sed 's/-D\(\S*\)/--set \1/g' <<< "$cmakeFlags")" |
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.
I'm ok with this, I guess. But to add more options:
- a
for
-loop would avoid spawning subprocesses, and--set ${flag#-D}
might be more readable thansed
- one could also patch the upstream scripts e.g. like so:
nixpkgs/pkgs/development/python-modules/torchaudio/0001-setup.py-propagate-cmakeFlags.patch
Line 4 in b576bc2
Subject: [PATCH] setup.py: propagate cmakeFlags
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.
The sed
command also filters for -D
prefixes, in case there are other items in cmakeFlags
. I'll keep it as is for simplicity.
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.
I am not okay with this. Please use something simpler and easier.
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.
Done - now using a for loop.
); | ||
|
||
checkPhase = '' | ||
${python.interpreter} nix_run_setup test |
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.
Any advantage using "${python.interpreter}"
over "python"
?
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.
Not sure; it was like this already.
I'm pretty sure we should be using pytestCheckHook
anyway.
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.
When rebasing please be aware that this way of running python tests will be removed in the current staging run and pytestCheckHook should be used
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.
Done
]; | ||
doCheck = !( | ||
# The tests attempt to use CUDA on the build platform. | ||
dlib.cudaSupport |
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.
Is there a way to disable just a subset of tests?
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.
Probably, but as each build takes 10-15 minutes on my laptop I don't have the time to determine which fail
I fell out of this for a bit... what's the state of this PR? Should look into merging this or into closing this? EDIT: In retrospective, it's ok to address the |
Sorry for the delay - I've been meaning to make changes based on your feedback, but haven't found the time. If #279927 adds the Python support and non-cudatoolkit-ness implemented here I'm happy to close it. Otherwise I can hopefully finish it sometime this week. My main concern re |
b239fd4
to
9536857
Compare
I have updated this branch to work with recent changes in #279927. It is now only changing the Python side of things. |
checkPhase = '' | ||
${python.interpreter} nix_run_setup test | ||
''; | ||
} // lib.optionalAttrs dlib.cudaSupport { stdenv = dlib.cudaPackages.backendStdenv; }) |
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.
Please inline this
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.
Done
9536857
to
3e1d45d
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4631 |
Description of changes
This PR updates dlib's Python derivation to use the same build flags as the C library derivation it's based on. Notably, this fixes CUDA support and removes duplication of optional feature logic.
Tests are also fixed in this PR by switching to
pytestCheckHook
. At the moment, builds are failing onmaster
.Closes #243880.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.