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

Optional dynamic linking #30

Merged
merged 3 commits into from
Feb 27, 2024
Merged

Optional dynamic linking #30

merged 3 commits into from
Feb 27, 2024

Conversation

awnawab
Copy link
Collaborator

@awnawab awnawab commented Feb 23, 2024

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.

@awnawab awnawab requested review from dareg and pmarguinaud February 23, 2024 14:54
@dareg
Copy link
Collaborator

dareg commented Feb 23, 2024

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
Copy link
Collaborator Author

awnawab commented Feb 23, 2024

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

@dareg
Copy link
Collaborator

dareg commented Feb 23, 2024

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
Copy link
Collaborator Author

awnawab commented Feb 23, 2024

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
Copy link
Collaborator

dareg commented Feb 26, 2024

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

  • if I specify -DENABLE_ACC=OFF -DBUILD_SHARED_LIBS=OFF it stills build shared library, when it should build a static library
  • if I specify -DENABLE_ACC=ON -DBUILD_SHARED_LIBS=ON cmake doesn't throw any error, neither warnings, just a message saying: "-- Feature SHARED_LIBS was not enabled (also not requested) -- following condition was not met: NOT HAVE_ACC"
    I think it would be good if it was throwing an error as you said previously.

@awnawab
Copy link
Collaborator Author

awnawab commented Feb 26, 2024

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
Copy link
Collaborator

dareg commented Feb 26, 2024

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
Copy link
Collaborator Author

awnawab commented Feb 26, 2024

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
Copy link
Collaborator

dareg commented Feb 27, 2024

I propose that we could do it this way: dareg@2021348
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
Copy link
Collaborator Author

awnawab commented Feb 27, 2024

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

@awnawab awnawab merged commit 5d5e98e into main Feb 27, 2024
6 checks passed
@awnawab awnawab deleted the naan-shared-linking branch February 27, 2024 19:43
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.

2 participants