-
Notifications
You must be signed in to change notification settings - Fork 142
Feature/shared exports msvc #525
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/shared exports msvc #525
Conversation
added an additional msvc shared pipeline also define exports/imports for yyjson
Summary of ChangesHello @BestITUserEUW, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request is a draft aimed at enabling the Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces the necessary changes to support building reflectcpp
as a shared library, particularly for MSVC on Windows. This is achieved by adding a common.hpp
header that defines the RFL_API
macro for symbol visibility, and applying this macro across the public API of the library. The CMake build system is also updated to handle the shared library build configuration. The changes are extensive but applied consistently. My review includes a critical fix for the CMake configuration to correctly handle symbol visibility for the bundled yyjson
dependency, and a minor style fix.
Regarding your question about handling DLLs for test executables, you've run into a common issue on Windows. A clean way to solve this in CMake is to have all runtime artifacts (executables and DLLs) placed in the same directory. You can achieve this by setting the CMAKE_RUNTIME_OUTPUT_DIRECTORY
variable early in your CMakeLists.txt
. For example:
# Place all executables and DLLs in the same directory
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
This way, the test executables will be in the same folder as reflectcpp.dll
and all its dependency DLLs, and they will be found automatically at runtime. This is generally preferable to copying files around with custom commands.
CMakeLists.txt
Outdated
if(REFLECTCPP_JSON) | ||
target_compile_definitions(reflectcpp | ||
PRIVATE | ||
YYJSON_EXPORTS | ||
PUBLIC | ||
YYJSON_IMPORTS | ||
) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current logic for defining YYJSON_EXPORTS
and YYJSON_IMPORTS
has a few issues:
- Conflicting Definitions: By using
PUBLIC
forYYJSON_IMPORTS
, bothYYJSON_EXPORTS
(fromPRIVATE
) andYYJSON_IMPORTS
will be defined when buildingreflectcpp
. This is usually incorrect and can lead to compilation errors, as headers typically use an#ifdef ... #elif ... #else
structure for symbol import/export. - Static Builds: These definitions are only required for shared library builds. They are unnecessary for static builds and might cause issues.
- External Dependencies: This should only apply when using the bundled
yyjson
dependency. If an externalyyjson
is used, it should manage its own symbol visibility.
To address this, I suggest making these definitions conditional on it being a shared build that uses the bundled dependency, and using INTERFACE
to propagate YYJSON_IMPORTS
to consumers without defining it for reflectcpp
itself. The condition should also use _REFLECTCPP_NEEDS_JSON_IMPL
to cover all formats that use yyjson
.
if(REFLECTCPP_BUILD_SHARED AND _REFLECTCPP_NEEDS_JSON_IMPL AND REFLECTCPP_USE_BUNDLED_DEPENDENCIES)
target_compile_definitions(reflectcpp
PRIVATE
YYJSON_EXPORTS
INTERFACE
YYJSON_IMPORTS
)
endif()
#define RFL_API | ||
#endif | ||
|
||
#endif No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liuzicheng1987 Is there a specific reason that only avro static library is linked when using vcpkg ? Linking a static library into a shared one complicates things. Avro issue could be fixed by linking public to avro and all it's dependants as we are linking manually + using whole archive like the following: target_link_libraries(reflectcpp
PUBLIC
$<LINK_LIBRARY:WHOLE_ARCHIVE,${_AVRO_STATIC_LIB}>
"${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/lib/snappy${CMAKE_STATIC_LIBRARY_SUFFIX}"
"${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/lib/lzma${CMAKE_STATIC_LIBRARY_SUFFIX}"
"${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/lib/jansson${CMAKE_STATIC_LIBRARY_SUFFIX}"
) This essentially means that the consumer of this library also has to link against these. or providing an abstraction layer for avro and use it solely in compilation units. So avro does not need to be linked against by consumers or just get a shared build of it |
added windows shared pipeline
remove benchmarks build from shared windows pipeline
Sources for changes i applied: Avro dependency list: avro-c |
@liuzicheng1987 Ready for review. In the shared tests you can see that .lib file is generated correctly and also verifies that the linkage for all implementations work and they are runnable.
As there was no answer to my previous question i took the most non-intrusive solution for avro. Make it public link the whole archive and also it's dependants. Sources for my actions can be found in the comment above |
@BestITUserEUW , thank you so much for your effort! |
This is only a draft for now. This PR addresses #520
@liuzicheng1987 Do you want any changes with the naming or the header file name ?
Apart from this i wanted to add a shared msvc pipeline but i ran into issues with the dll in the test executables not being on path.
Do you by any chance know an easy fix to this ?
Possible solutions i found are putting the build folder on path for all the test exes which i failed to do so.
Create a cmake helper function that copies everything that matches .dll from build folder into the test exes folder but that didn't feel right. (reflectcpp.dll is not enough as this again tries to load dependent dlls)