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

h5utils: init at 1.13.1 #37092

Merged
merged 1 commit into from Mar 20, 2018
Merged

h5utils: init at 1.13.1 #37092

merged 1 commit into from Mar 20, 2018

Conversation

SFrijters
Copy link
Member

Motivation for this change

New package: "A set of utilities for visualization and conversion of scientific data in the free, portable HDF5 format"

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@SFrijters SFrijters force-pushed the h5utils branch 2 times, most recently from acdce01 to ce22fdd Compare March 15, 2018 11:30
nlewo
nlewo requested changes Mar 17, 2018
View reviewed changes
pkgs/tools/misc/h5utils/default.nix Outdated
version = "1.13";
name = "h5utils-${version}";

src = fetchurl {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please switch to fetchFromGitHub.

Copy link
Member Author

Choose a reason for hiding this comment

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

As a counterpoint, the package Readme explicitly suggests installing from the tarball, not a git clone:

"(Download this .tar.gz file rather than cloning the git repo unless you are a developer, since the git repo requires additional tools to build as described below.)"

Does that change things?

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I also think it's simpler to fetch this archive. So fetchurl is fine in this case.
Could you then just add a comment explaining this choice?

pkgs/tools/misc/h5utils/default.nix Outdated
{ stdenv, fetchurl, hdf5, lib }:

let
mpi = hdf5.mpi;
Copy link
Member

Choose a reason for hiding this comment

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

(non blocking) This was not helping me to read your code. So from my point of view, this could be removed.

pkgs/tools/misc/h5utils/default.nix Outdated
sha256 = "1574wrymvcp3zh1m57f2c2ajxhbvpkfb04hyx1am95q856a0b2vy";
};

preBuild = if mpiSupport then "export CC=${mpi}/bin/mpicc" else "";
Copy link
Member

Choose a reason for hiding this comment

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

tips: you could use lib.optionalString

@SFrijters SFrijters mentioned this pull request Mar 18, 2018
Closed
@SFrijters SFrijters force-pushed the h5utils branch 3 times, most recently from 14bb0e1 to f051657 Compare March 18, 2018 12:15
@SFrijters
Copy link
Member Author

I've implemented @nlewo's suggestions, but I've also extended the derivation with some optional dependencies so it could use another look; it seemed to me that different packages use slightly different ways to do this (nullable dependencies, or boolean flags), so please let me know if I should change the way I did it here.

Also, it seems that the tarball release currently does not include some build fixes needed for some of the optional dependencies, so I've asked the upstream author to cut a new release if possible. If no response is forthcoming I could switch to fetchFromGitHub after all.

@nlewo
Copy link
Member

nlewo commented Mar 18, 2018

Ok, great.
On my side, this PR could be merged. But it seems you would prefer to see some fixes integrated.
So, let's wait some days to see if a new release is coming. Ping me when you want to move forward on this PR.

@SFrijters SFrijters force-pushed the h5utils branch 2 times, most recently from 9a91136 to e24b250 Compare March 19, 2018 21:03
@SFrijters SFrijters changed the title h5utils: init at 1.13 h5utils: init at 1.13.1 Mar 19, 2018
@SFrijters
Copy link
Member Author

@nlewo The package author has made a 1.13.1 point release, so my previous build errors are now fixed.

I did run into another issue though: with the optional octave dependency, a plugin is built that is copied into the octave directory in the Nix store, which obviously is read-only. It doesn't look like plugins are supported by the octave derivation, so I've removed this optional dependency for now, unless you think there is a way to handle this gracefully?

@nlewo
Copy link
Member

nlewo commented Mar 20, 2018

@SFrijters ok, that's cool for the new release. I know nothing regarding octave so I could not really help you on this subject. This PR looks good to me.
@GrahamcOfBorg build h5utils

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: h5utils

Partial log (click to expand)

shrinking /nix/store/am89gz8wm82srxxxawm53qblksw3r8hp-h5utils-1.13.1/bin/h5fromtxt
shrinking /nix/store/am89gz8wm82srxxxawm53qblksw3r8hp-h5utils-1.13.1/bin/h5tovtk
shrinking /nix/store/am89gz8wm82srxxxawm53qblksw3r8hp-h5utils-1.13.1/bin/h5totxt
shrinking /nix/store/am89gz8wm82srxxxawm53qblksw3r8hp-h5utils-1.13.1/bin/h5topng
gzipping man pages under /nix/store/am89gz8wm82srxxxawm53qblksw3r8hp-h5utils-1.13.1/share/man/
strip is /nix/store/fzcs0fn6bb04m82frhlb78nc03ny3w55-binutils-2.28.1/bin/strip
stripping (with command strip and flags -S) in /nix/store/am89gz8wm82srxxxawm53qblksw3r8hp-h5utils-1.13.1/bin
patching script interpreter paths in /nix/store/am89gz8wm82srxxxawm53qblksw3r8hp-h5utils-1.13.1
checking for references to /build in /nix/store/am89gz8wm82srxxxawm53qblksw3r8hp-h5utils-1.13.1...
/nix/store/am89gz8wm82srxxxawm53qblksw3r8hp-h5utils-1.13.1

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: h5utils

Partial log (click to expand)

shrinking /nix/store/7dwsm8hv6x2zngh2y17899xm4yrdqhyd-h5utils-1.13.1/bin/h5topng
shrinking /nix/store/7dwsm8hv6x2zngh2y17899xm4yrdqhyd-h5utils-1.13.1/bin/h5tovtk
shrinking /nix/store/7dwsm8hv6x2zngh2y17899xm4yrdqhyd-h5utils-1.13.1/bin/h5fromtxt
shrinking /nix/store/7dwsm8hv6x2zngh2y17899xm4yrdqhyd-h5utils-1.13.1/bin/h5totxt
gzipping man pages under /nix/store/7dwsm8hv6x2zngh2y17899xm4yrdqhyd-h5utils-1.13.1/share/man/
strip is /nix/store/3zq400fri5dv7d30lpxlqm2v9y1iis6j-binutils-2.28.1/bin/strip
stripping (with command strip and flags -S) in /nix/store/7dwsm8hv6x2zngh2y17899xm4yrdqhyd-h5utils-1.13.1/bin
patching script interpreter paths in /nix/store/7dwsm8hv6x2zngh2y17899xm4yrdqhyd-h5utils-1.13.1
checking for references to /build in /nix/store/7dwsm8hv6x2zngh2y17899xm4yrdqhyd-h5utils-1.13.1...
/nix/store/7dwsm8hv6x2zngh2y17899xm4yrdqhyd-h5utils-1.13.1

@nlewo
Copy link
Member

nlewo commented Mar 20, 2018

Thanks!

@nlewo nlewo merged commit 7ef5f23 into NixOS:master Mar 20, 2018
@SFrijters
Copy link
Member Author

Thanks for the coaching!

@SFrijters SFrijters deleted the h5utils branch September 30, 2020 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlewo nlewo nlewo requested changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SFrijters @nlewo @GrahamcOfBorg