raboof / notion

Tiling tabbed window manager
https://notionwm.net/
GNU Lesser General Public License v2.1
268 stars 63 forks source link

EXTRA_LIBS doesn't work anymore #349

Closed jeffpc closed 1 year ago

jeffpc commented 1 year ago

Since the cleanup done in d33784cd48e9498eb71ff2a9ce18124ad24964b8, EXTRA_LIBS is completely unused. In the past I used it to add libintl to the linker invocation otherwise the build on FreeBSD fails with a linker error. The following patch fixes it but I'm not sure if that's the right way to do it:

diff --git a/system-autodetect.mk b/system-autodetect.mk
index 83b78c05..f5022d61 100644
--- a/system-autodetect.mk
+++ b/system-autodetect.mk
@@ -184,7 +186,7 @@ endif
 CFLAGS += $(WARN) $(DEFINES) $(INCLUDES) $(EXTRA_INCLUDES) \
           -DHAS_SYSTEM_ASPRINTF=$(HAS_SYSTEM_ASPRINTF)

-LDFLAGS ?= -Wl,-O1 -Wl,--as-needed
+LDFLAGS ?= -Wl,-O1 -Wl,--as-needed $(EXTRA_LIBS)
 EXPORT_DYNAMIC=-Xlinker --export-dynamic

 # The following options are mainly for development use and can be used
raboof commented 1 year ago

/cc @SoapGentoo could you have a look at this?

SoapGentoo commented 1 year ago

@raboof sure!

SoapGentoo commented 1 year ago

@jeffpc have you tried loading LIBS from the environment? For instance

LIBS=-lmp3lame make

I just used the LAME library as an example to inject something. With this I get linker lines such as:

cc -g -Os -W -Wall -pedantic -DCF_XFREE86_TEXTPROP_BUG_WORKAROUND -DHAVE_X11_XFT -DHAVE_X11_BMF -I/usr/include/freetype2 -I/usr/include/harfbuzz -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -pthread  -I..  -DHAS_SYSTEM_ASPRINTF=1 -DNOTION_RELEASE='"4.0.2-46-g26def019"' -D_XOPEN_SOURCE -D_XOPEN_SOURCE_EXTENDED -std=c99 -DCF_HAS_VA_COPY -fPIC -DPIC -MMD -Wl,-O1 -Wl,--as-needed -shared -Wl,-soname -Wl,mod_notionflux.so mod_notionflux.o exports.o  -o mod_notionflux.so -lmp3lame -lX11 -lXext  -lXft

where you can see that it clearly picked up the flag I gave it in LIBS. The change in d33784cd48e9498eb71ff2a9ce18124ad24964b8 was done on purpose: In general, injecting libraries in LDFLAGS is wrong, since LDFLAGS has to come very early in the link line (since it only affects objects after its arguments). When using -Wl,--as-needed, most libraries will get excluded, meaning that even if you inject a library that is necessary to fix some missing dependency or such, the linker won't pick it up since it hasn't seen the object files that require it.

jeffpc commented 1 year ago

LIBS="-L/usr/local/lib -lintl" gmake worked.

In the past, I've seen what you say about LDFLAGS happen with static libs but not with shared objects. It makes sense to have one mechanism that that always works regardless of library type. I'm also for having notion's build be more...normal :)

Anyway, I suppose the top-level system-autodetect.mk should be cleaned up to at the very least remove the (now) misleading EXTRA_LIBS. It looks like EXTRA_INCLUDES should probably get a similar treatment.

Thanks for taking a look.

SoapGentoo commented 1 year ago

@raboof part of the problem is that the current Makefile build system is a bit special. Would you be open to something like Autotools or Meson? I would do the work for you obviously.

raboof commented 1 year ago

I would be in favor of making the Makefile "more normal", but I'm not sure whether migrating to a completely new system would be worth it... especially since there's so many to choose from (indeed autotools, meson, cmake, ninja, ....)

SoapGentoo commented 1 year ago

I've tried making it more normal, and have mostly succeeded (LIBS is the common idiom), but there's a ton of issues with the current one still. Trying to autodetect whether libintl is needed in a Makefile is fraught with peril (because conditional constructs in make aren't portable) and the lua handling could we improved. I think the current Makefile system is probably about as good as it gets...

raboof commented 1 year ago

I've tried making it more normal, and have mostly succeeded (LIBS is the common idiom)

Jup, thanks for that!

but there's a ton of issues with the current one still. Trying to autodetect whether libintl is needed in a Makefile is fraught with peril (because conditional constructs in make aren't portable) and the lua handling could we improved. I think the current Makefile system is probably about as good as it gets...

I agree it's not ideal, but I'm hesitant to introduce entirely new tooling. That said, if you contribute it and there's no grave concerns by other contributors (I think there's still some people building Notion for FreeBSD and perhaps Mac?) then I'll likely merge it :)

wilhelmy commented 1 year ago

because conditional constructs in make aren't portable

FWIW we already depend on gmake. From my time as a BSD packager I remember recommendations by the community to stick to gmake. BSD make is not very widespread (and AFAIK fragmented between different BSD dialects). POSIX make is a very limited standard that only standardizes the absolute bare bones.

wilhelmy commented 1 year ago

As for switching the build system to something else, after having played with multiple different build systems over the years, I can honestly say that I hate them all.

As for Lua handling (I re-wrote the lua-detection at some point) - is there anything missing, or is it about the gmake dependency?

SoapGentoo commented 1 year ago

overall building notion isn't a major pain point, but every self-written Makefile is special in its own way. If there's no appetite for a change, then we can keep it as is.

That said, @raboof could we maybe get a 4.0.3 release? Gentoo is currently using a git snapshot in order to get all the makefile fixes that make building it a lot easier in Gentoo.

raboof commented 1 year ago

That said, @raboof could we maybe get a 4.0.3 release? Gentoo is currently using a git snapshot in order to get all the makefile fixes that make building it a lot easier in Gentoo.

While I'm perfectly OK with downstream building from git (in principle the main branch should always be 'healthy'), I'll add making a release to my (long) TODO list ;)

wilhelmy commented 1 year ago

Sorry if I came across too negatively. I'm not really opposed to a different build system in theory. Pull requests are always welcome, but plain old gmake isn't so bad. What were you thinking about, meson/ninja?

I noticed lua-detect.mk has some shortcomings such as when lua isn't installed. I considered packaging notion for pkgsrc but if we're replacing the build system I might as well wait since I doubt there's active users at the moment.

SoapGentoo commented 1 year ago

As I said, I think notion's custom make-based buildsystem is ok, but it has the hallmarks of all hand-written buildsystem: overriding options requires studying the build system (think of the -lintl thing instead of having a script that correctly autodetects it) and most of the times things like pkg-config are eschewed for "simplicity", which only makes these build systems more brittle in practice.

My suggestion would have been meson/ninja, since its configure time is fast and the meson.build files are readable, but I'm also fine with the current situation.