-
Notifications
You must be signed in to change notification settings - Fork 103
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
Fix: function definition is marked dllimport #210
Conversation
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.
LGTM!
Thanks for your contribution @felixf4xu! 🚀
Thanks for the review. At the same time, I have a question about the compiling switches. The issue only happens when fastcdr is built as a shared dll (on Windows), in ros2, it's ROS2 actually supports Windows, maybe it's build as a static lib on Windows? The reason why I asked is that I actually have begun to work on fastrtps as well, it has even more issues like this one, but again, fastrtsp seems to be ready for windows shared dll because it already has all the code there, just not tested, maybe nobody wants to build it on Windows as shared dll? |
And I see the github actions for this fastcdr project, Windows is also listed there. I'm curious if it's configured to compile as static lib? I'm not familiar with github actions, is it possible to set some actions to build on windows as shared dll? |
@felixf4xu It might be something related with the compiler. As stated in the supported platforms here, our tier 1 build system on Windows is Visual Studio 2019. On that tier 1 platform we pass the CI with The error you mention in #209 is usually reported by MinGW. If you are building in MinGW then bear in mind that it is not an officially supported platform. |
@Mergifyio rebase |
Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com>
Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com>
✅ Branch has been successfully rebased |
CI run successfully on #247 |
Fix #209