search

mdavidsaver / pvxs

PVA protocol client/server library and utilities.
https://mdavidsaver.github.io/pvxs/
Other
19 stars 25 forks source link

Build does not respect STATIC_BUILD linker option for bundled libevent #12

Closed rreno17 closed 3 years ago

rreno17 commented 3 years ago

Describe the bug Building libevent on Linux produces both shared and static libraries. When building with PVXS, the STATIC_BUILD = YES and SHARED_LIBRARIES = NO options are ignored when linking libevent. In other words, the linker will not attempt to use libevent.a when there's a libevent.so.* in the library search path.

As a workaround, I built the bundled libevent with CMAKEFLAGS += -DEVENT__LIBRARY_TYPE=STATIC added to the bundle/Makefile. The output directory bundle/usr/<target>/lib only contains static .a files. When I do this the application will statically link libevent as expected.

To Reproduce Steps to reproduce the behavior:

  1. Build libevent normally (for me, make -C bundle libevent.linuxRT-x86_64 )
  2. Add SHARED_LIBRARIES=NO and STATIC_BUILD=YES lines to the PVXS configure/CONFIG_SITE
  3. Build normally (make from <TOP>)
  4. Set up a static building EPICS application. makeBaseApp.pl, modify configure/CONFIG_SITE with the static build variables (both STATIC_BUILD=YES and SHARED_LIBRARIES=NO), properly include PVXS per your instructions, etc.)
  5. build normally 
    objdump -p bin/<target>/<app> | grep NEEDED
...
NEEDED               libevent_core-2.2.so.1
NEEDED               libevent_pthreads-2.2.so.1

Expected behavior I'd expect an application build to respect STATIC_BUILD and link libevent statically. I think this is an issue with the files in cfg/ but I couldn't figure out a hack to make it ignore the shared libraries.

Information (please complete the following):

mdavidsaver commented 3 years ago

You're right. Looks like I only have logic to select static/shared for windows. I can't recall why.

https://github.com/mdavidsaver/pvxs/blob/85783e27c253d6b7249b099251bf6f5adecde5ab/bundle/Makefile#L44-L46

mdavidsaver commented 3 years ago

@rreno Are your builds also setting SHARED_LIBRARIES=NO?

I realize that pvxs/bundle/Makefile is the wrong place to be looking. Although the bundle build should be looking at $(SHARED_LIBRARIES) instead of $(STATIC_BUILD).

Shared libraries are being preferred (when both are available) because of the logic of gcc ... -levent_core .... This comes about because I'm using eg. PROD_SYS_LIBS instead of PROD_LIBS. Since I'm auto-magically injecting this, it should be possible to make a transparent change (excepting possible ordering issues).

https://github.com/mdavidsaver/pvxs/blob/85783e27c253d6b7249b099251bf6f5adecde5ab/configure/RULES_PVXS_MODULE#L10-L17

rreno17 commented 3 years ago

@mdavidsaver Yes, I am setting both SHARED_LIBRARIES=NO and STATIC_BUILD=YES in that order.

I made a change to configure/CONFIGURE_PVXS_MODULE that seems to have fixed the issue. It's somewhat klunky but it builds as I would have expected. It uses ld's -l:libname.a feature combined with a check against STATIC_BUILD.

diff --git a/configure/CONFIG_PVXS_MODULE b/configure/CONFIG_PVXS_MODULE
index 81074fd..c836d94 100644
--- a/configure/CONFIG_PVXS_MODULE
+++ b/configure/CONFIG_PVXS_MODULE
@@ -31,12 +31,25 @@ _PVXS_DEPLIB_DIRS += $(if $(LIBEVENT),$(LIBEVENT)/lib)
 PROD_DEPLIB_DIRS   += $(_PVXS_DEPLIB_DIRS)
 SHRLIB_DEPLIB_DIRS += $(_PVXS_DEPLIB_DIRS)

+ifeq ($(STATIC_BUILD),YES)
+LIBEVENT_SYS_LIBS  = :libevent_core.a
+
+else
 LIBEVENT_SYS_LIBS  = event_core

-endif
+endif # STATIC_BUILD
+
+endif # msvc

 # automatically add necessary libraries
+ifeq ($(STATIC_BUILD),YES)
+LIBEVENT_SYS_LIBS_POSIX_YES = :libevent_pthreads.a
+
+else
 LIBEVENT_SYS_LIBS_POSIX_YES = event_pthreads
+
+endif # STATIC_BUILD
+
 LIBEVENT_SYS_LIBS_WIN32 = iphlpapi netapi32 ws2_32

 LIBEVENT_SYS_LIBS += $(LIBEVENT_SYS_LIBS_POSIX_$(POSIX))

I do not have a huge amount of experience with the build system so any comments you have on this approach are appreciated!

rreno17 commented 3 years ago

Whoops that last diff actually causes some issues for the host build of the PVXS shared libraries. It tries to link pvxs.so.* with the static libevent libs. This diff fixes that issue. I changed it to check against SHARED_LIBRARIES instead of STATIC_BUILD.

diff --git a/configure/CONFIG_PVXS_MODULE b/configure/CONFIG_PVXS_MODULE
index 81074fd..39846eb 100644
--- a/configure/CONFIG_PVXS_MODULE
+++ b/configure/CONFIG_PVXS_MODULE
@@ -31,12 +31,25 @@ _PVXS_DEPLIB_DIRS += $(if $(LIBEVENT),$(LIBEVENT)/lib)
 PROD_DEPLIB_DIRS   += $(_PVXS_DEPLIB_DIRS)
 SHRLIB_DEPLIB_DIRS += $(_PVXS_DEPLIB_DIRS)

+ifeq ($(SHARED_LIBRARIES), YES)
 LIBEVENT_SYS_LIBS  = event_core

-endif
+else
+LIBEVENT_SYS_LIBS  = :libevent_core.a
+
+endif # SHARED_LIBRARIES
+
+endif # msvc

 # automatically add necessary libraries
+ifeq ($(SHARED_LIBRARIES),YES)
 LIBEVENT_SYS_LIBS_POSIX_YES = event_pthreads
+
+else
+LIBEVENT_SYS_LIBS_POSIX_YES = :libevent_pthreads.a
+
+endif # SHARED_LIBRARIES
+
 LIBEVENT_SYS_LIBS_WIN32 = iphlpapi netapi32 ws2_32

 LIBEVENT_SYS_LIBS += $(LIBEVENT_SYS_LIBS_POSIX_$(POSIX))
mdavidsaver commented 3 years ago

I hadn't noticed -l: before. Neat. It does seem to be specific to GNU binutils (like so many useful tricks). Sadly, neither the Apple nor the LLVM linker understands it. It has been around since at least binutils 2.19 (circa 2011), so at least that portability threshold is cleared. This all isn't necessarily a deal breaker as we can use ifeq (YES,$(GNU)) to distinguish.

I'm still thinking about the possibilities of naming the library by full path, or switching from _SYS_LIBS to _LIBS, in some cases.

mdavidsaver commented 3 years ago

I have a change in my queue where SHARED_LIBRARIES=NO will passes -DEVENT__LIBRARY_TYPE=STATIC or --disable-shared when building the bundled libevent. While it won't by itself address the core issue, it should be sufficient for your immediate situation.

mdavidsaver commented 3 years ago

With 4ae08f8c2e02011269e930d6e3fb90e0e89c352e the combination of SHARED_LIBRARIES=NO and STATIC_BUILD=YES should work as you expect.

This issue isn't fully resolved though as SHARED_LIBRARIES=YES and STATIC_BUILD=YES is a valid combination on *nix targets.

mdavidsaver commented 3 years ago

550164b581e79f423b3a883ecd00df5f592fc245 tries to complete a fix for this issue. It changes the automatic injection of the event_core library to be conditional. If STATIC_BUILD=NO then <target>_SYS_LIBS += event_core continues to be injected. If STATIC_BUILD=YES then <target>_LIBS += event_core and event_core_DIR = ... are injected.

This mostly works, except for mingw static builds. The bundled libevent built by cmake installs static libraries with *unix style library names (libevent_core.a) rather than the windows style (event_core.lib) expected by the epics makefiles. The mingw linker can use either. The symptom being an error like:

No rule to make target '../../lib/windows-x64-mingw/event_core.lib', needed by ...
rreno17 commented 3 years ago

Thanks for your work on this! I checked out 6c67fc6 and after setting up my RELEASE.local it works out of the box on my setup from the original issue. No GNU extensions required!

mdavidsaver commented 3 years ago

Thank you for confirming. From the CI result, it looks like 6c67fc627ce651ea5c659982c8e448402b9c3f7f has fixed the mingw static build without breaking anything else. This seems good enough for the present.