Update wgrib2 from JCSDA/NOAA-EMC fork#32857
Conversation
spackbot-app
bot
added
conflicts
maintainers
new-variant
new-version
update-package
labels
spackbot-app
bot
requested review from
Hang-Lei-NOAA,
edwardhartnett,
kgerheiser and
t-brown
tldahlgren
left a comment
tldahlgren
left a comment
There was a problem hiding this comment.
Confirmed sha256. Have a minor question.
| variant("netcdf3", default=True, | ||
| description="Link in netcdf3 library to write netcdf3 files") | ||
| variant("netcdf4", default=False, | ||
| description="Link in netcdf4 library to write netcdf3/4 files") |
There was a problem hiding this comment.
Can you build with both netcdf3 and netcdf4? If not, perhaps a multi-valued variant like ipolates is order here.
There was a problem hiding this comment.
I don't think you can. I think you are suggesting something like this:
variant("netcdf", default="3",
description="Version of netCDF library to use",
values=("3", "4"))
To me this would be confusing because to the inexperienced it reads like you can either use netcdf version 3 or version 4 files, the information that netcdf4 can read and write both 3 (classic) and 4 (all others) is lost?
There was a problem hiding this comment.
Do users actually still use netCDF3? You could change the description to something along the lines of:
description="Version of netCDF library to use. Note, version 4 also supports version 3 (classic)."
There was a problem hiding this comment.
Another option, if you want to leave both variants in and want to allow only one at a time, is to add a conflict() if both are enabled.
There was a problem hiding this comment.
I added the conflict. But it turns out that the +netcdf4 build is not working, because one needs to manually (or encoded in the package.py file) download the netcdf4 source code during the install. I'll demote the PR to draft mode and return it to "ready for review" once I got the time to fix this.
…ack/repos/builtin/packages/wgrib2/package.py
|
Style errors are now fixed. |
…nt, add conflict for variants netcdf3, netcdf4
tldahlgren
left a comment
tldahlgren
left a comment
There was a problem hiding this comment.
Re-confirmed sha256. Will not merge since in Draft mode, etc.
….com/climbfuji/spack into feature/update_wgrib2_from_jcsda_emc
|
@tldahlgren @t-brown I fixed the failing wgrib2 build with the netcdf4 variant. I had to add line 88 and lines 163-167. You will see that wgrib2 uses its own, internal hdf5 and netcdf dependencies and doesn't link against external/spack versions. There is nothing I can do about this, it would be up to the developers at NOAA-EMC to change this. I installed both |
tldahlgren
left a comment
tldahlgren
left a comment
There was a problem hiding this comment.
(Re) confirmed sha256 in changes.
|
@t-brown @edwardhartnett Are you fine with being removed as maintainers from this package? |
I'm OK. |
| if self.spec.satisfies("+netcdf4"): | ||
| with working_dir(self.build_directory): | ||
| os.system( | ||
| "wget https://downloads.unidata.ucar.edu/netcdf-c/4.8.1/netcdf-c-4.8.1.tar.gz" |
There was a problem hiding this comment.
Really don't like this and line 170. Is there no way we can use the Spack versions?
There was a problem hiding this comment.
Same here, but that is really not something I should be doing. That's up to the package creators at EMC.
There was a problem hiding this comment.
As an alternative, we could completely disable the +netcdf4 variant in spack until they fix wgrib2.
There was a problem hiding this comment.
@t-brown Can we get a motion on this PR? I am only the messenger trying to update the package in the authoritative spack repo (and fixing bugs along the way). As @edwardhartnett said, people are encouraged to move away from wgrib2 since its author has refused to cooperate on improving the package.
There was a problem hiding this comment.
I thought you were removing me as a maintainer. If so, I should not be holding you up at all.
|
@t-brown please leave me as a maintainer so I can see changes. Although NOAA EMC is discouraging further use of wgrib2, due to it's difficult home-rolled build system which conflicts with modern practices, we still depend on this utility. I would like to get notification of changes. |
@edwardhartnett you should requesting @climbfuji keep you, rather than I. |
tldahlgren
left a comment
tldahlgren
left a comment
There was a problem hiding this comment.
Re-confirmed new sha256.
Funny thing is, nobody got removed or added in this PR ;-) but thanks everyone for getting this merged. |
My apologies. I had the PR up for a while so didn't see the comments. Do you want to revert or submit a new PR for the changes? |
No worries, I think that we are actually all good and that the whole conversation about whether or not to remove anyone was a misunderstanding. |
* Update wgrib2 from JCSDA/NOAA-EMC fork * var/spack/repos/builtin/packages/wgrib2/package.py: fix typo in comment, add conflict for variants netcdf3, netcdf4 * wget hdf5/netcdf4 internal dependencies for wgrib2 * Black-format var/spack/repos/builtin/packages/wgrib2/package.py * More format changes in var/spack/repos/builtin/packages/wgrib2/package.py
* Update wgrib2 from JCSDA/NOAA-EMC fork * var/spack/repos/builtin/packages/wgrib2/package.py: fix typo in comment, add conflict for variants netcdf3, netcdf4 * wget hdf5/netcdf4 internal dependencies for wgrib2 * Black-format var/spack/repos/builtin/packages/wgrib2/package.py * More format changes in var/spack/repos/builtin/packages/wgrib2/package.py
4 participants
Addresses #32856 by updating wgrib2 from NOAA-EMC spack fork.
This major update fixes problems with wgrib2's internal use of several libraries such as netcdf etc. NOAA-EMC uses this version for the UFS applications and tools.