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

Fix dll builds under mingw-w64 #2290

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mjwoods
Copy link
Contributor

@mjwoods mjwoods commented Apr 13, 2022

Building the netcdf dll and related executables under mingw-w64 (with cmake) fails due to an undefined reference to `NC4_show_metadata'. The code contains a workaround for the MS C compiler, and this can be extended to gcc in mingw-w64.

After that, the build fails again due to undefined references to functions in tst_utils.h, which are declared as dllimport when they should be extern.

After fixing tst_utils.h, the build completes and all except 4 tests pass. The failed tests are:
82 - nc_test4_tst_filter (Failed)
83 - nc_test4_tst_specific_filters (Failed)
222 - nczarr_test_run_filter (Failed)
223 - nczarr_test_run_specific_filters (Failed)

All of the failed tests relate to filters, which are not being built under mingw-w64. I think this is because libdl is not available by default on mingw-w64, although it can be installed as an optional package. These tests should probably be disabled if the filters are not built.

@mjwoods mjwoods requested a review from WardF as a code owner April 13, 2022 09:57
@DennisHeimbigner
Copy link
Collaborator

I am surprised at this. We routinely test our code using mingw as a github action.
Could I request two things:

  1. tell me the ./configure options you are using
  2. look at our mingw action (https://github.com/Unidata/netcdf-c/blob/main/.github/workflows/run_tests_win_mingw.yml) and tell us if we are missing something critical.

@DennisHeimbigner
Copy link
Collaborator

Additional point: we will not get rid of the EXTERNL because it is the macro that allows us to add necessary __declspec declarations for visual studio.
We can however change the definition of EXTERNL so that it treats mingw correctly.

@mjwoods
Copy link
Contributor Author

mjwoods commented Apr 14, 2022

Hi @DennisHeimbigner , I was using cmake -DBUILD_SHARED_LIBS=ON -DENABLE_DLL=ON. Now I have tried using the configure script based on your github action, and it does work for me. I'm not sure what is causing the difference, but I'm happy to use configure from now on.

@mjwoods
Copy link
Contributor Author

mjwoods commented Apr 14, 2022

On the EXTERNL issue, I think there are some complications in tst_utils.h. EXTERNL has been used for functions that are not defined in the libnetcdf.dll (or any DLL), so extern on its own is sufficient. But there is (at least) one function (e.g. nc__testurl) which is defined in the libnetcdf.dll, so EXTERNL is needed there to handle the dllimport/dllexport syntax. In other words, there is no single definition of EXTERNL that would handle all definitions in tst_utils.h.

@DennisHeimbigner
Copy link
Collaborator

We also EXTERNL some functions so our unit-tests can access them,
In any case, we will keep EXTERNL. If you want to modify it, we can do that.
It occurs in two places: netcdf.h and ncexternl.h

@WardF WardF added this to the 4.9.1 milestone Apr 26, 2022
Copy link
Member

@WardF WardF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am leaving a review since we have adopted the 'review required' workflow, but the comment thread from Dennis is spot on, insofar as we need to retain EXTERNL but can modify the definition as he described.

@DWesl
Copy link
Contributor

DWesl commented Aug 20, 2022

Relevant definition appears to be here:

netcdf-c/include/netcdf.h

Lines 544 to 556 in 12a9083

/* Declaration modifiers for DLL support (MSC et al) */
#if defined(DLL_NETCDF) /* define when library is a DLL */
# if defined(DLL_EXPORT) /* define when building the library */
# define MSC_EXTRA __declspec(dllexport)
# else
# define MSC_EXTRA __declspec(dllimport)
# endif
# include <io.h>
#else
#define MSC_EXTRA /**< Needed for DLL build. */
#endif /* defined(DLL_NETCDF) */
#define EXTERNL MSC_EXTRA extern /**< Needed for DLL build. */

Perhaps cmake is setting DLL_NETCDF when autotools does not?

There's similar bits in include/ncexternl.h, oc2/oc.h, oc2/ocx.h, include/ncuri.h. There's also a couple of files that just to the __declspec(dll...) without the extern bit after.

Out of curiousity, where does autotools set DLL_NETCDF, which determines how the important EXTERNL macro gets defined? CMake definition appears to be here:

netcdf-c/config.h.cmake.in

Lines 106 to 110 in 5ccb71d

/* set this only when building a DLL under MinGW */
#cmakedefine DLL_EXPORT 1
/* set this only when building a DLL under MinGW */
#cmakedefine DLL_NETCDF 1

Does the CMake build succeed if those lines are removed from config.h.cmake.in? It looks like that would trigger gcc's auto-export-everything behavior.

Also, CMake on MinGW is unsupported, and the Autotools CI job explicitly requests that the linker mark every function __declspec(dllexport) because none of the tests link without it, so that system needs an overhaul anyway.

@WardF WardF modified the milestones: 4.9.1, 4.9.2 Feb 13, 2023
@WardF WardF modified the milestones: 4.9.2, 4.9.3 May 16, 2023
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.

4 participants