shared.ldflags broken when building with helper-library

[umlaeute]

now that we have a simple test-suite it starts to catch errors :-)

using the shared.ldflags variable fails to expand the shared.lib variable.

Building the tests/multishared/ project I get:

$ make -C tests/multishared
make: Entering directory '<<pd-lib-builder>>/tests/multishared'
++++ info: using Makefile.pdlibbuilder version 0.6.0
++++ info: using Pd API /usr/include/pd/m_pd.h
++++ info: making target all in lib multishared
++++ info: making shared.o in lib multishared
cc -DPD -I "/usr/include/pd" -DUNIX  -fPIC  -Wall -Wextra -Wshadow -Winline -Wstrict-aliasing -O3 -ffast-math -funroll-loops -fomit-frame-pointer -march=core2 -mfpmath=sse -msse -msse2 -msse3 -o shared.o -c shared.c
++++ info: linking objects in shared lib libmultishared.pd_linux.so
cc -rdynamic -fPIC -shared -Wl,-soname,  -o libmultishared.pd_linux.so shared.o -lc -lm
/usr/bin/ld: SONAME must not be empty string; ignored
++++ info: making multisharedB.o in lib multishared
cc -DPD -I "/usr/include/pd" -DUNIX  -fPIC  -Wall -Wextra -Wshadow -Winline -Wstrict-aliasing -O3 -ffast-math -funroll-loops -fomit-frame-pointer -march=core2 -mfpmath=sse -msse -msse2 -msse3 -o multisharedB.o -c multisharedB.c
++++ info: linking objects in multisharedB.pd_linux for lib multishared
cc -rdynamic -shared -fPIC -Wl,-rpath,"\$ORIGIN",--enable-new-dtags    -o multisharedB.pd_linux multisharedB.o  -lc -lm   libmultishared.pd_linux.so
++++ info: making multisharedA.o in lib multishared
cc -DPD -I "/usr/include/pd" -DUNIX  -fPIC  -Wall -Wextra -Wshadow -Winline -Wstrict-aliasing -O3 -ffast-math -funroll-loops -fomit-frame-pointer -march=core2 -mfpmath=sse -msse -msse2 -msse3 -o multisharedA.o -c multisharedA.c
++++ info: linking objects in multisharedA.pd_linux for lib multishared
cc -rdynamic -shared -fPIC -Wl,-rpath,"\$ORIGIN",--enable-new-dtags    -o multisharedA.pd_linux multisharedA.o  -lc -lm   libmultishared.pd_linux.so
++++info: target all in lib multishared completed
make: Leaving directory '<<pd-lib-builder>>/tests/multishared'
$

note the lines:

cc -rdynamic -fPIC -shared -Wl,-soname,  -o libmultishared.pd_linux.so shared.o -lc -lm
/usr/bin/ld: SONAME must not be empty string; ignored

my compiler (gcc-9.2.1) is quite forgiving, so the test actually succeeds.
however, on other systems (notably an android test-build), the -Wl,-soname, flag (without an actual SONAME) raises an error (that's how i discovered it).

Running git bisect I see that this was introduced with a6975e9 (reordering makefile sections)

[umlaeute]

sidenote:

i also noticed that the shared.ldflags definition is sometimes a simply expanded (:=) variable and sometimes a recursively expanded (=) variable.

Linux:

pd-lib-builder/Makefile.pdlibbuilder

Line 496 in 3eb4832

shared.ldflags := -rdynamic -fPIC -shared -Wl,-soname,$(shared.lib)

Darwin:

pd-lib-builder/Makefile.pdlibbuilder

Lines 518 to 520 in 3eb4832

	shared.ldflags = -dynamiclib -undefined dynamic_lookup \
	-install_name @loader_path/$(shared.lib) \
	-compatibility_version 1 -current_version 1.0

[katjav]

This was a useful reminder that it a successful build and load doesn't tell everything. Commit e6cff66 is meant to solve this issue.

The inconsistency between immediate and deferred expansion in Linux vs Darwin might have been accidental but I tend to use simply expanded variables whenever possible.

[umlaeute]

thanks for the quick fix.

ad inconsistencies: it seems that on Windows, the shared.ldflags are still using immediate expansion.

ad reminder: probably the test-builds should be run with -Werror...

[katjav]

ad inconsistencies: it seems that on Windows, the shared.ldflags are still using immediate expansion.

For Windows shared.ldflags doesn't use a variable that is not yet defined, therefore I would stay with immediate expansion.

[katjav]

probably the test-builds should be run with -Werror...

-Werror could be passed for all build phases with this in the test Makefiles (after inclusion of Makefile.pdlibbuilder):

all: c.flags += -Werror
all: cxx.flags += -Werror
all: c.ldflags += -Werror
all: cxx.ldflags += -Werror
all: shared.ldflags += -Werror

Or alternatively there could be a special target like allerror for which this is specified. Not sure if gcc honours -Werror in the context of the linker though.

[umlaeute]

for the linker it's --fatal-warnings (or -Wl,--fatal-warnings when called via the compiler).

i'm not entirely sure whether we should enable those flags unconditionally though.

i also wonder, what would be the most scalable way to apply this to all test-projects.

probably something like:

tests/single/Makefile:

# [...]
PDLIBBUILDER_DIR=..
include $(PDLIBBUILDER_DIR)/Makefile.pdlibbuilder
# [...]

tests/Makefile.pdlibbuilder:

# thin wrapper around Makefile.pdlibbuilder to enable fatal warnings
include ../../Makefile.pdlibbuilder
ifdef fatal-warnings
all: c.flags += -Werror
all: cxx.flags += -Werror
all: c.ldflags += -Wl,--fatal-warnings
all: cxx.ldflags += -Wl,--fatal-warnings
all: shared.ldflags += -Wl,--fatal-warnings
endif

[katjav]

Admittedly that is a clever trick.

The "Fakefile.pdlibbuilder" confused me therefore I tried to figure out different ways to get the same result. When those extra flags are defined for target vars in tests/Makefile they do get passed to the submakes. But, oddly and annoyingly, not for target all.

The most flexible approach is probably to put all test project boilerplate into tests/Makefile.test, and only specify project-specific stuff in the test project makefiles. Then we can add more variables and targets as they may come up in the future. Something like this (not tested yet):

tests/single/Makefile:

[...]
include ../Makefile.test
[...]

tests/Makefile.test:

PDLIBBUILDER_DIR=../..
include $(PDLIBBUILDER_DIR)/Makefile.pdlibbuilder

ifdef fatal-warnings
all: c.flags += -Werror
all: cxx.flags += -Werror
all: c.ldflags += -Wl,--fatal-warnings
all: cxx.ldflags += -Wl,--fatal-warnings
all: shared.ldflags += -Wl,--fatal-warnings
endif

buildcheck: all
    $(foreach v, $(executables), test -e '$v';)

installcheck: install
    $(foreach v, $(executables), test -e $(installpath)/'$v';)
    $(foreach v, $(datafiles), test -e $(installpath)/'$v';)

[umlaeute]

well, i deliberately re-used Makefile.pdlibbuider for the wrapper, so the test-projects could still serve as a template for real projects (where you would only need to adjust to the PDIBBUILDER_DIR variable.

but more importantly: i kind of dislike the generic formulation of the buildcheck/installcheck targets, as this makes for tautological tests: i'm pretty sure that Makefile.pdlibbuilder will always have all the $(executables) ready when the all target succeeds, so it's kind of pointless to test for that. I'm much more interested whether Makefile.pdlibbuilder agrees with me on what $(executables) actually expands to.

[katjav]

well, i deliberately re-used Makefile.pdlibbuider for the wrapper, so the test-projects could still serve as a template for real projects

Agreed, the tests projects should be transparent as examples and therefore they should include Makefile.pdlibbuilder.

In the meantime I'm thinking that the best place for the fatal-warnings option is in Makefile.pdlibbuilder itself, with the advantage that it can be used with any Pd lib in testing phase. Then we don't need a wrapper construction where one Makefile.pdlibbuilder does a bit more than the other.

About tautological tests: clear, so we don't need a boilerplate file (so far).