zhaojh329 / libuhttpd

A very flexible, lightweight and high performance HTTP server library based on libev and http-parser for Embedded Linux.
MIT License
386 stars 67 forks source link

src/CMakeLists.txt: add UHTTPD_ENABLE_WERROR option #12

Closed ffontaine closed 4 years ago

ffontaine commented 4 years ago

Add UHTTPD_ENABLE_WERROR option to be able to disable -Werror

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

zhaojh329 commented 4 years ago

Why?

ffontaine commented 4 years ago

It's a request from Yann (see https://patchwork.ozlabs.org/project/buildroot/patch/20201025164359.1837392-1-fontaine.fabrice@gmail.com/).

ffontaine commented 4 years ago

You can find more information about why we should offer the option to disable -Werror here: https://embeddedartistry.com/blog/2017/05/22/werror-is-not-your-friend/

zhaojh329 commented 4 years ago

I think with this flag, GCC can help us better detect code problems, make our code stronger.

ffontaine commented 4 years ago

Yes, you're right, that's why the PR kept it ON by default. However for a packager/buildroot perspective, we don't want to have build failure because of these warnings.

zhaojh329 commented 4 years ago

https://github.com/zhaojh329/libuhttpd/commit/a388b50e08aa7aa780f3a3e30cfe4002701e7bfe

yann-morin-1998 commented 4 years ago

I think with this flag, GCC can help us better detect code problems, make our code stronger.

This is absolutely true, indeed. This is true from your perspective, as the developper-maintainer of the code.

But from a distribution's or a buildsystem's (collectively, a downstream) point of view, -Werror is not helpful at all; instead, it is a burden.

Consider for example, that as of today, with the latest gcc, gcc-10, you have no warning at all; this is great. But tomorrow. gcc-11 gets out, and adds a new warning. Surely, your code is no worse than it was the day before. So, if the code was OK with gcc-10, why should it be now considered bad because of gcc-11? Note also that the opposite may happen: an older compiler may emit warnings where your newer compiler does not.

As a second example, consider for example that your compiler does not default to stack-smashing protection (SSP), so you have no idea about any warning in that area; now another compiler defaults to enabling SSP, and emits some warning. But surely, the code is no worse than it was when you tested it.

Yes, as the developper, you will want to detect those and fix them; but for a downstream's point of view, these new warnings-turned-errors are mostly meaningless now.

There is no way that, as the upstream developper, you can know all the architectures, all the compiler versions, all the compiler flags, and all combinations thereof, that your package will be built against.

As such, unconditionally enabling -Werror is counterproductive for downstreams. If there is a way to disable -Werror, downstreams will use it; otherwise, they will just patch it out.

Thanks. :-)

zhaojh329 commented 4 years ago

I think with this flag, GCC can help us better detect code problems, make our code stronger.

This is absolutely true, indeed. This is true from your perspective, as the developper-maintainer of the code.

But from a distribution's or a buildsystem's (collectively, a downstream) point of view, -Werror is not helpful at all; instead, it is a burden.

Consider for example, that as of today, with the latest gcc, gcc-10, you have no warning at all; this is great. But tomorrow. gcc-11 gets out, and adds a new warning. Surely, your code is no worse than it was the day before. So, if the code was OK with gcc-10, why should it be now considered bad because of gcc-11? Note also that the opposite may happen: an older compiler may emit warnings where your newer compiler does not.

As a second example, consider for example that your compiler does not default to stack-smashing protection (SSP), so you have no idea about any warning in that area; now another compiler defaults to enabling SSP, and emits some warning. But surely, the code is no worse than it was when you tested it.

Yes, as the developper, you will want to detect those and fix them; but for a downstream's point of view, these new warnings-turned-errors are mostly meaningless now.

There is no way that, as the upstream developper, you can know all the architectures, all the compiler versions, all the compiler flags, and all combinations thereof, that your package will be built against.

As such, unconditionally enabling -Werror is counterproductive for downstreams. If there is a way to disable -Werror, downstreams will use it; otherwise, they will just patch it out.

Thanks. :-)

First of all, thank you very much. I find it awkward to deal with this. I don't seem to see any other open source programs doing anything similar.

yann-morin-1998 commented 4 years ago

I don't seem to see any other open source programs doing anything similar.

For example, binutils have a --disable-werror configure flag:

[...]
3530 AC_ARG_ENABLE(werror,
3531 [AS_HELP_STRING([--enable-werror],
3532                 [enable -Werror in bootstrap stage2 and later])],
[...]

glibc also has a similar flag:

[...]
 294 AC_ARG_ENABLE([werror],
 295               AC_HELP_STRING([--disable-werror],
 296                              [do not build with -Werror]),
 297               [enable_werror=$enableval],
 298               [enable_werror=yes])
[...]

gdb also has one.

Incidentally, those three are core components of a toolchain, and they do have flags to disable use of -Werror.

And many others do, in one form or another: grub2, llvm, libhid, libcamera, ltrace, lxc, openocd, wireshark, zeromq, etc...

Also, you may look in the Buildroot tree, and see that we do disable -Werror when possible, otherwise we patch it out: git grep -i werror

Thanks. :-)

zhaojh329 commented 4 years ago

If it's turned off in another environment, there's no way to check whether my code is robust in another environment.

yann-morin-1998 commented 4 years ago

If it's turned off in another environment, there's no way to check whether my code is robust in another environment.

Being robust is not about being warning-free (even if that helps). Being robust is about running as expected; this is validated with a test-suite.

zhaojh329 commented 4 years ago

If it's turned off in another environment, there's no way to check whether my code is robust in another environment.

Being robust is not about being warning-free (even if that helps). Being robust is about running as expected; this is validated with a test-suite.

I known what you mean. However, by the "Werror", I can at least make fewer mistakes or avoid some bugs that should not appear.

ffontaine commented 4 years ago

It should be noted that libuhttpd raises autobuilder failures for more than 10 days on riscv64 (http://autobuild.buildroot.org/results/7a0/7a0dd4a7ba8f1eeaf1e15e4b4c73537a86a2d8d5/build-end.log):

/nvme/rc-buildroot-test/scripts/instance-0/output-1/build/libuhttpd-3.4.0/src/file.c:44:24: error: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'long long unsigned int' [-Werror=format=]
   44 |     snprintf(buf, len, "\"%" PRIx64 "-%" PRIx64 "-%" PRIx64 "\"",
      |                        ^~~~~
   45 |              s->st_ino, s->st_size, (uint64_t)s->st_mtime);
      |              ~~~~~~~~~  
      |               |
      |               long long unsigned int

This time I'll let you fix it and send the appropriate patch to buildroot mailing list as it seems that you really don't want to disable -Werror.

zhaojh329 commented 4 years ago

It should be noted that libuhttpd raises autobuilder failures for more than 10 days on riscv64 (http://autobuild.buildroot.org/results/7a0/7a0dd4a7ba8f1eeaf1e15e4b4c73537a86a2d8d5/build-end.log):

/nvme/rc-buildroot-test/scripts/instance-0/output-1/build/libuhttpd-3.4.0/src/file.c:44:24: error: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'long long unsigned int' [-Werror=format=]
   44 |     snprintf(buf, len, "\"%" PRIx64 "-%" PRIx64 "-%" PRIx64 "\"",
      |                        ^~~~~
   45 |              s->st_ino, s->st_size, (uint64_t)s->st_mtime);
      |              ~~~~~~~~~  
      |               |
      |               long long unsigned int

This time I'll let you fix it and send the appropriate patch to buildroot mailing list as it seems that you really don't want to disable -Werror.

Thanks. I will.