On 2018-03-08 05:18 +0000, Wookey wrote:
I've been packaging libopenCSD for debian. That has resulted in various changes, largely to the makefiles. Most of those changes should go upstream, rather than exist only in the debian version.
I'll post my issues/changes here for discussion so we can decide what is upstreamable.
FLAGS
The makefiles use 'CPP' and 'CPP_FLAGS' for C++, not C-preprocessor. This isn't wrong, but goes against convention. Normally 'CXX' and 'CXXFLAGS' is used for C++ settings. This makes for slightly confusing code if passing in distro settings for these variable from outside.
I suggest that we change the names to the conventional ones?
My main interest here is that distros want to set build flags, generally for security hardening reasons. And other users may well want to set flags for other reasons. The makefile should facilitate this, as it does for DEBUG, ARCH, OCSD_ROOT. It's good practice to use external *FLAGS if they are passed in.
I'm also not sure there is any real benefit to setting MASTER_*_FLAGS at the top level, then setting *_FLAGS from those in the sub-makefiles. In principle it gives a way of setting the default for everything built in a submakefile, but this is barely used (not at all for LIB_FLAGS), one without -fpic, one with $(WSUPRESS) and one removed -shared in LINKER_FLAGS (decoder/tests/build/linux/c_api_pkt_print_test/makefile )
Would it not make more sense to use *_FLAGS throughout?, either removing or specifying -shared/-fpic/$(WSUPRESS) in the build rules that are exceptions?
Using the conventionally-named variables and allowing them to be overridden by setting it when calling make would make it straightforward to change the options.
As an example, on debian the default settings are: $ dpkg-buildflags --get CFLAGS -g -O2 -fdebug-prefix-map=/home/wookey/packages/libopencsd/debian/libopencsd-0.8.0=. -fstack-protector-strong -Wformat -Werror=format-security
$ dpkg-buildflags --get CXXFLAGS -g -O2 -fdebug-prefix-map=/home/wookey/packages/libopencsd/debian/libopencsd-0.8.0=. -fstack-protector-strong -Wformat -Werror=format-security
$ dpkg-buildflags --get CPPFLAGS -Wdate-time -D_FORTIFY_SOURCE=2
You might feel that some of those are in fact sensible upstream-defaults too. I don't know about that, and it doesn't affect how things should work.
If all that sounds reasonable, and I've not missed something, I'll knock up a patch.
Wookey