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

Added missing export for c4::yml::Tree class #181

Merged
merged 1 commit into from
Dec 28, 2021

Conversation

aviktorov
Copy link
Contributor

No description provided.

@biojppm
Copy link
Owner

biojppm commented Dec 28, 2021

Thanks for the PR. Indeed the export was missing. But did this cause an actual problem to you? The CI extensively covers both shared and static libraries, but was flagging nothing with the missing export so I'm wondering. If you did see a problem, then something may be off in the CI, so that's why it is important to know.

@biojppm biojppm merged commit 2548f60 into biojppm:master Dec 28, 2021
@aviktorov
Copy link
Contributor Author

Hi there :) I'm happy to help!

It was causing link errors in my setup:

  1. I made header-only rapidyaml library using amalgamate tool;
  2. Then I put ryml_all.hpp to the public headers folder and put implementation inside one dll (during that stage I also had to add C4CORE_SHARED C4CORE_EXPORTS RYML_SHARED RYML_EXPORTS compiler definitions in order to have proper symbols exported). I guess amalgamate tool can help with that introducing another define which will set export definitions for both c4core and ryml;
  3. After that, I tried to use first dll in another dll which contained specific parsing code and that caused linker errors because of missing exports.

I also found similar issues in debug builds with: c4::is_debugger_attached, c4::basic_substring and some other symbols. Will do another pull request!

@aviktorov
Copy link
Contributor Author

Put another pull request to C4Core here: biojppm/c4core#56

@biojppm
Copy link
Owner

biojppm commented Dec 28, 2021

Thanks for the reply - it is interesting! The cmake scripts are helping with the defines for the shared library using set_target_properties(${target} PROPERTIES WINDOWS_EXPORT_ALL_SYMBOLS ON) (BTW, I guess you're using windows?). I will try to reproduce in the CI from the amalgamated header - I see now that no CI test was using the amalgamated header as a shared library.

The amalgamated header was merged just today, so that's still a bit rough around the edges. I opened a new issue to track the missing exports. Hopefully no more major issues are outstanding.

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