Makefile: Fix check for pkg-config + fix compilation with non-GNU make (e.g. bmake and bsdmake)

[hartwork]

In reaction to #109 (review)

Addresses two issues:

• which is not the best choice for portability (reported by @eli-schwartz)
• .c files would keep getting recompiled for no good reason (reported by @edgar-bonet)

[eli-schwartz]

.c files would keep getting recompiled for no good reason (reported by @edgar-bonet)

This happens because:

ttyplot.o: require_pkgconfig

means that ttyplot has require_pkgconfig as an input, but it is a phony target -- it is always run, and produces no output, so it is always out of date.

There are 3 general solutions here:

• make require_pkgconfig produce a stamp file that avoids rerunning this rule after it has previously succeeded
• https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html use order-only prerequisites, to always run require_pkgconfig before compiling .o files, but not make .o files become out of date when that happens
• graduate to a real build system such as a configure script that can detect ncurses and generate a Makefile / build.ninja

[hartwork]

.c files would keep getting recompiled for no good reason (reported by @edgar-bonet)

This happens because:

ttyplot.o: require_pkgconfig

means that ttyplot has require_pkgconfig as an input, but it is a phony target -- it is always run, and produces no output, so it is always out of date.

@eli-schwartz that is understood, yes.

There are 3 general solutions here:

• make require_pkgconfig produce a stamp file that avoids rerunning this rule after it has previously succeeded

I didn't like that option too much e.g. because pkg-config could be uninstalled after the stamp file is created. We can argue how likely of a scenario that is, but I'm not a fan.

• https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html use order-only prerequisites, to always run require_pkgconfig before compiling .o files, but not make .o files become out of date when that happens

I knew order-only dependencies and like them but even with nothing to rebuild, command -v pkg-config is run once every time in practice, so I conclude they are not a good match with phony targets. I'm also not sure how portable order-only targets are, did not find much with a quick net search, any ideas? To reproduce what I saw:

The diff:

diff --git a/Makefile b/Makefile
index aea08f0..3d7dde1 100755
--- a/Makefile
+++ b/Makefile
@@ -22,10 +22,10 @@ clean:
        rm -f ttyplot stresstest *.o
 
 require_pkgconfig:
-       which pkg-config
+       command -v pkg-config >/dev/null
 
-ttyplot.o: require_pkgconfig
+ttyplot.o: | require_pkgconfig
 
-stresstest.o: require_pkgconfig
+stresstest.o: | require_pkgconfig
 
 .PHONY: all clean install uninstall require_pkgconfig

The make:

# make --version | head -n1
GNU Make 4.4.1

The output:

# make ; make
command -v pkg-config
command -v pkg-config

It's a bit less output than what we have in the pull request right now, but it's calling command -v the same number of times. Is this already what you had in mind or is there more potential in this that I could uncover?

• graduate to a real build system such as a configure script that can detect ncurses and generate a Makefile / build.ninja

I'd be with you here personally but #65 (comment) .

[eli-schwartz]

I knew order-only dependencies and like them but even with nothing to rebuild, command -v pkg-config is run once every time in practice, so I conclude they are not a good match with phony targets.

But running it once in the all target every time you run make, is the same number of times, right?

I'm also not sure how portable order-only targets are, did not find much with a quick net search, any ideas? To reproduce what I saw:

My suggestion was based off of the fact that the current Makefile is GNU specific already. Has it been tested with other Make implementations?

My usual goto here is to run bmake and see how that handles things. It... does not.

$ bmake
cc -pipe -g -Wall -Wextra   -o torture torture.c -lm

???

$ bmake clean
rm -f ttyplot torture *.o

$ bmake ttyplot
cc -pipe -g -Wall -Wextra   -o ttyplot ttyplot.c `pkg-config --libs ncurses`

Foiled... it is not running the require_pkgconfig code at all. Fortunately, I may not have which installed but I do have pkg-config installed. :D

I don't usually try to give suggestions that work with portable non-GNU make implementations if the existing Makefile doesn't appear to handle that to begin with.

...

This is another reason to prefer a real build system.

I'd be with you here personally but #65 (comment) .

Hmm

I'll pass on this one. Definitely not CMake, Autotools, maybe but I really dont like the complexity it brings. Ttyplot is so small that a few defines and makefile should be enough.

@tenox7 Is there a specific reason you don't like the complexity that autotools brings? Because I poked at it for a couple of minutes:

$ git --no-pager diff -M25 --stat --summary --cached -- Makefile.in Makefile
 Makefile => Makefile.in | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)
 rename Makefile => Makefile.in (25%)


$ git --no-pager diff -M25 --stat --summary --cached -- configure.ac
 configure.ac | 9 +++++++++
 1 file changed, 9 insertions(+)
 create mode 100644 configure.ac

autotools can be really, really, really simple. I didn't even use automake, so the Makefile barely changed at all. Here's what Makefile looks like after running configure:

prefix = /usr/local
exec_prefix = ${prefix}
bindir = ${exec_prefix}/bin
datarootdir = ${prefix}/share
mandir = ${datarootdir}/man

CFLAGS = -Wall -Wextra -g -O2 -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 
LDFLAGS =  -lncurses -ltinfo 

torture: LDLIBS = -lm

all: ttyplot

install: ttyplot ttyplot.1
	install -d $(DESTDIR)$(bindir)
	install -d $(DESTDIR)$(mandir)/man1
	install -m755 ttyplot   $(DESTDIR)$(bindir)
	install -m644 ttyplot.1 $(DESTDIR)$(mandir)/man1

uninstall:
	rm -f $(DESTDIR)$(bindir)/ttyplot
	rm -f $(DESTDIR)$(mandir)/man1/ttyplot.1

clean:
	rm -f ttyplot torture *.o

.PHONY: all clean install uninstall require_pkgconfig

The only thing configure does, is set the 7 variable assignments at the top of the file by doing system probing.

[hartwork]

A few things I learned in the meantime:

• make on macOS is GNU make these days (at least in GitHub Actions CI images)
• bsdmake 24 (from homebrew) does not like stresstest: LDLIBS = -lm, says make: don't know how to make LDLIBS. Stop.
• bmake 20200710 (e.g. on Ubuntu 22.04) has the same issue, saying bmake[1]: don't know how to make LDLIBS. Stop.
• bmake 20230909 is fine with it, but considers stresstest the default target, not all. Flipping two lines in Makefile makes recent bmake happy. That's what @eli-schwartz ran into above.
• bmake ignores the dependencies between .o and require_pkgconfig because of different implicit rules than GNU make

Hence:

• Supporting true BSD make would be more work, it is not supported as is
• Supporting recent bmake is cheap
• Renaming Makefile to GNUmakefile and officially requiring GNU make would be an option

[eli-schwartz]

I know (how) it can be fixed -- I just don't really consider it worth the time lol.

My motto is "if it ain't tested, or used by a core developer, don't think you actually support it". I'd just move it to GNUmakefile. Pretty much regardless of your platform, you'll be able to manually install gmake. It may not be installed by default, but it will be available. This is a much more worthwhile time investment than fixing bsdmake bugs on the rare occasion they are reported, then regressing again.

[hartwork]

@eli-schwartz full ack, well said.

[edgar-bonet]

@eli-schwartz: Thanks for bringing this up, and thanks also for the analysis of the always-rebuild problem, and for the tip about testing with bmake.

@hartwork wrote:

I'm also not sure how portable order-only targets are

bmake says it doesn't know how to make |.

My take on this is that changing the build system, whether that may be explicitly requiring GNU make or going with autotools/cmake/whatever, is a decision that belongs to @tenox7. He may not be very active on this right now, but my understanding is that he owns a few non-Linux Unix boxes (macOS and others) and he does care about portability to these Unices.

In the mean time, I suggest adding this suffix rule, which works with both GNU make and bmake:

.c:
	@pkg-config --version > /dev/null
	$(CC) $(CFLAGS) $< $(LDLIBS) -o $@

The @ prefix silences the first command. Yet, if pkg-config is not found, it spits an explicit error message.

[hartwork]

@edgar-bonet I think this picture i missing that we already have torture: LDLIBS = -lm (or stresstest: LDLIBS = -lm now) in the Makefile since commit 70e28d5 of release 1.5.1 which is already no longer portable regarding Makefile; could have been by accident though (@tenox7?). Regarding that trouble line we could:

• a) Keep the line and be official about the requirement for GNU make (which favors the rename to avoid contact with non-GNU make)
• b) Get stresstest out of target all again and accept that not all users will be able to build the stresstest tool code, i.e. partial support. We should then make the CI cover compilation with bsdmake (homebrew) and old bmake (ubuntu) and recent bmake (homebrew) also to protect against regressions and get it supported in the sense that @eli-schwartz defined supported above (I have a commit like that in git reflog somewhere already.) Feels a bit like investing in museum support though.
• c) Link all executable against -lm all the time (and then the user can use -Wl,--as-needed for LDFLAGS to avoid overlinking) and make the makefile 100% compatible with non-recent BSD make. Maybe not the worst?

The prefix rule you mention would need to include LDFLAGS also and in the right place to not break -Wl,--as-needed, would need to check what the right place is to not be wrong about it.

I should also not that we have things in the code now that required POSIX >=2008 and C99 so supporting non-GNU make older than those two is potential "money out the window", at least with moderately consistent setups in mind.

What do you think?

[hartwork]

The prefix rule you mention would need to include LDFLAGS also and in the right place to not break -Wl,--as-needed, [..].

@edgar-bonet PS: For proof of that, I'm tricking GNU make to print its implicit rules for direct "app.c -> app" compilation to make it reveal use of LDFLAGS and where:

# cd "$(mktemp -d)" && echo missing{,.c} | xargs -n1 touch && CC='true $${CC}' CFLAGS='$${CFLAGS}' LDFLAGS='$${LDFLAGS}' LDLIBS='$${LDLIBS}' make missing | sed 's,^true ,,'
${CC} ${CFLAGS}  ${LDFLAGS}  missing.c  ${LDLIBS} -o missing

# make --version | head -n1
GNU Make 4.4.1

I should have started with make -p -f <(echo 'all:') right away, where GNU make tells us about its implicit rules verbatim. To quote the relevant part of its output:

LINK.c = $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH)

.c:
        $(LINK.c) $^ $(LOADLIBES) $(LDLIBS) -o $@

After inlining and dropping CPPFLAGS, TARGET_ARCH and LOADLIBES we'd end up with…

.c:
        $(CC) $(CFLAGS) $(LDFLAGS) $^ $(LDLIBS) -o $@

…but we don't have to inline $(LINK.c).

[hartwork]

@edgar-bonet I have put your idea into place now, adding LDFLAGS support, a fix for LDLIBS, CI coverage for stricter makes as well as and linker flag --as-needed to protect against future regressions. I believe this gets us full cover and compatibility, I'm looking forward to review.