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.

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

Add legacy macro option #3030

Merged
merged 8 commits into from
Sep 27, 2024
Merged

Add legacy macro option #3030

merged 8 commits into from
Sep 27, 2024

Conversation

WardF
Copy link
Member

@WardF WardF commented Sep 26, 2024

Adds legacy macro option to enable _FillValue, any other macros which may reveal themselves. As discussed in #2858, the issue with _FillValue is that it can, in some circumstances, conflicts with a symbol libc++18. While _FillValue is historically important and widely used, (and technically tricky to move away from), we can't avoid the fact that _ is a reserved identifier and will potentially cause problems for users as libc++18 and later become more common in the wild.

We replaced _FillValue with NC_FillValue in #2858, and we encourage users to adjust their software to account for this. But not all of our users are maintaining software they rely upon, and we do not (and cannot) put people in a position where software they rely upon but are not equipped to modify will not compile.

Invoking --enable-legacy-macros (autotools) or -DNETCDF_ENABLE_LEGACY_MACROS=TRUE (cmake) will define the _FillValue macro, for use when building netCDF as part of a larger, un-modified technical ecosystem.

…install_dir to use /usr/local/hdf5/lib/plugin by default unless a 'prefix' is specified by the user, in which case it becomes prefix/hdf5/lib/plugin. This can still be overridden with the build flags appropriate to the build system.
@WardF WardF added this to the 4.9.3 milestone Sep 26, 2024
@WardF WardF self-assigned this Sep 26, 2024
@edwardhartnett
Copy link
Contributor

I believe this should have the opposite default setting, as the number of users who will encounter problems with _FillValue is tiny compared to the number that will have problems with its absence.

At least give users a few releases with the new macros available but not yet mandatory.

@WardF
Copy link
Member Author

WardF commented Sep 27, 2024

@edwardhartnett It would be great if we could get more release candidate exposure so that we had a better feel for this. Flipping it (as you suggest) doesn't do any harm and could (as pointed out) be better in the short run. I have documentation to add before I was going to merge this, so I'll flip the polarity for the next rc. No harm in changing a considered opinion, after all ;).

@gsjaardema
Copy link
Contributor

For some customer input, I am fine with either option. We have already added code to test whether NC_FillValue is defined and if it isn't, then use the old _FillValue

@czender
Copy link
Contributor

czender commented Sep 27, 2024

Echoing @gsjaardema , I just amended NCO to use NC_FillValue, which NCO #defines if/when netCDF does not.

@WardF WardF merged commit 0ad7164 into Unidata:main Sep 27, 2024
107 checks passed
@WardF WardF deleted the gh3029-unsafe-macro.wif branch September 27, 2024 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add enable-legacy-macros option.
4 participants