libplctag icon indicating copy to clipboard operation
libplctag copied to clipboard

Potential PRs for static WIN32 builds and separating shared and static builds

Open gotnone opened this issue 2 years ago • 3 comments

I have been working on packaging libplctag for vcpkg. I encountered a handful of issues which required patching.

  1. Currently libplctag builds both shared libraries and static libraries at the same time.
  • This led to the decision in the WIN32 build to rename the static library to plctag_static.lib to avoid name collision with the import library for the shared library
  • Changing this may be disruptive to downstream consumers who are dependent on the existing build
  1. Static library builds for WIN32 were not actually usable because of LIB_EXPORT using declspec decorators in libplctag.h
  • This issue affected MSVC and MinGW (gcc and likely clang [untested])
  • I believe that this could be fixed by adding an additional level of #ifdef which might check if LIBPLCTAG_STATIC is defined. In the static build the LIB_EXPORT declspec decorators could be replaced by extern
  1. PkgConfig config file libplctag.pc.in is not configured and the resulting libplctag.pc is not created if BUILD_EXAMPLES is FALSE
  • This file is useful for consumers of the library, even if they don't want to build the examples
  • Fixing this requires moving the CONFIGURE_FILE(... outside of the elseif (BUILD_EXAMPLES) conditional
  1. PkgConfig config file libplctag.pc.in adds -pthread to Libs: section even for WIN32 builds
  • This causes MSVC to issue a warning regarding /pthread if the resulting .pc file is consumed:
find_package(PkgConfig REQUIRED)
pkg_search_module(libplctag REQUIRED IMPORTED_TARGET libplctag)

target_link_libraries(main PRIVATE PkgConfig::libplctag)
  • The hardcoded -pthread could be replaced with @PTHREAD_LDFLAGS@ where PTHREAD_LDFLAGS is set for UNIX and perhaps MINGW builds
  1. PkgConfig config file does not add -lws2_32 to Libs: section which is needed for WIN32 builds that use PkgConfig to include the library
  • This could be fixed by adding @WINSOCK_LDFLAGS@ to the Libs section of the libplctag.pc.in file and setting WINSOCK_LDFLAGS to -lws2_32 for WIN32 in CMakeLists.txt
  1. The install command for WIN32 shared libraries placed dll files in lib
  • My understanding of the convention is that on WIN32 dll files need to go to bin
  • Perhaps this could be fixed by dropping DESTINATION lib${LIB_SUFFIX} from the install(... function

Would you be interested in PRs for fixing some of these issues?

If implementing the changes noted in 1 to separate static from shared builds is too disruptive, would you be interested in a PR that did some work to separate these builds, but by default did build both of them?

gotnone avatar Sep 13 '23 18:09 gotnone

Thanks for the interest in the library. I've been under the weather for a while. Let me look through these. I am interested in seeing them broken out. Definitely I have interest in the static/dynamic patch. That's been bugging me for a while but I did not know about the fix!

kyle-github avatar Oct 25 '23 17:10 kyle-github

Wow. I am really late replying here. I would like a PR if you still are interested. The existing CMake configuration is overly complicated. It would be nice to really clean it up.

kyle-github avatar Jun 09 '24 14:06 kyle-github