Skip to content

Build shared library in addition to static#91

Closed
tamboril wants to merge 1 commit intopuppetlabs:masterfrom
tamboril:co_dvtrading_shlib
Closed

Build shared library in addition to static#91
tamboril wants to merge 1 commit intopuppetlabs:masterfrom
tamboril:co_dvtrading_shlib

Conversation

@tamboril
Copy link

Because this library creates at least one global type with a destructor,
namely path_parser::api_origin, it cannot be linked statically to both
a shared library and an executable that links with the shared library.

It will run the destructor twice on exit (valgrind catches this, but executable
can also crash hard).

While I could work around this with a change to the source, I elected to just
generate a shared library in addition to the static one, the use of which I confirmed
fixes the problem.

@MikaelSmith
Copy link
Contributor

This could be done with a CMake setting, BUILD_SHARED_LIBS, as well. See https://cmake.org/cmake/help/v3.0/variable/BUILD_SHARED_LIBS.html. I'll consider building the shared library, could be useful in general.

@tamboril
Copy link
Author

Oh. Wasn't aware of that setting. You may still need the change SET(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE) or else the resulting shared libraries might not reference (through their RPATH) the boost libs they require.

@MikaelSmith
Copy link
Contributor

Makes sense. We currently set it when building via a CMake toolchain file.

@MikaelSmith
Copy link
Contributor

@puppetlabs/cpp-hocon-maintainers have other thoughts on this?

@tamboril
Copy link
Author

This should be built as SHARED instead of MODULE and should have VERSION and SOVERSION set.

@paravoid
Copy link

Do you intend of having a stable ABI, i.e. not bumping the SONAME on every release? That would be preferrable but the only way this could get packaged in distributions. (Breaking the ABI infrequently while bumping the SONAME is always fine)

@MikaelSmith
Copy link
Contributor

The intention should be to have a stable ABI, and only break it on major versions.

@paravoid
Copy link

Perfect! I also see that you use symbol versioning, so that should make it even better.

RIght now the build results in a SONAME of "libcpp-hocon.so.0.1.5". I suppose this will be fixed with @tamboril's suggestion about VERSION/SOVERSION?

@MikaelSmith
Copy link
Contributor

I think so. I should add a caveat that we'll do that with a 1.0 release, but I'm not sure there will be significant changes before then. SOVERSION I would assume to fix the versioning at the end.

@hogarth-sv
Copy link

hogarth-sv commented Aug 31, 2017

@MikaelSmith

I'm going to need this as part of updating facter in Fedora.

For obvious reasons we prefer shared libraries over static.

Is there going to be any progress on this soon to have it as a shared library straight from yourselves upstream rather than needing a maintainer patch to make it suitable for the distribution?

@MikaelSmith
Copy link
Contributor

MikaelSmith commented Aug 31, 2017

@hogarthj is there a reason you need both libraries? For just a shared library setting BUILD_SHARED_LIBS when you configure CMake should be enough.

@hogarth-sv
Copy link

hogarth-sv commented Aug 31, 2017

@MikaelSmith That's a good point ... we have no need for the static library in Fedora and setting that works fine for my needs.

Thanks for the reminder on that.

The only thing I'll need to get in place as a maintainer patch for now then I think is the relevant cmake magic so find_package(CPPHOCON REQUIRED) in facter is honored properly ... that would be a cleaner solution than adjusting the facter build to locate it.

@MikaelSmith
Copy link
Contributor

Closing this as handled with external CMAKE config.

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