ecmwf-ifs / field_api

Apache License 2.0
3 stars 8 forks source link

Optional dynamic linking #30

Closed awnawab closed 7 months ago

awnawab commented 7 months ago

If openacc is disabled, then we are not limited by the !$acc declare create statements in field_RANKSUFF_access_module and we can link libfield_api statically. This PR adds this functionality.

dareg commented 7 months ago

Wouldn't it be better to use the standard BUILD_SHARE_LIBS option and let the user configure it, as it is done in fiat? And then force it in static if the OpenACC option is used.

awnawab commented 7 months ago

Good point! It is better to have shared/static linking as a configurable option, I'll do that.

dareg commented 7 months ago

Or even better than forcing it one way, maybe stopping cmake and showing a message explaining that some options conflicts. It might save time the day someone will try to build a shared version and will only get a static version.

awnawab commented 7 months ago

Thanks for all the feedback. I think we should keep the default behaviour of switching to static linking if openacc is enabled; a lot of our build configs automatically switch to static libs if openacc is enabled and field_api should conform to that expectation. However if somebody explicitly requests a shared lib via -DENABLE_SHARED_LIBS=ON and OpenACC is also enabled, cmake will throw an error.

dareg commented 7 months ago

Hi, I couldn't get it to work correctly in two cases:

awnawab commented 7 months ago

Thanks a lot for the feedback @dareg. ecbuild options can be toggled by appending ENABLE to the option name. In this case the option is called SHARED_LIBS, and so the cmake argument should be ENABLE_SHARED_LIBS rather than BUILD_SHARED_LIBS. Could you please try with ENABLE_SHARED_LIBS and see if that gives you the correct behaviour. I am also happy to rename the option to BUILD_SHARED_LIBS, but in that case the cmake argument would be -DENABLE_BUILD_SHARED_LIBS.

dareg commented 7 months ago

But isn't BUILD_SHARED_LIBS a 'standard' ecbuild option available for any projects? I don't think you need to redefine it. When compiling fiat you can use -DBUILD_SHARED_LIBS=ON or -DBUILD_SHARED_LIBS=OFF to control the build process. But if you use -DENABLE_BUILD_SHARED_LIBS=ON it just complains that the option is never used. I think we should use the same option and not yet again another flavour of configuration.

awnawab commented 7 months ago

BUILD_SHARED_LIBS is in fact a standard CMake cache variable, not ecbuild, and it defaults to ON for all projects. Unfortunately the default value for standard cmake cache variables cannot be overwritten (https://cmake.org/cmake/help/latest/policy/CMP0077.html#policy:CMP0077). The problem this creates is that if BUILD_SHARED_LIBS is 'ON', CMake cannot tell the difference whether the 'ON' is from the default value or a user-provided input. Therefore we cannot build a test to check if the user is providing conflicting options by setting both -DBUILD_SHARED_LIBS=ON and -DENABLE_ACC=ON.

Long story short: if we want to use the variable BUILD_SHARED_LIBS we can only build a warning for potentially conflicting build options rather than checking for conflicting user inputs and throwing an error accordingly.

dareg commented 7 months ago

I propose that we could do it this way: https://github.com/dareg/field_api/commit/202134835587c145abfe6f7c8a2a597493673fba Users will use the BUILD_SHARED_LIBS variable to specify which build they want. By default it will build a shared library, but if ENABLE_ACC is used it will build a static library (and show a warning). What do you think?

awnawab commented 7 months ago

Makes a lot of sense! I'll amend my PR accordingly.