-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
geant4: 10.5.1 -> 10.6.0 #78775
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
geant4: 10.5.1 -> 10.6.0 #78775
Conversation
However, this package I don't believe is quite ready to be merged, because when I install it a shell-hook error message is complaining about the lack of Also this pull request changed |
pkgs/development/libraries/physics/geant4/bash-variable-fix.patch
Outdated
Show resolved
Hide resolved
|
||
# Because you compile against Geant4, it's useful to have all the libs you'd | ||
# want available | ||
propagatedBuildInputs = [ clhep expat zlib libGLU libGL xlibsWrapper libXmu findutils ] |
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 propagatedBuildInputs
has quite narrow scope of uses. First thing is that you need to understand that it only works in the nix builds (and nix-shell, which tries to emulate the build environment). So adding findutils
to propagatedBuildInputs
will work only in those situations, but not for a program that runs outside of a nix build. This is the reason why we prefer patching and wrapping [1].
Now regarding the throwing all libraries into propagatedBuildInputs
, this is also not what we usually do. While it is true that some libraries may impose some transitive dependencies on their users, these cases are usually an exception. A typical example would be if a library A is written in C/C++ and contains an unconditional include of a header of a library B, then, for sure, B goes to the propagatedBuildInputs
of A. Or other example could be if A's build system imposes a requirement to also statically link library B. We actually want to keep propagatedBuildInputs
as small as possible to ensure that the build environments of the dependant builds are most minimal (performant) and most predictable (easy to track).
[1] see makeWrapper in https://nixos.org/nixpkgs/manual/#ssec-stdenv-functions
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 able to find the source of dependency on findutils
. I tried removing it from the nativeBuildInputs
:
diff --git a/pkgs/development/libraries/physics/geant4/default.nix b/pkgs/development/libraries/physics/geant4/default.nix
index 02f609be526..d96293516aa 100644
--- a/pkgs/development/libraries/physics/geant4/default.nix
+++ b/pkgs/development/libraries/physics/geant4/default.nix
@@ -72,7 +72,7 @@ stdenv.mkDerivation rec {
# Because you compile against Geant4, it's useful to have all the libs you'd
# want available
- propagatedBuildInputs = [ clhep expat zlib libGLU libGL xlibsWrapper libXmu findutils ]
+ propagatedBuildInputs = [ clhep expat zlib libGLU libGL xlibsWrapper libXmu ]
++ stdenv.lib.optionals enableGDML [ xercesc ]
++ stdenv.lib.optionals enableXM [ motif ]
++ stdenv.lib.optionals enableQT [ qtbase ]
And I don't see any shell hook message:
% nix-shell -I nixpkgs=`pwd` -p geant4
[nix-shell:/tmp/pr78775]$
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 apologize. That was quite sloppy on my part. I did see the shell hook message, but only with overriding enableQt = true
, and it seems to come from a different package.
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.
Ok, let's talk about propagatedBuildInputs
I did some digging.
When you use the Geant4Config.cmake file (to compile against Geant4) it searches for the following packages using find_dependency (depending on configuration), clhep, expat, zlib, xercesc, vecgeom, freetype, hdf5, x11, Qt5foo, opengl, motif, (xquartzgl apple only).
so I'll place just the ones matching those in propagatedBuildInputs, and the rest in buildInputs.
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 must say I'm surprised by what they do in their configuration file. Usually, those are supposed to hold the hardcoded values. Instead, they lookup bunch of dependencies, which, I guess, supposed to be used by the caller. This is strange.
fa9b01f
to
aa7f627
Compare
@GrahamcOfBorg build geant4 |
Also, would be nice to unbreak diff --git a/pkgs/development/libraries/physics/geant4/g4py/default.nix b/pkgs/development/libraries/physics/geant4/g4py/default.nix
index dddd7078b86..69fb5a99738 100644
--- a/pkgs/development/libraries/physics/geant4/g4py/default.nix
+++ b/pkgs/development/libraries/physics/geant4/g4py/default.nix
@@ -18,13 +18,15 @@ stdenv.mkDerivation {
inherit (geant4_nomt) version src;
pname = "g4py";
- sourceRoot = "geant4.10.05.p01/environments/g4py";
-
nativeBuildInputs = [ cmake ];
buildInputs = [ geant4_nomt xercesc boost_python python ];
GEANT4_INSTALL = geant4_nomt;
+ postPatch = ''
+ cd environments/g4py
+ '';
+
preConfigure = ''
# Fix for boost 1.67+
substituteInPlace CMakeLists.txt \ |
Thanks for catching that! I don't use g4py so I tend to forget it exists. |
aa7f627
to
677a52f
Compare
@OmnipotentEntity Thanks! |
Motivation for this change
Updating version to 10.6.0 and updating associated datasets to their newest version.
Also updated hashes to base32, and fixed issue with bash scripting due to the (new?) default of
set -u
.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)