dealii: init at 9.4.0 #181948
dealii: init at 9.4.0 #181948
Conversation
|
Result of 1 package built:
|
|
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/1045 |
| inherit version; | ||
|
|
||
| src = fetchzip { | ||
| url = "https://github.com/dealii/dealii/releases/download/v${version}/dealii-${version}.tar.gz"; |
There was a problem hiding this comment.
Why are we not fetching from git?
There was a problem hiding this comment.
I've read that fetchzip is now recommended instead of fetchFrom* for caching reasons. I'll change it then.
| let | ||
| version = "9.4.0"; | ||
|
|
||
| joinPkg = pkg: symlinkJoin { |
There was a problem hiding this comment.
What is this about? Why are we not using the packages ad normal?
There was a problem hiding this comment.
Could you elaborate a bit? My intention was to include both normal and dev outputs of some packages.
There was a problem hiding this comment.
Is this required? Normally this is only required in special exceptions. What is the problem you encountered?
Also if this is really required please just add both packages next to each other.
There was a problem hiding this comment.
You were correct, I needed to specify dev output only for a single dependency.
| ]; | ||
|
|
||
| postPatch = '' | ||
| for s in /build/source/doc/doxygen/scripts/*; do |
There was a problem hiding this comment.
postPatch goes right after src. Also you can point patchshebangs to a directory.
| , symlinkJoin | ||
| , withDebug ? false | ||
| , commonCxxFlags ? "" | ||
| , debugCxxFlags ? "" |
There was a problem hiding this comment.
Why do we need this? This can be easily done with overlays, can't it?
There was a problem hiding this comment.
This was for convenience. Adding CMake flags with spaces in their values isn't obvious (my question at discourse), so I wanted to add that to the derivation.
There was a problem hiding this comment.
but is that something that a regular user needs all the time or something rarely used?
There was a problem hiding this comment.
This library usually powers quite CPU-intensive software, so I may reasonably expect users to override flags with something like -march=znver3. A generic build wouldn't e.g. enable AVX, which can be used by the library and gives a noticeable performance boost.
There was a problem hiding this comment.
We should turn that on by default if stdenv.hostPlatform.avxSupport is true.
Also I think using overlays would be nicer and make the code less complex. Then we can also save that option entirely.
There was a problem hiding this comment.
We should turn that on by default if
stdenv.hostPlatform.avxSupportis true.
Isn't this option always false? On all machines I have access to with Nix it evaluates to false (as well as all SSE options), but all of them have sufficiently modern CPU.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks a lot, I'll look into that. Surprisingly, though, system-features cannot be overriden via --option on a NixOS system, but can be on a non-NixOS one. And do I understand correctly that these flags can only be set to both a package and its dependencies?
EDIT: by dependencies I mean both buildInputs and nativeBuildInputs.
There was a problem hiding this comment.
I am not super familiar with that. Best to ask on matrix or discourse.
There was a problem hiding this comment.
After some thinking, it's best to leave all custom build flag stuff out of the derivation and document overriding elsewhere, like on the wiki.
| postBuild = '' | ||
| make documentation | ||
| cp -r ${offlineDocs}/doc/doxygen/deal.II/images /build/source/build/doc/doxygen/deal.II | ||
| ''; |
There was a problem hiding this comment.
Using the absolute path in the sandbox is not allowed.
| doCheck = true; | ||
| preCheck = '' | ||
| export LD_LIBRARY_PATH=$(pwd)/lib:$LD_LIBRARY_PATH | ||
| export PATH=${mpi}/bin:$PATH |
| ''; | ||
|
|
||
| passthru = { | ||
| mpi = if withMPI then mpi else null; |
There was a problem hiding this comment.
This shouldn't be used to detect if it is build with MPI. It should never be null. A better way would be to passthru the boolean.
There was a problem hiding this comment.
Not really sure what do you mean. Is it something like this?
passthru = {
inherit withMPI mpi;
};
There was a problem hiding this comment.
yeah, thats nicer than having packages which are null.
There was a problem hiding this comment.
Okay, will change it then!
| the complex data structures and algorithms required. | ||
| ''; | ||
| homepage = "https://www.dealii.org/"; | ||
| downloadPage = "https://dealii.org/download.html"; |
There was a problem hiding this comment.
Please point this to GitHub releases where we do wliaf the files from.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/writing-a-derivation-to-support-optimized-builds/20980/3 |
Description of changes
deal.II is a comprehensive C++ library for solving partial differential equations focused on finite element methods.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes