auto/{cc,options}: Removed unnecessary compiler flags

[BalkanMadman]

The build system should not unconditionally set compilation flag that vary a lot between each user and, as a result, are meant to be toggled individually by distribution/developer/end user.

Among these flags are the options that control program optimisation, debug symbols, the -pipe options and -Werror. -Werror might be harmful since compilers continuously add warnings so the users of new compilers may unexpectedly fail to compile njs since -Werror is set.

The compilation flags should be supplied in the standard CFLAGS environment variable.

Proposed changes

We hit this problem while packaging njs. The build system appends two -O, a -pipe and a -g to CFLAGS even if the user doesn't have these flags in their CFLAGS. I believe, the correct solution is to not unconditionally add any non-warning CFLAGS.

Checklist

Before creating a PR, run through this checklist and mark each as complete:

• I have read the CONTRIBUTING document
• If applicable, I have added tests that prove my fix is effective or that my feature works
• If applicable, I have checked that any relevant tests pass after adding my changes

[xeioex]

@BalkanMadman

NJS follows nginx here. It serves us well. I see no reason to change it without strong arguments.

We hit this problem while packaging njs. The build system appends two -O, a -pipe and a -g to CFLAGS even if the user doesn't have these flags in their CFLAGS.

Normally the last options trump the first ones if they conflict. Does it break anything for you. And if yes, could you, please, share any details? (compilers, OS, CFLAGS)

With CFLAGS, you can still overrule compiler options. See here.
Also, see in our CI pipeline here how options are passed.

[eli-schwartz]

• -pipe is harmless, everyone independently seems to add it anyway :P however, per the warning:

  This fails to work on some systems where the assembler is unable to read from a pipe; but the GNU assembler has no trouble.

  It is easiest to simply let builders set this when they want it.

• -g is a big pain, because people who want debug always add it to CFLAGS themselves, but people who do not want it now get big fat binaries and slower link time.

• -O is... fine, whatever, everyone overrides it with -O2 anyway so you just have weird / confusing compile commands.

What doesn't make sense is... all other build systems typically handle -g -O1 by defaulting to it ("sane defaults") if CFLAGS is unset. But if it is set, we assume the user has a preferred value to use.

Werror is a far worse, highly fatal problem. The only solution is not to do it. Only use targeted -Werror=xxx. When the compiler is updated, -Wextra (which is unstable) is prone to adding new default warnings, which become fatal and break stable release tags of your software. -Werror must only be used in CI -- that way new development versions of the code must fix warnings to have a clean build and pass CI, but stable releases are unaffected.

[thresheek]

I think -Werror by default is a reasonable choice for a project that is actively maintained. If you encounter an issue it's super easy to provide an appropriate -Wnoerror for your local builds and bug the maintainers to fix the underlying issue. No amount of CI is able to catch all the possible scenarios.

As for -g I don't believe that to be painful at all. Distros' build systems will either assume or force it by default anyway, and then the package managers will strip the binaries separating debuginfo into separate files. And the rest of folks who build from scratch are very likely to be savvy enough to call strip(1) on their binaries.

[BalkanMadman]

Would you accept a patch that sets the aforementioned flags only if CFLAGS are unset?

[xeioex]

@BalkanMadman

Would you accept a patch that sets the aforementioned flags only if CFLAGS are unset?

I am not sure it is a good idea to do this now. njs is already in many distros, and we may break their builds. If they, for some reasons, rely on the default flags.

[eli-schwartz]

I think -Werror by default is a reasonable choice for a project that is actively maintained. If you encounter an issue it's super easy to provide an appropriate -Wnoerror for your local builds and bug the maintainers to fix the underlying issue. No amount of CI is able to catch all the possible scenarios.

It's not exactly very "spirit of open source" for software to intentionally not build until you politely ask the upstream maintainers for permission and then wait for a new upstream release.

You seem to believe that people who encounter issues due to use of -Werror will "bug the maintainers to fix the underlying issue" but in reality when downstreams encounter scenarios like this, it simply results in increased burnout, and then the software doesn't work for a while until someone provides an appropriate patch file for the software to delete -Werror.

Keep in mind as well that many people who build software don't know much about C/C++, because they are users rather than developers. All they know is that the software doesn't compile on the latest version of Ubuntu even when following the copy-pasted directions for building. And for distros such as Gentoo, where typically all or most software is compiled locally with diverse options and on diverse hardware, even being packaged by the distro doesn't suffice to have prebuilt binaries -- produced by older compiler versions -- and the people who are trying and failing to build the software (and seeing the errors) are a very long distance indeed, away from being experienced distribution package maintainers who know all the tricks about wrangling troublesome software into building.

If you are unlucky, such people give up entirely and simply choose not to use "broken software". If you are lucky, such people submit bug reports but not even to you -- to the distro instead -- without details other than "it failed, I don't know why but here's a gigantic logfile of the failure" and then wait days or weeks for the distribution maintainer to have time in between the needs of IRL to investigate the issue, then eventually forward the bug report to you, then wait unknown periods of time for you to release a new tagged version (possibly a couple of months!) before they can even try again!

Or: the distribution package maintainer will (eventually, after users have already been broken once) fix it locally by removing the -Werror, and at that point there is "nothing" to report upstream since "it works now". (And because distribution package maintainers don't like getting into painful and frustrating arguments with upstreams that have made it clear they don't really think much about downstream users.)

When you say "No amount of CI is able to catch all the possible scenarios." -- you are really saying "we understand that -Werror is bad for our users, but we want our users to involuntarily participate in our CI". Otherwise why are you making the build fail for users over lint-style -Werrors that don't even indicate a coding defect or runtime bug, but simply indicate that GCC started suggesting some coding style isn't recommended anymore?

As for -g I don't believe that to be painful at all. Distros' build systems will either assume or force it by default anyway, and then the package managers will strip the binaries separating debuginfo into separate files. And the rest of folks who build from scratch are very likely to be savvy enough to call strip(1) on their binaries.

Absolutely not -- distro build systems have global toggles for whether debug info should be generated, which is very different from "assume or force it" because toggles have an off state.

Unfortunately, when software chooses to ignore the toggle, that means the compiler has to do a lot more work and in extreme scenarios you will run out of disk space long before the package manager can call strip(1) -- try building Qt Webengine sometime, with debug info. ;) Now granted that particular problem isn't a problem for njs, fair enough. But the general policy exists for good reason, and njs is breaking the policy and raises packaging warnings. Even if the package manager then forces -g itself, it will warn you if the software sets -g on its own precisely because "only the package manager itself is allowed to do that". What are you gonna do. 🤷

As for users building from scratch being "savvy enough", you're assuming they realize they need to! :) How should they notice that the binary has significantly large debug info that isn't just on account of the software itself being large? And... the readme of this project specifically says when installing from specific distro repositories, that there is an alternate package "with debug info", then warns you that "you should not use this for production environments, but it may be useful when developing". If I read a warning like this I would ABSOLUTELY not assume that ./configure && make is doing the very thing that the readme says is inadvisable!

[eli-schwartz]

I am not sure it is a good idea to do this now. njs is already in many distros, and we may break their builds. If they, for some reasons, rely on the default flags.

I'm not aware of a single distro that can even conceptually have their builds broken by changing this. If they need optimizations or debug, they will set those manually. If they don't want either one, then changing this will fix the distro, not break it.

The presence or lack of -pipe is uninteresting to me (I personally think you shouldn't be assuming the GNU assembler, but setting it only if you know the GNU assembler is active). The worst thing that can possibly happen from removing it is... Nothing. It makes no difference whatsoever, it doesn't change the generated code in the slightest, all it can do is slightly speed up the compiler. Removing it could make some builds slower if it isn't otherwise set, but that isn't a breakage, that is a slowage.

Removing -Werror cannot break any distro builds because Werror by definition can only cause otherwise successful builds to fail, so removing it can only ever result in more builds succeeding.