Cdo: init at 1.7.2 #22496
Cdo: init at 1.7.2 #22496
Conversation
| @@ -0,0 +1,60 @@ | |||
| { stdenv, fetchurl, curl | |||
| , enable_data ? true # DATA support [default=yes] | |||
There was a problem hiding this comment.
Note that these parameters are not discoverable. Packages in Nixpkgs are usually configured with lowest-common denominator in mind, using params only sparingly. Reifying every configure switch as a parameter is certainly going overboard in any case.
Please consider reducing this to only cover options users are likely to want to change or for features that are particularly costly in terms of build footprint.
|
|
||
| # Configure phase | ||
| configureFlags = '' | ||
| ${if enable_data then "--enable-data" else ""} |
There was a problem hiding this comment.
Please consider using optionalString or specifying configureFlags as a list of strings, using optional(s) for optional flags.
| ''; | ||
|
|
||
| meta = { | ||
| description = "collection of command line Operators to manipulate and analyse Climate and NWP model Data"; |
There was a problem hiding this comment.
Please capitalize meta.description.
|
|
||
| meta = { | ||
| description = "collection of command line Operators to manipulate and analyse Climate and NWP model Data"; | ||
| longDescription = '' Supported data formats are GRIB 1/2, netCDF 3/4, SERVICE, EXTRA and IEG. |
There was a problem hiding this comment.
A better/more idiomatic syntax for longDescription (or multi-line Nix strings in general) is
''
Lorem ipsum
Lorem ipsum.
''
|
Also please consider building this with sandboxing enabled to help ensure that there are no implicit dependencies. |
|
I adjusted my default.nix file according to your remarks. |
| , enable_cdi_lib ? false # build, install and link to a CDI library [default=no] | ||
| , enable_all_static ? false # build a completely statically linked CDO binary [default=no] | ||
| , enable_cxx ? false # Use CXX as default compiler [default=no] | ||
| , with_gnu_ld ? false # assume the C compiler uses GNU ld [default=no] |
There was a problem hiding this comment.
Is it actually necessary to specify --with-gnu-ld? I can't recall that ever being an issue, autoconf seems to be able to figure it out.
Also see other expressions that use feature flags; you normally use parameters named fooSupport or withFoo. For consistency, I recommend following that pattern.
There was a problem hiding this comment.
Thanks for the comment, after testing it's not necessary. So I deleted the option
| "--with-netcdf=${netcdf}" "--with-hdf5=${hdf5}"] | ||
| ++ stdenv.lib.optional (enable_cdi_lib) "--enable-cdi-lib" | ||
| ++ stdenv.lib.optional (enable_all_static) "--enable-all-static" | ||
| ++ stdenv.lib.optional (enable_cxx) "--enable-cxx" |
There was a problem hiding this comment.
Syntax error here, missing list terminator.
There was a problem hiding this comment.
Sorry for this,
I have just pushed the new version
|
@ltavard: Could you squash your changes to a single |
|
All good! Just added darwin and fixed some ws and then merged |
(cherry picked from commit bf491f8)
Motivation for this change
CDO is a collection of command line Operators to manipulate and analyse Climate and NWP model Data.
We want to deploy it on our HPC platform.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)