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

Improve static library support #203

Closed

Conversation

scpeters
Copy link
Member

🦟 Bug fix

Port of gazebosim/sdformat#394 from @joxoby to ign-cmake2. This publicly defines the *_STATIC_DEFINE symbol for a library target if BUILD_SHARED_LIBS is Off.

Summary

I believe a special symbol needs to be defined when building with static libraries on Windows. Logic for defining this symbol was added to libsdformat11 in gazebosim/sdformat#394, and I noticed it while preparing to forward-port gazebosim/sdformat#780 to libsdformat11, since I noticed that ign-cmake2 doesn't currently define this symbol. I've added some examples using static libraries, but I'm not sure they're rich enough to prove this fixes any problems on Windows. Perhaps @traversaro @joxoby @madebr have some suggestions?

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Port of gazebosim/sdformat#394 to ign-cmake2.
This publicly defines the *_STATIC_DEFINE symbol for
a library target if BUILD_SHARED_LIBS is Off.

Signed-off-by: Steve Peters <[email protected]>
This adds an example that builds a static library
and a second example that links against that static
library.

Signed-off-by: Steve Peters <[email protected]>
@github-actions github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🌱 garden Ignition Garden 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel labels Dec 29, 2021
scpeters added a commit to scpeters/sdformat that referenced this pull request Dec 29, 2021
Restore functionality of gazebosim#394. Submitted a fix upstream
in gazebosim/gz-cmake#203.

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit to scpeters/sdformat that referenced this pull request Dec 29, 2021
Restore functionality of gazebosim#394. Submitted a fix upstream
in gazebosim/gz-cmake#203.

Signed-off-by: Steve Peters <[email protected]>
@traversaro
Copy link
Contributor

I am currently away from laptop keyboard, but if I recall correctly this should not be necessary as the static case is already handled by the generate_export_header macro. Manually defining the static macro is only necessary when using the same header for both shared and static variants of a library (see https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html) or when using an hardcoded export header file (as we were doing in sdformat if I recall correctly).

@traversaro
Copy link
Contributor

I guess also @Ace314159 could be interested.

@traversaro
Copy link
Contributor

I am currently away from laptop keyboard, but if I recall correctly this should not be necessary as the static case is already handled by the generate_export_header macro.

Related code: https://github.com/Kitware/CMake/blob/master/Modules/GenerateExportHeader.cmake#L302 .

@scpeters
Copy link
Member Author

I am currently away from laptop keyboard, but if I recall correctly this should not be necessary as the static case is already handled by the generate_export_header macro. Manually defining the static macro is only necessary when using the same header for both shared and static variants of a library (see https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html) or when using an hardcoded export header file (as we were doing in sdformat if I recall correctly).

ah, I believe you are right. I had noticed the *_STATIC_DEFINE macros, but didn't realize that it would generate the macros differently for static libraries. I will revert 9666346 and just keep the new examples. I'll also revert gazebosim/sdformat@72c4853 from gazebosim/sdformat#808, since I think it won't be needed either

@scpeters
Copy link
Member Author

examples only in #202

@scpeters scpeters closed this Dec 30, 2021
@scpeters scpeters mentioned this pull request Jan 7, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🌱 garden Ignition Garden Gazebo 1️1️ Dependency of Gazebo classic version 11
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants