phoboslab / qoi

The “Quite OK Image Format” for fast, lossless image compression
MIT License
6.94k stars 330 forks source link

Add CFLAGS as per GNU Coding Standards #247

Closed lbatalha closed 1 year ago

lbatalha commented 1 year ago

According to the GNU Coding Standards the CFLAGS parameter should always be present.

The reason I ran into this is because I am updating the AUR package to use the Makefile to build instead of the current direct gcc calls, and I need to inject the -I /usr/include/stb/ option (because stb is installed as a dependency and dynamically linked). I would like to avoid going back to a solution involving sed to modify the sources.

I have tested this, and when CFLAGS is not set, the result seems to be just an extra empty whitespace in the ouput:

❯ make                                    
cc -std=gnu99 -O3  qoibench.c -o qoibench -lpng
cc -std=c99 -O3  qoiconv.c -o qoiconv

When it is set, it looks like:

❯ make CFLAGS='-I /usr/include/stb/' 
cc -std=gnu99 -O3 -I /usr/include/stb/ qoibench.c -o qoibench -lpng
cc -std=c99 -O3 -I /usr/include/stb/ qoiconv.c -o qoiconv

I assume this is due to the way things are set in the Makefile (ie: $CFLAGS vs $(CFLAGS) ?) but I am not sure as I do not have a lot of experience with makefiles intricacies.

amstan commented 1 year ago
make CFLAGS='-I /usr/include/stb/' 

Instead of doing that, why don't you just set CFLAGS_CONV and CFLAGS_BENCH?

alex@alex-framework:~/Projects/qoi (master)% pacman -Ql|grep stb_image
stb /usr/include/stb/stb_image.h
stb /usr/include/stb/stb_image_resize.h
stb /usr/include/stb/stb_image_write.h
alex@alex-framework:~/Projects/qoi (master)% make CFLAGS_CONV="-I /usr/include/stb -std=c99 -O3" conv
cc -I /usr/include/stb -std=c99 -O3 qoiconv.c -o qoiconv
2.21s real  2.16s user  0.04s system  99% cpu  118kB mem $ make CFLAGS_CONV="-I /usr/include/stb -std=c99 -O3" conv

If you don't like the name of those, perhaps you could change that instead of making a new one (perhaps just merge lines 2 and 4 given they're both declared to the save value).

lbatalha commented 1 year ago

Instead of doing that, why don't you just set CFLAGS_CONV and CFLAGS_BENCH?

The reason is because if I set those variables, it overrides them fully (ignoring whatever is in the Makefile), so for example if the Makefile ever gets updated in the future with anything other than -std=gnu99 -O3 for bench, my PKGBUILD will keep using those which could cause silent problems even if it builds correctly due to new or modified flags.

Additionally you cannot do something like CFLAGS_BENCH="$CFLAGS_BENCH -I /usr/include/stb/" because make will complain about recursive variables

❯ make CFLAGS_BENCH='-I /usr/include/stb/ $(CFLAGS_BENCH)' 
[...]
make: *** Recursive variable 'CFLAGS_BENCH' references itself (eventually).  Stop.

If you don't like the name of those, perhaps you could change that instead of making a new one (perhaps just merge lines 2 and 4 given they're both declared to the save value).

The names seem okay to me, and having them separate seems reasonable for more flexibility in the future. Merging them wouldn't really change the main reason for this PR, since the point is that CFLAGS is a standardized variable where the user can add overrides. Since qoi seems to default to static linking of stb, I also did not want to change that behavior, because it would probably be very specific to particular platforms or distributions. (In earlier versions of the AUR package, I would sed -i 's/"stb_image.h"/<stb_image.h>/g' qoiconv.c, which is an inelegant solution compared to -I imo.

phoboslab commented 1 year ago

I'm quite clueless when it comes to makefiles. The reasoning here makes sense and it doesn't seem to have any downsides for current uses. Maybe there's some other, cleaner way to do this (PRs welcome)!? I'll merge this for now. Thank you!