configure.ac: misc tweaks

[N-R-K]

More details in each commit message.

[guijan]

Btw, I vaguely recall I tried to do this once and @daltomi was against it, better give him time to comment.

[N-R-K]

Btw, I vaguely recall I tried to do this once and @daltomi was against it,

I'll try to dig up the PR and see what the rationale was.

better give him time to comment.

Yeah sure, I'll give this a week or two before merging.

[N-R-K]

I'll try to dig up the PR and see what the rationale was.

Only thing I found was this https://github.com/resurrecting-open-source-projects/scrot/pull/120 (which was merged).

(Also found this https://github.com/resurrecting-open-source-projects/scrot/pull/54 which was trying to remove -g by default - and for the same rationale as well).

[guijan]

Hmm, I wonder how many platforms are currently compiling scrot with -DNDEBUG? And if you add it to the default flags, are there platforms that will still compile scrot without -DNDEBUG? I don't want scrot to behave differently across platforms because of the way they handle compiler flags.

Also, lots of the assert()s are checking for null pointers, the only platform I know in which a null pointer dereference wouldn't cause a segfault is MMU-less Linux, we could delete those and save lines of code and the only difference would be a less informative crash.

[N-R-K]

Hmm, I wonder how many platforms are currently compiling scrot with -DNDEBUG? And if you add it to the default flags, are there platforms that will still compile scrot without -DNDEBUG? I don't want scrot to behave differently across platforms because of the way they handle compiler flags.

The goal here wasn't to have a uniform compiler flag across platforms - that's not really possible even if you wanted it. People are always free to compile with whatever option they choose, including -Ofast, -ftrapv and other behaviour changing flags.

If someone already have CFLAGS set, either directly by the user or some distro default, then that's their choice and we should respect that.

Main point here was to have a "release" build by default if no CFLAGS is set. Current default CFLAGS is half debug half release, which doesn't make much sense.

[N-R-K]

Also, lots of the assert()s are checking for null pointers, the only platform I know in which a null pointer dereference wouldn't cause a segfault is MMU-less Linux, we could delete those and save lines of code and the only difference would be a less informative crash.

I'm fine removing null pointer asserts when the pointer is being dereferenced immediately (or soon) after.

[guijan]

We don't have to give OSes a standard way to make scrot behave differently, we can write and use our own scrotAssert() that only shows up in the binary if -DSCROT_ASSERT is passed to the compiler (the opposite of -DNDEBUG) and keep it out of the default flags, that way systems are less likely to add our own nonstandard #define to their packages and the assertations won't show up in the binary because an OS failed to add -DNDEBUG, or we can remove assert() entirely. Also, the Hyperbola Linux build infrastructure doesn't seem to add -DNDEBUG to the compile flags. I can't seem to figure out which repository the relevant file is in, but in an installation, it's /etc/makepkg.conf.

There's nothing wrong with having both optimization and debug symbols, cmake has a RelWithDebInfo build type https://cmake.org/cmake/help/v3.26/manual/cmake-buildsystem.7.html#default-and-custom-configurations, Meson has debugoptimized https://mesonbuild.com/Builtin-options.html. Autotools doesn't have build types, but we can assume the default is the same sort of thing. I'm afraid there's an OS out there that doesn't touch compiler flags and wants debug symbols and will end up with a bug in the package because of this. We can touch optimization and warning flags as we want because the default build remains functionally the same, but keep -g in, an ELF binary gets mmap()ed and won't load unused parts of the file into memory anyway.

I actually like going for the highest optimization level in the default configuration because it implicitly communicates that our program is expected to work with it. Higher optimization levels expose undefined behavior, and some programs do break under LTO or -O3, so some operating systems are wary of LTO'ing or -O3'ing everything by default. I wouldn't go for something that breaks defined code like -Ofast though.

Also, I compiled 148b22374b59bae2aead52d056517eb99bc78145 with different optimization flags and compared the binary sizes:

$ wc -ch /tmp/scrot-* | grep -v 'total' | sort
  44.7K /tmp/scrot-Os-flto-DNDEBUG
  45.2K /tmp/scrot-Os-flto
  45.6K /tmp/scrot-O2-flto-DNDEBUG
  47.0K /tmp/scrot-O2-flto
  47.4K /tmp/scrot-O3-flto-DNDEBUG
  48.6K /tmp/scrot-O3-flto

[N-R-K]

I'm afraid there's an OS out there that doesn't touch compiler flags and wants debug symbols

A non-trivial amount of upstream do not enable debug symbols by default. If such an OS exists then it's broken by design, you cannot expect each and every upstream to have debug symbols enabled by default. So I'd rather have the default CFLAGS optimized for practical common use-cases.

[guijan]

I see, I haven't contributed to all that many programs.

What do we do about OSes not passing -DNDEBUG? I actually didn't know this was a problem until I looked into it now. We should really do the right thing by default instead of hoping OSes pass -DNDEBUG for us, this can't be fixed by changing default compilation options because the OSes will specify their own flags, and we can't unconditionally add the flag either because that won't give the OS or user full control over the compilation flags. The assert() code needs to be changed to our own scrotAssert() or removed.

[N-R-K]

What do we do about OSes not passing -DNDEBUG?

It's pretty common to have release builds by default and then use -DDEBUG to get debug builds. I,e opt-into debug builds rather than opt-out.

We can do that for scrot too - I have no objection to that (in fact, I think it's a better approach and what I typically do for my own programs).

[N-R-K]

Removed -DNDEBUG since that's not relavant anymore, and switched to -O3.

[N-R-K]

@guijan I still see some unnecessary standard header checks when running ./autogen.sh && ./configure:

checking for err... yes
checking for errx... yes
checking for warn... yes
checking for warnx... yes
checking for stdio.h... yes
checking for stdlib.h... yes
checking for string.h... yes
checking for inttypes.h... yes
checking for stdint.h... yes
checking for strings.h... yes
checking for sys/stat.h... yes
checking for sys/types.h... yes
checking for unistd.h... yes
checking for sys/queue.h... yes

Perhaps some autoconf default thing?

[guijan]

Yes, that's AC_CHECK_HEADERS default if its 4th parameter is missing or empty. You can pass a single space as its 4th parameter and it won't do those checks.

This line is to blame: https://github.com/resurrecting-open-source-projects/scrot/blob/4370fa0503b39b978656d2d0d2516ee9dbc19f36/configure.ac#L44 Should be:

AC_CHECK_HEADERS([sys/queue.h],, [LIBBSD_NEEDED=yes], [ ])

Can't seem to suggest changes for lines the PR doesn't touch.

[N-R-K]

Thanks, that worked.

less time waiting to compile while working on scrot will be great

I don't really notice any measurable difference between this PR and current master while running ./configure. (I've personally been side stepping autoconf entirely and have a simpler build which compiles in ~250ms. only issue is src/config.h which needs to be generated by ./configure)

[N-R-K]

only issue is src/config.h which needs to be generated by ./configure

I was looking at this to see if we can reduce dependency on autoconf or not. Ideally someone should just be able to run the following from the cli and scrot should just build without issues:

$ c99 -o scrot src/*.c $(pkg-config --cflags --libs ...)

The long list of library dependency is a bit unfortunate and makes this somewhat non-practical, but that can be fixed with a custom deps.pc file easily.

As for config.h, we only require it for the VERSION and PACKAGE macro. I think we can simply remove config.h and append those macros via -D. What do you think @guijan ?

---

Additionally, looking into this, I found that scrot doesn't build under c99 because we don't define proper feature-test-macro. See #306.

[guijan]

A program that uses Make or a shell script is eventually heavily limited by its choice like st or reimplements Autotools much worse like stress-ng. And then the program parametrizes platform differences (poorly) and leaves figuring out the right combination of #defines to package maintainers, or perhaps it has a few platforms the developers like with presets commented out in Makefile and expects maintainers to comment out the right one.

A meta build system like Autotools (even if Autotools is a bad one...) makes life easier. They're almost programming languages, so we can put in the logic to figure out all of that automatically and make things compile by default on as many platforms as possible, we don't have to write our own install/tarball/etc scripts, and so on.

[N-R-K]

Just to be clear, I'm not suggesting (or trying) to remove autoconf - rather, I'm trying to decouple the source code from it. This turned out to be not too difficult - only thing that scrot's source needed from autoconf was PACKAGE and VERSION macro.

See #308 for a more concrete example of what I'm trying to do.

They're almost programming languages

Yes, and complicated (meta)build systems are a mini program on their own - which comes with it's own set of bugs, maintainance burden, misfeatures etc. The "benefit" they provide doesn't come free of cost.