Re: [hackers] [dmenu][PATCH] don't mangle CPPFLAGS with CFLAGS

From: François-Xavier Carton <fx.carton91_AT_gmail.com>
Date: Sat, 30 Jul 2022 22:14:10 +0200

On Tue, Jul 26, 2022 at 11:57:04PM +0600, NRK wrote:
> On Mon, Jul 04, 2022 at 03:01:35PM +0600, NRK wrote:
> > currently, doing something like the following would fail to build:
> >
> > $ make CFLAGS="-O3 -march=native"
> >
> > this is because CPPFLAGS get mangled with CFLAGS and so changing CFLAGS
> > kills the build.
> >
> > similarly it's conventional to have library flags in LDLIBS, so that
> > user can change LDFLAGS to add "-flto" or other flags they might want
> > without the build unnecessarily failing.

This is not the reason the two variables exist; they have a different
meaning. LDLIBS is for libraries (eg. -lfoo /path/to/bar.a), which are
input files to be linked like objects, while LDFLAGS are for
flags/options (eg. -Lpath -Wl,--as-needed). Options goes before input
files; order matters for input files but not for (most) flags. So the
traditional link command is $(LD) $(LDFLAGS) $(OBJECTS) $(LDLIBS)

Users may want to override LDFLAGS, or LDLIBS, or both in some
situations. So its unrelated to user overrides. And I think it's the
same for CPPFLAGS vs CFLAGS: the distinction is there for makefiles that
run the preprocessor separately.

(What I like to do in my makefiles, is to first define default options
that are just preferences with "?=" (eg. CFLAGS ?= -O2 -march=native)
and then add the important options with "+=" (eg. CFLAGS += -I. -std=c89),
so a user can override only the defaults with "CFLAGS=... make" or
override the whole thing with "make CFLAGS=...". Here, it does not
matter much I think, since the user is supposed to edit config.mk to his
liking.)

> > ---
> >
> > P.S: Could also drop `-std=c99` from CFLAGS and set CC to c99. But
> > didn't do that since it wasn't needed for my goal of being able to do
> > something like the following:
> >
> > $ make CFLAGS="-O3 -flto" LDFLAGS="-O3 -flto"
> >
> > Makefile | 8 +++++---
> > config.mk | 6 +++---
> > 2 files changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index a03a95c..8d25ad0 100644
> > --- a/Makefile
> > +++ b/Makefile
> > _AT_@ -11,11 +11,13 @@ all: options dmenu stest
> > options:
> > _AT_echo dmenu build options:
> > _AT_echo "CFLAGS = $(CFLAGS)"
> > + _AT_echo "CPPFLAGS = $(CPPFLAGS)"
> > _AT_echo "LDFLAGS = $(LDFLAGS)"
> > + _AT_echo "LDLIBS = $(LDLIBS)"
> > _AT_echo "CC = $(CC)"
> >
> > .c.o:
> > - $(CC) -c $(CFLAGS) $<
> > + $(CC) -c $(CPPFLAGS) $(CFLAGS) $<
> >
> > config.h:
> > cp config.def.h $_AT_
> > _AT_@ -23,10 +25,10 @@ config.h:
> > $(OBJ): arg.h config.h config.mk drw.h
> >
> > dmenu: dmenu.o drw.o util.o
> > - $(CC) -o $_AT_ dmenu.o drw.o util.o $(LDFLAGS)
> > + $(CC) -o $_AT_ dmenu.o drw.o util.o $(LDFLAGS) $(LDLIBS)
> >

I would put LDFLAGS before the object files.

> > stest: stest.o
> > - $(CC) -o $_AT_ stest.o $(LDFLAGS)
> > + $(CC) -o $_AT_ stest.o $(LDFLAGS) $(LDLIBS)
> >

(same here)

> > clean:
> > rm -f dmenu stest $(OBJ) dmenu-$(VERSION).tar.gz
> > diff --git a/config.mk b/config.mk
> > index b0bd246..a513b74 100644
> > --- a/config.mk
> > +++ b/config.mk
> > _AT_@ -24,9 +24,9 @@ INCS = -I$(X11INC) -I$(FREETYPEINC)
> > LIBS = -L$(X11LIB) -lX11 $(XINERAMALIBS) $(FREETYPELIBS)
> >
> > # flags
> > -CPPFLAGS = -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE=700 -D_POSIX_C_SOURCE=200809L -DVERSION=\"$(VERSION)\" $(XINERAMAFLAGS)
> > -CFLAGS = -std=c99 -pedantic -Wall -Os $(INCS) $(CPPFLAGS)
> > -LDFLAGS = $(LIBS)
> > +CPPFLAGS = $(INCS) -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE=700 -D_POSIX_C_SOURCE=200809L -DVERSION=\"$(VERSION)\" $(XINERAMAFLAGS)
> > +CFLAGS = -std=c99 -pedantic -Wall -Os
> > +LDLIBS = $(LIBS)
> >

Technically, only the -l flags should go there, the -L ones should go to
LDFLAGS.

> > # compiler and linker
> > CC = cc
> > --
> > 2.35.1
> >
> >
>
> Any comment on this? Or any specific reason for deviating from the
> conventional approach and mangling `CFLAGS` and `CPPFLAGS`?
>
> - NRK
>

Other than my comments on link flags, the patch looks good.
Received on Sat Jul 30 2022 - 22:14:10 CEST