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

Feature/export headers #524

Merged
merged 19 commits into from
Apr 27, 2023
Merged

Feature/export headers #524

merged 19 commits into from
Apr 27, 2023

Conversation

pnoltes
Copy link
Contributor

@pnoltes pnoltes commented Apr 17, 2023

This PR adds the generation and usage of an export header and its macros to the libraries etcdlib, dfi, utils and framework.
For the updated libraries the default (C) symbol visibility is also set to hidden.
As result almost all headers for the libraries etcdlib, dfi, utils and framework are updated to ensure the correct symbol visibility.

This is the first step for the issue #442.

The export headers are generated using the CMake function generate_export_header and for dfi, utils and framework the header filename is configured as celix_<lib>_export.h and the base macro name as CELIX_<LIB>.

The utils and framework libraries also have an OBJECT library variant (needed for error injection). For now I updated the CMakelists.txt file to configure the SHARED / OBJECT libs in 2 different ways:

  • The current way of configuring the _obj OBJECT library and having the SHARED library depending (link PUBLIC) on the _obj library.
  • Using a cmake function to configure both the OBJECT and SHARED libraries and generating a export header for both library (with the same filename and macro names).

The different ways can be toggled using the CMake variables CELIX_UTILS_CMAKE_CONFIGURE_ALT_OPTION and CELIX_FRAMEWORK_CMAKE_CONFIGURE_ALT_OPTION, but before merging the PR I would like to choice one way and remove the other.

The reason I added an other way of configuring the SHARED and OBJECT library for utils and framework is that the generated export header use a #ifdef to distinguish against building and using a library and IMO to let this work as intended both the OBJECT and SHARED library should have their own export header.

See the following snippets from the utils and utils_obj generated export header:

#    ifdef utils_EXPORTS
        /* We are building this library */
#      define CELIX_UTILS_EXPORT __attribute__((visibility("default")))
#    else
        /* We are using this library */
#      define CELIX_UTILS_EXPORT __attribute__((visibility("default")))
#    endif
#    ifdef utils_obj_EXPORTS
        /* We are building this library */
#      define CELIX_UTILS_EXPORT __attribute__((visibility("default")))
#    else
        /* We are using this library */
#      define CELIX_UTILS_EXPORT __attribute__((visibility("default")))
#    endif

For both Linux and OSX the visibility attribute is always "default", so for Linux/OSX only 1 export header will also work.

Although I think generating 2 export headers for the OBJECT and SHARED libraries is more correct, I am not sure if we
should do this because it is more confusion.
Also note that only 1 export header will be installed (the SHARED export header).

@PengZheng PengZheng self-requested a review April 19, 2023 02:41
Copy link
Contributor

@PengZheng PengZheng left a comment

Choose a reason for hiding this comment

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

Nice to have symbol visibility control. I have some remarks.

libs/etcdlib/CMakeLists.txt Outdated Show resolved Hide resolved
libs/framework/CMakeLists.txt Outdated Show resolved Hide resolved
libs/framework/CMakeLists.txt Outdated Show resolved Hide resolved
libs/framework/CMakeLists.txt Outdated Show resolved Hide resolved
libs/framework/CMakeLists.txt Outdated Show resolved Hide resolved
libs/utils/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@PengZheng PengZheng left a comment

Choose a reason for hiding this comment

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

I think the last three commits have addressed the correctness concern about building the object library. If not, feel free to revert them.

@PengZheng PengZheng merged commit 335935c into master Apr 27, 2023
@PengZheng PengZheng deleted the feature/export_headers branch April 27, 2023 02:12
@pnoltes
Copy link
Contributor Author

pnoltes commented Apr 27, 2023

the last days I was not able to update this PR, but the changes LGTM.

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