Re: [hackers] [sent] [PATCH 3/3] Makefile: config.mk: Improve Makefile and config.mk

2022-06-30 Thread Tom Schwindl 
> Hi Tom o/

Hi Quentin \o/

> > > >  # includes and libs
> > > > -INCS = -I. -I/usr/include -I/usr/include/freetype2 -I${X11INC}
> > > > -LIBS = -L/usr/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
> > > > +INCS = -I/usr/include/freetype2 -I${X11INC}
> > > > +LIBS = -lm -L${X11LIB} -lXft -lfontconfig -lX11
> > > 
> > > The -L option should go into LDFLAGS
> > 
> > Since $LIBS is only used to provide $LDFLAGS, we could copy it's contents
> > directly into $LDFLAGS and drop $LIBS. Good catch.
> 
> No, those are two different things.
> 
> LDFLAGS are linker configuration parameters,
> like indeed where to search libraries amongst other things.
> 
> The LIBS macro is to indicate what libraries to link against;
> those are not configuration parameters,
> but actual ordered operands in the linking chain.

Now that I'm looking over this again, I think the `-L` option can simply be
dropped. It isn't necessary in this context.

We could do it like surf (at least iirc) and drop $LDFLAGS in favour of
$LIBS. Let $LDFLAGS be an optional configuration option for the user.

> > > >  # flags
> > > >  CPPFLAGS = -DVERSION=\"${VERSION}\" -D_XOPEN_SOURCE=600
> > > > -CFLAGS += -g -std=c99 -pedantic -Wall ${INCS} ${CPPFLAGS}
> > > > -LDFLAGS += -g ${LIBS}
> > > > -#CFLAGS += -std=c99 -pedantic -Wall -Os ${INCS} ${CPPFLAGS}
> > > > -#LDFLAGS += ${LIBS}
> > > > +CFLAGS = -std=c99 -pedantic -Wall -Wstrict-prototypes 
> > > > -Wold-style-definition -Os ${INCS} ${CPPFLAGS}
> > > 
> > > The -std option isn't necessary with the default CC.
> > > I'd remove any warning from production build,
> > > warnings are for development,
> > > then developpers can set whatever warnings they like.
> > > 
> > > >  # compiler and linker
> > > > -CC ?= cc
> > > > +CC = cc
> > > 
> > > This should be removed altogether if you want a standard Makefile,
> > > then CC will just be the expected c99.
> > 
> > I'd keep the `-std` option and the CC assignment.
> > We want to be C99 compliant regardless of which compiler the user decides 
> > to use.
> > Additionally, CC is a configuration option in all suckless programs and
> > to be consistent here seems reasonable, in my opinion.
> 
> That's the point. To be compliant with C99, use a c99 compiler,
> which is the default compiler called by standard Makefiles.
> By defining CC to something non-standard,
> we introduce an indirection change here,
> that then forces the (non-standard) -std option to be passed to random 
> compiler
> in the hope that it'll compatible with it.
> If the user decides to replace the standard compiler
> with another one, then they'll know which option to pass to it to make it 
> respecting
> the original contract.
> 
> As for some of the other suckless programs that specify CC,
> that's just a legacy format that should be replaced anyway IMO.
> 

On most systems `c99` is just a simple wrapper script with, roughly, the
following content:

exec /usr/bin/cc -std=c99 "$@"

`-std` is already used here and we don't gain much by using the systems `c99`.
But in the case in which `c99` isn't a script (which we probably should assume),
your way is definitely the better one.
Luckily, that's something the maintainer has to decide.

Btw. does anyone know if he is still active on this list (or in general)?

-- 
Best Regards,
Tom Schwindl

Re: [hackers] [sent] [PATCH 3/3] Makefile: config.mk: Improve Makefile and config.mk

2022-06-29 Thread Quentin Rameau 
> Hi Quentin,

Hi Tom o/

> > >  # includes and libs
> > > -INCS = -I. -I/usr/include -I/usr/include/freetype2 -I${X11INC}
> > > -LIBS = -L/usr/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
> > > +INCS = -I/usr/include/freetype2 -I${X11INC}
> > > +LIBS = -lm -L${X11LIB} -lXft -lfontconfig -lX11
> > 
> > The -L option should go into LDFLAGS
> 
> Since $LIBS is only used to provide $LDFLAGS, we could copy it's contents
> directly into $LDFLAGS and drop $LIBS. Good catch.

No, those are two different things.

LDFLAGS are linker configuration parameters,
like indeed where to search libraries amongst other things.

The LIBS macro is to indicate what libraries to link against;
those are not configuration parameters,
but actual ordered operands in the linking chain.

> > >  # flags
> > >  CPPFLAGS = -DVERSION=\"${VERSION}\" -D_XOPEN_SOURCE=600
> > > -CFLAGS += -g -std=c99 -pedantic -Wall ${INCS} ${CPPFLAGS}
> > > -LDFLAGS += -g ${LIBS}
> > > -#CFLAGS += -std=c99 -pedantic -Wall -Os ${INCS} ${CPPFLAGS}
> > > -#LDFLAGS += ${LIBS}
> > > +CFLAGS = -std=c99 -pedantic -Wall -Wstrict-prototypes 
> > > -Wold-style-definition -Os ${INCS} ${CPPFLAGS}
> > 
> > The -std option isn't necessary with the default CC.
> > I'd remove any warning from production build,
> > warnings are for development,
> > then developpers can set whatever warnings they like.
> > 
> > >  # compiler and linker
> > > -CC ?= cc
> > > +CC = cc
> > 
> > This should be removed altogether if you want a standard Makefile,
> > then CC will just be the expected c99.
> 
> I'd keep the `-std` option and the CC assignment.
> We want to be C99 compliant regardless of which compiler the user decides to 
> use.
> Additionally, CC is a configuration option in all suckless programs and
> to be consistent here seems reasonable, in my opinion.

That's the point. To be compliant with C99, use a c99 compiler,
which is the default compiler called by standard Makefiles.
By defining CC to something non-standard,
we introduce an indirection change here,
that then forces the (non-standard) -std option to be passed to random compiler
in the hope that it'll compatible with it.
If the user decides to replace the standard compiler
with another one, then they'll know which option to pass to it to make it 
respecting
the original contract.

As for some of the other suckless programs that specify CC,
that's just a legacy format that should be replaced anyway IMO.

Re: [hackers] [sent] [PATCH 3/3] Makefile: config.mk: Improve Makefile and config.mk

2022-06-27 Thread Tom Schwindl 
Hi Quentin,

> I think that you can also drop the || altogether,
> make will just tell you whatever problem it encountered
> while trying to invoque cscope.
> 
> > -   @mkdir -p ${DESTDIR}${MANPREFIX}/man1
> > -   @cp sent.1 ${DESTDIR}${MANPREFIX}/man1/sent.1
> > -   @chmod 644 ${DESTDIR}${MANPREFIX}/man1/sent.1
> > +   mkdir -p ${DESTDIR}${MANPREFIX}/man1
> > +   cp sent.1 ${DESTDIR}${MANPREFIX}/man1/sent.1
> 
> I'd cp -f here too.

I agree.

> >  # includes and libs
> > -INCS = -I. -I/usr/include -I/usr/include/freetype2 -I${X11INC}
> > -LIBS = -L/usr/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
> > +INCS = -I/usr/include/freetype2 -I${X11INC}
> > +LIBS = -lm -L${X11LIB} -lXft -lfontconfig -lX11
> 
> The -L option should go into LDFLAGS

Since $LIBS is only used to provide $LDFLAGS, we could copy it's contents
directly into $LDFLAGS and drop $LIBS. Good catch.

> >  # flags
> >  CPPFLAGS = -DVERSION=\"${VERSION}\" -D_XOPEN_SOURCE=600
> > -CFLAGS += -g -std=c99 -pedantic -Wall ${INCS} ${CPPFLAGS}
> > -LDFLAGS += -g ${LIBS}
> > -#CFLAGS += -std=c99 -pedantic -Wall -Os ${INCS} ${CPPFLAGS}
> > -#LDFLAGS += ${LIBS}
> > +CFLAGS = -std=c99 -pedantic -Wall -Wstrict-prototypes 
> > -Wold-style-definition -Os ${INCS} ${CPPFLAGS}
> 
> The -std option isn't necessary with the default CC.
> I'd remove any warning from production build,
> warnings are for development,
> then developpers can set whatever warnings they like.
> 
> >  # compiler and linker
> > -CC ?= cc
> > +CC = cc
> 
> This should be removed altogether if you want a standard Makefile,
> then CC will just be the expected c99.

I'd keep the `-std` option and the CC assignment.
We want to be C99 compliant regardless of which compiler the user decides to 
use.
Additionally, CC is a configuration option in all suckless programs and
to be consistent here seems reasonable, in my opinion.

-- 
Best Regards,
Tom Schwindl

Re: [hackers] [sent] [PATCH 3/3] Makefile: config.mk: Improve Makefile and config.mk

2022-06-27 Thread Tom Schwindl 
Hi Hiltjo,

> I would remove the echo also for the verbose changes.

I get the point of consistency, but I think an output as the following is more
confusing than helpful.

echo sent build options:
sent build options:

This seems to just pollute stdout with unnecessary information.
I've even played with the idea of removing all echos and just keep the ones
under the `options` target, because I don't think the others add much value.
But I'll wait for the maintainers feedback before rewriting the whole Makefile.

> I see the CFLAGS is also changed. You should document it in the commit 
> message.

I agree.

> I'm not sure I agree with adding -Wall or the many -Woptions though.
> 
> It is up to the current sent maintainer to decide.

Since most Suckless tools include a few `-W` options, NRK's suggestion
was sound to me, but that's indeed a point the maintainer has to decide.

And just for the record: `-Wall` was already there, I just added the other
`-W` options.

-- 
Best Regards,
Tom Schwindl

Re: [hackers] [sent] [PATCH 3/3] Makefile: config.mk: Improve Makefile and config.mk

2022-06-26 Thread Quentin Rameau 
Hi Tom,

> Let the Makefile be a bit more verbose and remove unnecessary extensions
> and flags in config.mk.
> ---
  
>  cscope: ${SRC} config.h
>   @echo cScope
> - @cscope -R -b || echo cScope not installed
> + cscope -R -b || echo cScope not installed

I think that you can also drop the || altogether,
make will just tell you whatever problem it encountered
while trying to invoque cscope.

> - @mkdir -p ${DESTDIR}${MANPREFIX}/man1
> - @cp sent.1 ${DESTDIR}${MANPREFIX}/man1/sent.1
> - @chmod 644 ${DESTDIR}${MANPREFIX}/man1/sent.1
> + mkdir -p ${DESTDIR}${MANPREFIX}/man1
> + cp sent.1 ${DESTDIR}${MANPREFIX}/man1/sent.1

I'd cp -f here too.

>  # includes and libs
> -INCS = -I. -I/usr/include -I/usr/include/freetype2 -I${X11INC}
> -LIBS = -L/usr/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
> +INCS = -I/usr/include/freetype2 -I${X11INC}
> +LIBS = -lm -L${X11LIB} -lXft -lfontconfig -lX11

The -L option should go into LDFLAGS

>  # flags
>  CPPFLAGS = -DVERSION=\"${VERSION}\" -D_XOPEN_SOURCE=600
> -CFLAGS += -g -std=c99 -pedantic -Wall ${INCS} ${CPPFLAGS}
> -LDFLAGS += -g ${LIBS}
> -#CFLAGS += -std=c99 -pedantic -Wall -Os ${INCS} ${CPPFLAGS}
> -#LDFLAGS += ${LIBS}
> +CFLAGS = -std=c99 -pedantic -Wall -Wstrict-prototypes -Wold-style-definition 
> -Os ${INCS} ${CPPFLAGS}

The -std option isn't necessary with the default CC.
I'd remove any warning from production build,
warnings are for development,
then developpers can set whatever warnings they like.

>  # compiler and linker
> -CC ?= cc
> +CC = cc

This should be removed altogether if you want a standard Makefile,
then CC will just be the expected c99.

Re: [hackers] [sent] [PATCH 3/3] Makefile: config.mk: Improve Makefile and config.mk

2022-06-26 Thread Hiltjo Posthuma 
On Sun, Jun 26, 2022 at 11:07:52AM +, Tom Schwindl wrote:
> Let the Makefile be a bit more verbose and remove unnecessary extensions
> and flags in config.mk.
> ---
>  Makefile  | 37 +++--
>  config.mk | 18 --
>  2 files changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 56e636734216..d73909283970 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,5 +1,6 @@
>  # sent - plain text presentation tool
>  # See LICENSE file for copyright and license details.
> +.POSIX:
>  
>  include config.mk
>  
> @@ -18,43 +19,43 @@ config.h:
>   cp config.def.h config.h
>  
>  .c.o:
> - @echo CC $<
> - @${CC} -c ${CFLAGS} $<
> + @echo ${CC} $<
> + ${CC} -c ${CFLAGS} $<
>  
>  ${OBJ}: config.h config.mk
>  
>  sent: ${OBJ}
> - @echo CC -o $@
> - @${CC} -o $@ ${OBJ} ${LDFLAGS}
> + @echo ${CC} -o $@
> + ${CC} -o $@ ${OBJ} ${LDFLAGS}
>  
>  cscope: ${SRC} config.h
>   @echo cScope
> - @cscope -R -b || echo cScope not installed
> + cscope -R -b || echo cScope not installed
>  
>  clean:
>   @echo cleaning
> - @rm -f sent ${OBJ} sent-${VERSION}.tar.gz
> + rm -f sent ${OBJ} sent-${VERSION}.tar.gz
>  
>  dist: clean
>   @echo creating dist tarball
> - @mkdir -p sent-${VERSION}
> - @cp -R LICENSE Makefile config.mk config.def.h ${SRC} sent-${VERSION}
> - @tar -cf sent-${VERSION}.tar sent-${VERSION}
> - @gzip sent-${VERSION}.tar
> - @rm -rf sent-${VERSION}
> + mkdir -p sent-${VERSION}
> + cp -R LICENSE Makefile config.mk config.def.h ${SRC} sent-${VERSION}
> + tar -cf sent-${VERSION}.tar sent-${VERSION}
> + gzip sent-${VERSION}.tar
> + rm -rf sent-${VERSION}
>  
>  install: all
>   @echo installing executable file to ${DESTDIR}${PREFIX}/bin
> - @mkdir -p ${DESTDIR}${PREFIX}/bin
> - @cp -f sent ${DESTDIR}${PREFIX}/bin
> - @chmod 755 ${DESTDIR}${PREFIX}/bin/sent
> + mkdir -p ${DESTDIR}${PREFIX}/bin
> + cp -f sent ${DESTDIR}${PREFIX}/bin
> + chmod 755 ${DESTDIR}${PREFIX}/bin/sent
>   @echo installing manual page to ${DESTDIR}${MANPREFIX}/man1
> - @mkdir -p ${DESTDIR}${MANPREFIX}/man1
> - @cp sent.1 ${DESTDIR}${MANPREFIX}/man1/sent.1
> - @chmod 644 ${DESTDIR}${MANPREFIX}/man1/sent.1
> + mkdir -p ${DESTDIR}${MANPREFIX}/man1
> + cp sent.1 ${DESTDIR}${MANPREFIX}/man1/sent.1
> + chmod 644 ${DESTDIR}${MANPREFIX}/man1/sent.1
>  
>  uninstall:
>   @echo removing executable file from ${DESTDIR}${PREFIX}/bin
> - @rm -f ${DESTDIR}${PREFIX}/bin/sent
> + rm -f ${DESTDIR}${PREFIX}/bin/sent
>  
>  .PHONY: all options clean dist install uninstall cscope
> diff --git a/config.mk b/config.mk
> index d61c55437f46..15fdcfa10dc7 100644
> --- a/config.mk
> +++ b/config.mk
> @@ -11,20 +11,18 @@ X11INC = /usr/X11R6/include
>  X11LIB = /usr/X11R6/lib
>  
>  # includes and libs
> -INCS = -I. -I/usr/include -I/usr/include/freetype2 -I${X11INC}
> -LIBS = -L/usr/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
> +INCS = -I/usr/include/freetype2 -I${X11INC}
> +LIBS = -lm -L${X11LIB} -lXft -lfontconfig -lX11
>  # OpenBSD (uncomment)
> -#INCS = -I. -I${X11INC} -I${X11INC}/freetype2
> +#INCS = -I${X11INC} -I${X11INC}/freetype2
>  # FreeBSD (uncomment)
> -#INCS = -I. -I/usr/local/include -I/usr/local/include/freetype2 -I${X11INC}
> -#LIBS = -L/usr/local/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
> +#INCS = -I/usr/local/include/freetype2 -I${X11INC}
> +#LIBS = -lm -L${X11LIB} -lXft -lfontconfig -lX11
>  
>  # flags
>  CPPFLAGS = -DVERSION=\"${VERSION}\" -D_XOPEN_SOURCE=600
> -CFLAGS += -g -std=c99 -pedantic -Wall ${INCS} ${CPPFLAGS}
> -LDFLAGS += -g ${LIBS}
> -#CFLAGS += -std=c99 -pedantic -Wall -Os ${INCS} ${CPPFLAGS}
> -#LDFLAGS += ${LIBS}
> +CFLAGS = -std=c99 -pedantic -Wall -Wstrict-prototypes -Wold-style-definition 
> -Os ${INCS} ${CPPFLAGS}
> +LDFLAGS = ${LIBS}
>  
>  # compiler and linker
> -CC ?= cc
> +CC = cc
> -- 
> 2.36.1
> 
> 

I would remove the echo also for the verbose changes.

I see the CFLAGS is also changed. You should document it in the commit message.
I'm not sure I agree with adding -Wall or the many -Woptions though.

It is up to the current sent maintainer to decide.

-- 
Kind regards,
Hiltjo

[hackers] [sent] [PATCH 3/3] Makefile: config.mk: Improve Makefile and config.mk

2022-06-26 Thread Tom Schwindl 
Let the Makefile be a bit more verbose and remove unnecessary extensions
and flags in config.mk.
---
 Makefile  | 37 +++--
 config.mk | 18 --
 2 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/Makefile b/Makefile
index 56e636734216..d73909283970 100644
--- a/Makefile
+++ b/Makefile
@@ -1,5 +1,6 @@
 # sent - plain text presentation tool
 # See LICENSE file for copyright and license details.
+.POSIX:
 
 include config.mk
 
@@ -18,43 +19,43 @@ config.h:
cp config.def.h config.h
 
 .c.o:
-   @echo CC $<
-   @${CC} -c ${CFLAGS} $<
+   @echo ${CC} $<
+   ${CC} -c ${CFLAGS} $<
 
 ${OBJ}: config.h config.mk
 
 sent: ${OBJ}
-   @echo CC -o $@
-   @${CC} -o $@ ${OBJ} ${LDFLAGS}
+   @echo ${CC} -o $@
+   ${CC} -o $@ ${OBJ} ${LDFLAGS}
 
 cscope: ${SRC} config.h
@echo cScope
-   @cscope -R -b || echo cScope not installed
+   cscope -R -b || echo cScope not installed
 
 clean:
@echo cleaning
-   @rm -f sent ${OBJ} sent-${VERSION}.tar.gz
+   rm -f sent ${OBJ} sent-${VERSION}.tar.gz
 
 dist: clean
@echo creating dist tarball
-   @mkdir -p sent-${VERSION}
-   @cp -R LICENSE Makefile config.mk config.def.h ${SRC} sent-${VERSION}
-   @tar -cf sent-${VERSION}.tar sent-${VERSION}
-   @gzip sent-${VERSION}.tar
-   @rm -rf sent-${VERSION}
+   mkdir -p sent-${VERSION}
+   cp -R LICENSE Makefile config.mk config.def.h ${SRC} sent-${VERSION}
+   tar -cf sent-${VERSION}.tar sent-${VERSION}
+   gzip sent-${VERSION}.tar
+   rm -rf sent-${VERSION}
 
 install: all
@echo installing executable file to ${DESTDIR}${PREFIX}/bin
-   @mkdir -p ${DESTDIR}${PREFIX}/bin
-   @cp -f sent ${DESTDIR}${PREFIX}/bin
-   @chmod 755 ${DESTDIR}${PREFIX}/bin/sent
+   mkdir -p ${DESTDIR}${PREFIX}/bin
+   cp -f sent ${DESTDIR}${PREFIX}/bin
+   chmod 755 ${DESTDIR}${PREFIX}/bin/sent
@echo installing manual page to ${DESTDIR}${MANPREFIX}/man1
-   @mkdir -p ${DESTDIR}${MANPREFIX}/man1
-   @cp sent.1 ${DESTDIR}${MANPREFIX}/man1/sent.1
-   @chmod 644 ${DESTDIR}${MANPREFIX}/man1/sent.1
+   mkdir -p ${DESTDIR}${MANPREFIX}/man1
+   cp sent.1 ${DESTDIR}${MANPREFIX}/man1/sent.1
+   chmod 644 ${DESTDIR}${MANPREFIX}/man1/sent.1
 
 uninstall:
@echo removing executable file from ${DESTDIR}${PREFIX}/bin
-   @rm -f ${DESTDIR}${PREFIX}/bin/sent
+   rm -f ${DESTDIR}${PREFIX}/bin/sent
 
 .PHONY: all options clean dist install uninstall cscope
diff --git a/config.mk b/config.mk
index d61c55437f46..15fdcfa10dc7 100644
--- a/config.mk
+++ b/config.mk
@@ -11,20 +11,18 @@ X11INC = /usr/X11R6/include
 X11LIB = /usr/X11R6/lib
 
 # includes and libs
-INCS = -I. -I/usr/include -I/usr/include/freetype2 -I${X11INC}
-LIBS = -L/usr/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
+INCS = -I/usr/include/freetype2 -I${X11INC}
+LIBS = -lm -L${X11LIB} -lXft -lfontconfig -lX11
 # OpenBSD (uncomment)
-#INCS = -I. -I${X11INC} -I${X11INC}/freetype2
+#INCS = -I${X11INC} -I${X11INC}/freetype2
 # FreeBSD (uncomment)
-#INCS = -I. -I/usr/local/include -I/usr/local/include/freetype2 -I${X11INC}
-#LIBS = -L/usr/local/lib -lc -lm -L${X11LIB} -lXft -lfontconfig -lX11
+#INCS = -I/usr/local/include/freetype2 -I${X11INC}
+#LIBS = -lm -L${X11LIB} -lXft -lfontconfig -lX11
 
 # flags
 CPPFLAGS = -DVERSION=\"${VERSION}\" -D_XOPEN_SOURCE=600
-CFLAGS += -g -std=c99 -pedantic -Wall ${INCS} ${CPPFLAGS}
-LDFLAGS += -g ${LIBS}
-#CFLAGS += -std=c99 -pedantic -Wall -Os ${INCS} ${CPPFLAGS}
-#LDFLAGS += ${LIBS}
+CFLAGS = -std=c99 -pedantic -Wall -Wstrict-prototypes -Wold-style-definition 
-Os ${INCS} ${CPPFLAGS}
+LDFLAGS = ${LIBS}
 
 # compiler and linker
-CC ?= cc
+CC = cc
-- 
2.36.1