search

tenox7 / ttyplot

a realtime plotting utility for terminal/console with data input from stdin
Apache License 2.0
957 stars 42 forks source link

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

Closed hartwork closed 7 months ago

hartwork commented 7 months ago

In reaction to https://github.com/tenox7/ttyplot/pull/109#pullrequestreview-1749382975

Addresses two issues:

eli-schwartz commented 7 months ago

.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:

hartwork commented 7 months ago

.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.

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 https://github.com/tenox7/ttyplot/issues/65#issuecomment-1461874941 .

eli-schwartz commented 7 months ago

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 commented 7 months ago

A few things I learned in the meantime:

Hence:

eli-schwartz commented 7 months ago

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 commented 7 months ago

@eli-schwartz full ack, well said.

edgar-bonet commented 7 months ago

@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 commented 7 months ago

@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 70e28d5284de1d94d7b4b8188dab68423ef798d5 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:

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 commented 7 months ago

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 commented 7 months ago

@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.