Add --{en,dis}able-werror

[ffontaine]

Allow the user to disable -Werror (it remains enabled by default). This will allow buildroot to drop the following patch: https://git.buildroot.net/buildroot/tree/package/open-lldp/0002-Makefile.am-disable-Werror.patch

Signed-off-by: Fabrice Fontaine fontaine.fabrice@gmail.com

[apconole]

Are there still warnings being printed (such as the ones referenced in the buildroot patch)? I'd also like to address those as well, if possible. The ability to disable -Werror is still a useful enhancement, for over-zealous compilers.

[ffontaine]

Unfortunately, I can't retrieve the autobuilder configuration that raised this issue. This issue was raised by Yegor in 2018 when open-lldp was added to buildroot (BR). Here is an extract of https://patchwork.ozlabs.org/project/buildroot/patch/20180112140107.15301-1-laurent_pubs@yahoo.com/:

Perhaps you could tell Laurent which configuration you've used to produce this build failure, so that he can reproduce it.

I get this error even on my Debian 9 host. In BR I've been playing with powerpc defconf, that has boost issues in autobuilders.

[ffontaine]

I made a mistake, this is not the correct link, here is the good one: https://git.buildroot.net/buildroot/commit/package/open-lldp?id=c38853d708a70208859fa056d527b4cf40a9265a. It has a link to the autobuilder, I'll check if I can get you the warnings.

[kloczek]

IMO that patch is really bad. First it hard codes exact compiler options. Second it is possible to do the same without touching any file in source tree by just execute autoreconf -fiv; CFLAGS="-Wall -Wextra -Wformat=2" ./configure; make

Build framework should never ever hard code any compiler warning options, linker or compiler optimisation as well. With that it will be always possible to manipulate any optimisations or do additional code testing or scanning by passing exact settings in CFLAGS, LDFLAGS, CC, LD, AR, NM. RANLIB env variables.

[ffontaine]

Why this patch is bad?

Firstly, it doesn't hardcode more options, it only allows the user to disable -Werror.

Secondly, you can't achieve the same result by setting CFLAGS before the configure call. The CFLAGS will be appended, it will not override the CFLAGS provided by Makefile.am.

[kloczek]

Why this patch is bad?

KISS principle.

[ffontaine]

Then what do you suggest? Remove -Werror from Makefile.am? It's also fine with me.

[kloczek]

Yes and move that to bootstrap.sh and/or any CI procedure if it is used. As I wrote dist build procedure should have not hard coded such details like exact compiler options.

[apconole]

Many projects have a convenience function to set warnings=error during the configure stage (see: ovs, qemu, emacs, gcc, etc). I don't think such a feature is inappropriate.

OpenLLDP does not give such flexibility. Since e88f8d086f73 ("Add -Werror and -Wextra") it has been a fixed value, 'on' - and that means the user must deal with warnings immediately, even if a buggy compiler generates them. I am in favor of a feature to allow the user to enable/disable this flag, as other notable projects have such a feature. It is both user and developer friendly. There is already a workaround added because of the inflexibility (see parse_cli.o entry in Makefile.am which disables -Werror - presumably to work around some issue).

I would like to know what the warnings are that were unavoidable so that we can also fix those. I know that currently the code is compiling under the travis build with both clang and gcc without any build splats.

I am okay merging this.

[kloczek]

Many projects have a convenience function to set warnings=error during the configure stage (see: ovs, qemu, emacs, gcc, etc). I don't think such a feature is inappropriate.

And that part is completely redundant/pure waste of time not only those projects developers but additionally people like me who are working with thousands of packages on daily bases. All what I can tell you that in my distribution I'm patching to remove all that tweaks. Why? Try to imaging that working on whole OS distribution you want to test all packages to produce report about which one packages are sensitive on exact type of warnings. If implemented in packaging layer build process is 100% strips down generate all error/warnings messages it is possible to make such scanning, If not it makes such process very difficult.

Not to many people are working on whole OS distribution scale and sometimes even don't want to understand that what is sometimes handy/good for single package may not be layer above when single package is only small part odf something bigger.

If I can form any advice from that point of view it would be: please keep basic build process completely transparent adding build process customisation as the options injected over env variables in scripts like aytogen.sh/boostrap.sh.

Such separation gives as well better flexibility working on single project source tree because it is easier to add ad hoc generate any other source code processing and this can be done without touching original code stored in VCS like git. There plenty of tools like code scanners which can be used by passing such scanner in $CC. Issue issues that if in build process will be hard coded some $CC options this will automatically cause failing of those scanners with unrecognised parameter.

[apconole]

How do folks feel about a patch like follows:

diff --git a/.travis.yml b/.travis.yml
index 051d59b..97c10ef 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -3,7 +3,7 @@ dist: xenial
 #before_install: sudo apt-get update
 install: sudo apt-get install -y libconfig-dev libnl-3-dev rpm
 before_script: ./bootstrap.sh
-script: ./contrib/build-rpm.sh && ./configure && make && make test && sudo make install #&& rpmbuild -ba lldpad.spec
+script: ./contrib/build-rpm.sh && ./configure && make CFLAGS="-Werror -O2 -g" && make check && sudo make install

 matrix:
   include:
diff --git a/Makefile.am b/Makefile.am
index 16be654..cdb785e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -19,7 +19,7 @@ ACLOCAL_AMFLAGS = -I m4
 parse_cli.o: CFLAGS+=-U_FORTIFY_SOURCE -Wno-error

 ## system requires a shared libconfig
-AM_CFLAGS = -Wall -Werror -Wextra -Wformat=2 $(LIBCONFIG_CFLAGS) $(LIBNL_CFLAGS)
+AM_CFLAGS = -Wall -Wextra -Wformat=2 $(LIBCONFIG_CFLAGS) $(LIBNL_CFLAGS)
 AM_LDFLAGS = $(LIBCONFIG_LIBS) $(LIBNL_LIBS) -lrt

 ## header files to be installed, for programs using the client interface to lldpad 

[apconole]

I also want to remove the parse_cli.o exceptions. GCC-9 builds look just fine on my fedora system.

[kloczek]

AM_CFLAGS = -Wall -Wextra -Wformat=2 $(LIBCONFIG_CFLAGS) $(LIBNL_CFLAGS)

I would remove from here "-Wall -Wextra -Wformat=2" and move that string as well to boostrap.sh

[apconole]

@ffontaine will you respin your patch as above, or should I submit one?

[ffontaine]

If you can submit a new one, it would be perfect as I won't be able to update the PR soon.

[apconole]

I will tag you both in my forthcoming patch