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

Fix inconsistency when build both shared/static lib #1214

Closed
wants to merge 2 commits into from

Conversation

dota17
Copy link
Member

@dota17 dota17 commented Sep 2, 2020

Hi @SpaceIm please check this patch if it solves your issue.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.801% when pulling c829c73 on dota17:fix-cmake into 5be07bd on open-source-parsers:master.

set(STATIC_LIB ${PROJECT_NAME}_static)
else()
set(STATIC_LIB ${PROJECT_NAME}_lib)
endif()
add_library(${STATIC_LIB} STATIC ${PUBLIC_HEADERS} ${JSONCPP_SOURCES})

# avoid name clashes on windows as the shared import lib is alse named jsoncpp.lib
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: alse -> also

@@ -160,7 +164,7 @@ if(BUILD_STATIC_LIBS)
endif()

set_target_properties(${STATIC_LIB} PROPERTIES
OUTPUT_NAME jsoncpp${STATIC_SUFFIX}
OUTPUT_NAME jsoncpp
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend to keep them different (and consistent across versions) to support packaging of dual (shared+static) build.
As you may noticed MSVC generates import lib (.lib) for .dll, which should not be confused with static .lib.

@@ -151,7 +151,11 @@ if(BUILD_SHARED_LIBS)
endif()

if(BUILD_STATIC_LIBS)
set(STATIC_LIB ${PROJECT_NAME}_static)
if (BUILD_SHARED_LIBS)
set(STATIC_LIB ${PROJECT_NAME}_static)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add_library(<default name> ALIAS jsoncpp_static) or add_library(<default name> INTERFACE) and them `target_link_libraries( INTERFACE jsoncpp_static)

@baylesj
Copy link
Contributor

baylesj commented Sep 12, 2024

Closing this due to inactivity. Feel free to reopen if you are able to address feedback.

@baylesj baylesj closed this Sep 12, 2024
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.

4 participants