ci: add -Werror

[N-R-K]

this adds support for EXTRA_CFLAGS, which seems to be a common convention to append additional cflags. e.g neomutt uses it, linux kernel used to use it.

Fixes: https://github.com/resurrecting-open-source-projects/scrot/issues/283

[guijan]

In Automake, CFLAGS are user compiler flags, AM_CFLAGS are project compiler flags, and foo_CFLAGS are the foo target's CFLAGS. We'd have 2 levels of user CFLAGS that way, just use CFLAGS. Also, tcc supports the C11 _Noreturn, that's a libc bug.

[N-R-K]

just use CFLAGS

The defaults of configure.ac will need to be needlessly duplicated that way.

Also, tcc supports the C11 _Noreturn, that's a libc bug.

I remember another case where tcc emitted warning due to not understanding exit() - even though glibc marks it with __attribute__((__noreturn__)).

Funnily enough, it seems to understand __attribute__((__noreturn__)) when manually put just fine.

[/tmp]~> cat test.c
#ifdef PANIC
        extern void panic(int) __attribute__((__noreturn__));
#else
        #include <stdlib.h>
#endif

int f(_Bool x)
{
        if (x)
                return 5;
#ifdef PANIC
        panic(1);
#else
        exit(1);
#endif
}
[/tmp]~> tcc -Wall -std=c99 -c test.c -o /dev/null
test.c:16: warning: function might return no value: 'f'
[/tmp]~> tcc -Wall -std=c99 -c test.c -o /dev/null -DPANIC

But if I #include <stdlib.h> (even when using the panic function), then starts to warn again. Looks like something about stdlib.h is tripping tcc up.

[N-R-K]

https://github.com/bminor/glibc/blob/5f828ff824e3b7cd133ef905b8ae25ab8a8f3d66/misc/sys/cdefs.h#L279-L284

/* GCC and clang have various useful declarations that can be made with
   the '__attribute__' syntax.  All of the ways we use this do fine if
   they are omitted for compilers that don't understand it.  */
#if !(defined __GNUC__ || defined __clang__)
# define __attribute__(xyz)     /* Ignore */
#endif

Okay, as I suspected, they're defining __attribute__ to be a no-op. Makes sense why tcc is getting confused.

[guijan]

EXTRA_CFLAGS would give people another interface to fiddle with, better not.

We can try reporting the __attribute__((__noreturn__)) or _Noreturn issue to glibc and see if they fix it. TCC even #defines __GNUC__ to 4, that's why it works with OpenBSD's err.h.

[N-R-K]

We can try reporting the __attribute__((__noreturn__)) or _Noreturn issue to glibc and see if they fix it.

I plan on sending a patch sometime this week.

TCC even #defines GNUC to 4, that's why it works with OpenBSD's err.h.

Not on my system (and an nsxiv contributor running KISS linux had the warning as well IIRC). Perhaps it's due to some OpenBSD patch?

---

But anyways, back on topic:

EXTRA_CFLAGS would give people another interface to fiddle with, better not.

I went with it since it's more conventional, not because I wanted to give another env var to the user (though I don't mind giving it).

If you like, I can change it to SCROT_PRIVATE_CFLAGS_APPEND or something like that. I'd really like to avoid duplicating the defaults in multiple places.

[guijan]

The OpenBSD tcc doesn't have any patches. It does, however, compile off a git commit: https://repo.or.cz/tinycc.git/commit/29ae3ed4d5b83eec43598d6cd7949bccb41c8083. Maybe that was added between whatever version your OS has and the one from OpenBSD 7.3.

Maybe the configure.ac's logic can spell out "If no CFLAGS were specified, use our defaults, and append $SCROT_PRIVATE_CFLAGS"?

[N-R-K]

Done.

Also noticed that the header check is utterly broken. Here's the relavant part from config.log:

configure:5463: checking for sys/queue.h
configure:5463: gcc -c  -flto -O3 -Wall -Wextra -Wpedantic -Werror  conftest.c >&5
conftest.c:19: error: ISO C forbids an empty translation unit [-Werror=pedantic]
cc1: all warnings being treated as errors
configure:5463: $? = 1
configure: failed program was:
| /* confdefs.h */
| #define PACKAGE_NAME "scrot"
| #define PACKAGE_TARNAME "scrot"
| #define PACKAGE_VERSION "1.9"
| #define PACKAGE_STRING "scrot 1.9"
| #define PACKAGE_BUGREPORT "https://github.com/resurrecting-open-source-projects/scrot/issues"
| #define PACKAGE_URL "https://github.com/resurrecting-open-source-projects/scrot"
| #define PACKAGE "scrot"
| #define VERSION "1.9"
| #define HAVE_ERR 1
| #define HAVE_ERRX 1
| #define HAVE_WARN 1
| #define HAVE_WARNX 1
| /* end confdefs.h.  */
| 
| 
| #include <sys/queue.h>
configure:5463: result: no

I'll leave this up to you to fix.

[guijan]

BTW, I've run into standalone BSD headers on musl causing warnings due to libbsd using a GCC extension to include them if they exist (and not error otherwise) while working on stress-ng. Might be that.

[N-R-K]

I've taken the easy way out by adding -Wno-error=cpp -Wno-error=pedantic.

Unfortunate all pedantic errors had to be disabled, but it's still an improvement over the existing state where all warnings go unnoticed. So I'll merge this in 1~2 days.

If you come up with a valid fix during that time, then attach the .patch here, I'll apply it with git am on this branch.

[N-R-K]

Might be that.

Nah, it's just due to the test program being empty, which isn't valid under ISO C.

conftest.c:19: error: ISO C forbids an empty translation unit [-Werror=pedantic]