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

@OmnipotentEntity OmnipotentEntity commented Jan 29, 2020

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

@OmnipotentEntity OmnipotentEntity commented Jan 29, 2020

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 ]

This comment has been minimized.

@veprbl

veprbl Jan 29, 2020
Member

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

This comment has been minimized.

@veprbl

veprbl Jan 30, 2020
Member

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

This comment has been minimized.

@OmnipotentEntity

OmnipotentEntity Jan 30, 2020
Author Contributor

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.

This comment has been minimized.

@OmnipotentEntity

OmnipotentEntity Jan 30, 2020
Author Contributor

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.

This comment has been minimized.

@veprbl

veprbl Jan 30, 2020
Member

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 OmnipotentEntity requested a review from veprbl Jan 30, 2020
@OmnipotentEntity OmnipotentEntity force-pushed the OmnipotentEntity:geant4-update-10.6.0 branch from fa9b01f to aa7f627 Jan 30, 2020
@OmnipotentEntity
Copy link
Contributor Author

@OmnipotentEntity OmnipotentEntity commented Jan 30, 2020

@GrahamcOfBorg build geant4

@veprbl
Copy link
Member

@veprbl 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

@OmnipotentEntity OmnipotentEntity commented Feb 2, 2020

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

@OmnipotentEntity OmnipotentEntity force-pushed the OmnipotentEntity:geant4-update-10.6.0 branch from aa7f627 to 677a52f Feb 2, 2020
@veprbl
veprbl approved these changes Feb 3, 2020
@veprbl veprbl merged commit faf20df into NixOS:master Feb 3, 2020
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
geant4 on aarch64-linux Success
Details
geant4 on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@veprbl
Copy link
Member

@veprbl veprbl commented Feb 3, 2020

@OmnipotentEntity Thanks!

jpgu-epam added 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.