Unidata / netcdf-c

Official GitHub repository for netCDF-C libraries and utilities.
BSD 3-Clause "New" or "Revised" License
520 stars 262 forks source link

With netcdf-c-4.9.3-rc1, pio fails #3017

Open edwardhartnett opened 2 months ago

edwardhartnett commented 2 months ago

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.

WardF commented 2 months ago

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

WardF commented 2 months ago

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.

edwardhartnett commented 2 months ago

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

WardF commented 2 months ago

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.

edwardhartnett commented 2 months ago

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.

ZedThree commented 2 months ago

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.