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.

Sign up for GitHub

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

Jump to bottom

geant4: 10.4.1 -> 10.5.1 #70535

Merged
merged 1 commit into from Oct 19, 2019
Merged

geant4: 10.4.1 -> 10.5.1 #70535

merged 1 commit into from Oct 19, 2019

Conversation

OmnipotentEntity
Copy link
Contributor

@OmnipotentEntity OmnipotentEntity commented Oct 6, 2019 •
edited

Motivation for this change

Update to latest version (several months old at this point). Update the data files to their latest version as well (some have been renamed and might disappear soon.)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @tmplt

@veprbl veprbl self-assigned this Oct 6, 2019
@ofborg ofborg bot requested a review from tmplt October 6, 2019 18:26
@OmnipotentEntity
Copy link
Contributor Author

I can't figure out how or even if the environment variables set by the packages herein worked previously. I may just be doing something wrong. You can set them manually and it works fine, but I wanted @tmplt feedback before modifying things with that, just in case I was screwing up.

@veprbl
Copy link
Member

veprbl commented Oct 6, 2019

@OmnipotentEntity The datasets are designed to work from nix-shell and from within nix builds. There is nothing put in place to make them automatically work in the static user environments (like nix-env or NixOS systemPackages).

@OmnipotentEntity
Copy link
Contributor Author

@veprbl I figured as much from the shellhook statement, but I couldn't figure out the incantation to make nix-shell install the data packages. I tried nix-shell -p 'geant4 geant4.data but I receive a big fat error message for my troubles.

veprbl
veprbl reviewed Oct 9, 2019
View reviewed changes
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This update breaks g4py package. I guess it needs to be updated too.

Also, I suggest we take the changes adding meta.priority and "fixConflicts" out of this PR to focus on bumping the version first.

@veprbl
Copy link
Member

veprbl commented Oct 9, 2019

@veprbl I figured as much from the shellhook statement, but I couldn't figure out the incantation to make nix-shell install the data packages. I tried nix-shell -p 'geant4 geant4.data but I receive a big fat error message for my troubles.

The "-p" option of nix-shell will not take the geant4.data because it's an attribute set (this limitation has to do with how this option is implemented in nix-shell). However, it should be possible to include specific datasets:

nix-shell -p geant4 -p geant4.data.G4NDL -p geant4.data.G4ABLA

@veprbl veprbl removed their assignment Oct 11, 2019
@OmnipotentEntity
Copy link
Contributor Author

Should I also update g4py in this pull request, or should I issue a second one for it separately?

@OmnipotentEntity
Copy link
Contributor Author

This push fixes the issues listed above. Including the one that @veprbl thought deserved its own PR, because I'd rather fix the underlying issue, and it's not that complicated a change.

@ofborg ofborg bot requested a review from tmplt October 19, 2019 18:48
@veprbl
Copy link
Member

veprbl commented Oct 19, 2019

Should I also update g4py in this pull request, or should I issue a second one for it separately?

I don't mind either way.

@OmnipotentEntity
Copy link
Contributor Author

I corrected the things you've indicated and updated g4py. Tested it as working at the barest level (an otherwise empty python file with "import Geant4").

@OmnipotentEntity
Copy link
Contributor Author

Thanks @veprbl for all of the help and the reviews. I think I learned a lot from you.

Thanks @tmplt for the review and putting together this package in the first place ;D

@veprbl
Copy link
Member

veprbl commented Oct 19, 2019

@GrahamcOfBorg build geant4 g4py

veprbl
veprbl approved these changes Oct 19, 2019
View reviewed changes
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on x86_64-linux (non-nixos). The B1 example compiles and seems to work.

@veprbl veprbl merged commit e36dca0 into NixOS:master Oct 19, 2019
@veprbl
Copy link
Member

veprbl commented Oct 19, 2019

Thank you for your contribution! If there there are some questions left, feel free to open an issue or post on Discourse.

@OmnipotentEntity OmnipotentEntity deleted the geant4-update-10.5.1 branch October 20, 2019 13:06
peti pushed a commit that referenced this pull request Oct 21, 2019
@OmnipotentEntity @peti
geant4: 10.4.1 -> 10.5.1 (#70535)
f5cb4ce
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veprbl veprbl veprbl approved these changes

@tmplt tmplt tmplt approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@OmnipotentEntity @veprbl @tmplt