Skip to content
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

Merged
merged 1 commit into from Feb 3, 2020
Merged

Conversation

OmnipotentEntity
Copy link
Contributor

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@OmnipotentEntity
Copy link
Contributor Author

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 find in the path. Because findutils is part of the propagatedBuildInputs and the standard environment, I don't know how else to ensure that it's available to the shell-hook. So a point in the right direction would be appreciated.

Also this pull request changed buildInputs to propagatedBuildInputs because you compile against Geant4, so it's normally useful to have these packages available, but it could be a little bit more judicious as to what should be a propagatedBuildInput or not.


# 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 ]
Copy link
Member

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

Copy link
Member

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]$ 

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

@OmnipotentEntity
Copy link
Contributor Author

@GrahamcOfBorg build geant4

@veprbl
Copy link
Member

veprbl commented Feb 1, 2020

Also, would be nice to unbreak g4py:

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 \

@OmnipotentEntity
Copy link
Contributor Author

Thanks for catching that! I don't use g4py so I tend to forget it exists.

@veprbl veprbl merged commit faf20df into NixOS:master Feb 3, 2020
@veprbl
Copy link
Member

veprbl commented Feb 3, 2020

@OmnipotentEntity Thanks!

jpgu-epam pushed a commit to jpgu-epam/nixpkgs that referenced this pull request Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants