netcdf-c icon indicating copy to clipboard operation
netcdf-c copied to clipboard

With netcdf-c-4.9.3-rc1, pio fails

Open edwardhartnett opened this issue 1 year ago • 10 comments

With netcdf-c-4.9.3-rc1, pio fails:

/collab1/data/Dusan.Jovic/sufs/simple-ufs/libs/ufslibs/build/pio-prefix/src/pio/src/clib/pio_nc.c:2433:56: error: use of undeclared identifier '_FillValue'
2433 |                     ierr = nc_put_att(file->fh, varid, _FillValue, xtype, 1, fill_valuep);
     |                                                        ^
/collab1/data/Dusan.Jovic/sufs/simple-ufs/libs/ufslibs/build/pio-prefix/src/pio/src/clib/pio_nc.c:2577:52: error: use of undeclared identifier '_FillValue'
2577 |                 ierr = nc_get_att(file->fh, varid, _FillValue, fill_valuep);
     |                                                    ^
2 errors generated.
make[5]: *** [src/clib/CMakeFiles/pioc.dir/build.make:216: src/clib/CMakeFiles/pioc.dir/pio_nc.c.o] Error 1
make[4]: *** [CMakeFiles/Makefile2:1178: src/clib/CMakeFiles/pioc.dir/all] Error 2
make[3]: *** [Makefile:146: all] Error 2

I tried both pio 2.5.10 which is what we currently use and latest pio 2.6.2. Both fail with 4.9.3-rc1.

edwardhartnett avatar Sep 11 '24 18:09 edwardhartnett

I wonder if this is the result of refactoring_FillValue to NC_FillValue, as discussed in #2858 and changed in #2911.

WardF avatar Sep 19 '24 16:09 WardF

In the short term, I wonder if I should add an option enable-unsafe-macros or some such that projects like pio and gdal have an option besides a broken API, while also knowing they'll need to change their code eventually.

WardF avatar Sep 19 '24 17:09 WardF

No add option --disable-legacy-macros. Most users don't want to change their code for this. And won't.

edwardhartnett avatar Sep 19 '24 19:09 edwardhartnett

I agree with the legacy phrasing, I'll make that change. Due to the conflict with modern standard libraries, it feels like it's going to need to be opt-in. As packages like pio and GDAL (GDAL already made the change, actually), there is no need to introduce an unnecessary potential conflict with other libraries. For now, --enable-legacy-macros will be a stop-gap that will also eventually no longer be necessary.

WardF avatar Sep 19 '24 19:09 WardF

Strongly disagree with the default. Don't break backwards compatibility for this. It's not even really unsafe or a problem, just a convention that's being imperfectly followed. Much code depends on it working as it does now.

edwardhartnett avatar Sep 19 '24 20:09 edwardhartnett

It's possible to deprecate the macro itself, here's one way to do it (in action):

#ifdef _MSC_VER
#define _FillValue _Pragma ("message( \"'_FillValue' macro is deprecated, please use NC_FillValue\")")  "_FillValue"
#else
#define _FillValue _Pragma ("GCC warning \"'_FillValue' macro is deprecated, please use NC_FillValue\"")  "_FillValue"
#endif

The MSVC solution is not great, I don't think it tells you where it's coming from, but it does tell you something at least.

ZedThree avatar Sep 20 '24 09:09 ZedThree

I still get (the same) compile error when I try to build pio with netcdf-c-4.9.3. I did set --enable-legacy-macros configure flag. In include/netcdf.h in the installation directory I see:

#ifdef NETCDF_ENABLE_LEGACY_MACROS
#define _FillValue      "_FillValue"
#endif
#define NC_FillValue      "_FillValue"

but it seems that NETCDF_ENABLE_LEGACY_MACROS is not defined. In the source directory, I see that NETCDF_ENABLE_LEGACY_MACROS is defined in config.h

$ grep -r NETCDF_ENABLE_LEGACY_MACROS config.h
#define NETCDF_ENABLE_LEGACY_MACROS 1

Where is NETCDF_ENABLE_LEGACY_MACROS supposed to be defined in the installation directory?

DusanJovic-NOAA avatar Feb 14 '25 15:02 DusanJovic-NOAA

Same error for me building esmf

In netcdf.h in installation directory you have these lines :

#ifdef NETCDF_ENABLE_LEGACY_MACROS
#define _FillValue      "_FillValue"
#endif

But NETCDF_ENABLE_LEGACY_MACROS is not defined.

I think you should have #define NETCDF_ENABLE_LEGACY_MACROS in netcdf.h, may be not by default but at least when built with option --enable-legacy-macros This is not the case for the time being

I have also opened an issue to suggest to ESMF team to use NC_FillValue

thebaptiste avatar Feb 14 '25 15:02 thebaptiste

Ok, I've written a couple responses to this and then deleted them as I think more about it so:

I am aware of the issue, I will be taking a look to wrap my head around it and determine what may need to go into a quick point release, and what the broader ramifications are.

WardF avatar Feb 14 '25 17:02 WardF

In the meantime, you should be able to (when compiling dependent software such as esmf, or anything which includes netcdf.h) define NETCDF_ENABLE_LEGACY_MACROS in the upstream build process and see the expected behavior.

WardF avatar Feb 14 '25 17:02 WardF