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

__declspec(dllimport) added to headers for windows #180

Merged
merged 4 commits into from
Sep 18, 2023
Merged

Conversation

milyin
Copy link
Contributor

@milyin milyin commented Sep 13, 2023

Fix for #165 issue.
Without __declspec(dllimport) the extern const .... values are mapped incorrectly when linked to dll import library, without any diagnostics

@milyin milyin requested a review from p-avital September 13, 2023 19:03
#else
#define ZENOHC_API
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a good trick for libraries that use cmake build system. You can control your export definition by distinguishing between PRIVATE and INTERFACE includes:

macro(target_define_export TARGET_NAME)
if(${WIN32})
	target_compile_definitions(${TARGET_NAME} PRIVATE ${TARGET_NAME}_EXPORT=__declspec\(dllexport\))
	target_compile_definitions(${TARGET_NAME} INTERFACE ${TARGET_NAME}_EXPORT=__declspec\(dllimport\))
else()
    target_compile_definitions(${TARGET_NAME} PUBLIC ${TARGET_NAME}_EXPORT=)
endif()
endmacro()

It isalso possible to handle static library generation here by checking target type:
get_target_property(target_type <target> TYPE)
It will be one of STATIC_LIBRARY, MODULE_LIBRARY, SHARED_LIBRARY, EXECUTABLE or one of the internal target types.

I do not insist on this approach, but it looks like a cleaner cmake-way to define library exports.

Copy link
Contributor

Choose a reason for hiding this comment

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

But this approach is applicable ONLY if your library is being integrated into other projects as source- cmake package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this approach is applicable ONLY if your library is being integrated into other projects as source- cmake package

Yes, and that's why I think this approach is not suitable. I believe that it's bad practice to place any pieces of code into cmake files. User should be able to integrate library into his build system without using of our cmake scripts. I.e. from my point of view:

  • setting compiler definitions with CMake - OK
  • assigning real pieces of code to variables (like __declspec(dllimport) with CMAke - Wrong

@milyin milyin merged commit e5c5d57 into master Sep 18, 2023
@milyin milyin deleted the dllimport branch September 18, 2023 13:19
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.

3 participants