addition of -Wall to default flags (redhat-rpm-config-8.0.38-1)

[Roland McGrath]

If there was a mention of this change, I overlooked it.

The addition of -Wall to the $RPM_OPT_FLAGS value has some unintended
consequences.  This broke my latest elfutils build until I worked around it.

The CFLAGS given to configure, i.e. what's in %optflags, is usually
appended to the package makefiles' own list of flags.  As gcc processes its
command line, the later flags override the earlier ones.  This is the right
thing e.g. when a package makefile uses -O and we override it to -O2 and so on.

When a package uses -W* flags already itself, it may use some -Wno-* flags
in its standard setting when some of the warnings are over-eager for some
of its code.  For example, elfutils uses -Wall -Werror (and some more
warning options) on every file, but appends -Wno-format for a few files
where gcc's format checking is not smart enough to know what is really OK.

With the new redhat-rpm-config setting of %optflags including -Wall, we
wind up with -Wall -Werror -Wno-format -Wall -Wp,-D_FORTIFY_SOURCE=2 ...
and the second -Wall overrides the -Wno-format that overrode part of the
effect of the first -Wall.  Since it uses -Werror, the additional spurious
warnings break the build.

There is an argument to be made that the upstream package is broken if you
cannot use "configure CFLAGS=-Wall".  But, folks should be aware of this
change and how it might explain sudden differences in the builds of
packages that worked before Monday.

For today's elfutils build, I am using this:

	RPM_OPT_FLAGS=${RPM_OPT_FLAGS/-Wall/}
	%configure CFLAGS="$RPM_OPT_FLAGS"


Thanks,
Roland

[Nicholas Miell]

On Sat, 2005-08-06 at 15:36 -0700, Roland McGrath wrote:

>  There is an argument to be made that the upstream package is broken
> if you
>  cannot use "configure CFLAGS=-Wall".  But, folks should be aware of this
>  change and how it might explain sudden differences in the builds of
>  packages that worked before Monday.
>   
I'd argue that any upstream package which includes -Werror by default is
broken, considering how often gcc warnings change. 

-- 
Nicholas Miell <nmiell(a)comcast.net&gt;

[Roland McGrath]

>  I'd argue that any upstream package which includes -Werror by
> default is
>  broken, considering how often gcc warnings change.  
I see that argument.  Some packages are like elfutils and intended only to
build on the latest gcc version, and update frequently to track it.
Whatever their rationale and the wisdom of it or lack thereof, that's the
way they are upstream.  We should come to a decision about the preferred
way to deal with these in Fedora packaging, be it to remove their -Werror
or whatever else.

The addition of -Wall created a problem that we didn't have before,
so I brought it up.


Thanks,
Roland

[Ulrich Drepper]

Nicholas Miell wrote:
>  I'd argue that any upstream package which includes -Werror by
> default is
>  broken, considering how often gcc warnings change.  
And I argue that we apparently must come to a state where -Werror is
enabled automatically.  The current state, aggravated by adding -Wall,
is that warnings are ignored.  The result: bugs the compiler finds are
no fixed.  I even found one case where the _FORTIFY_SOURCE magic found a
buffer overflow and the maintainer hasn't seen it.

It is crucial that packages are changed to have zero warnings.
Otherwise these bugs remain unnoticed since people think warnings are OK
and don't care.   The "apparent" part is that using -Werror is the only
way to do this.  Without enforcement people _think_ there are more
important things to do than fixing warnings.

Yes, it might mean that an update to a new gcc version means required
changes.  But guess what?  Whenever a warning pops up there is likely a
good reason for it and it is worthwhile spending time on it.  Just like
all these signed vs unsigned warnings in gcc 4.  They almost all the
time warrant looking at the code.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

[Nicholas Miell]

On Sat, 2005-08-06 at 16:55 -0700, Ulrich Drepper wrote:
>  Nicholas Miell wrote:
>  > I'd argue that any upstream package which includes -Werror by default is
>  > broken, considering how often gcc warnings change. 
>  
>  And I argue that we apparently must come to a state where -Werror is
>  enabled automatically.  The current state, aggravated by adding -Wall,
>  is that warnings are ignored.  The result: bugs the compiler finds are
>  no fixed.  I even found one case where the _FORTIFY_SOURCE magic found a
>  buffer overflow and the maintainer hasn't seen it.
>  
>  It is crucial that packages are changed to have zero warnings.
>  Otherwise these bugs remain unnoticed since people think warnings are OK
>  and don't care.   The "apparent" part is that using -Werror is the only
>  way to do this.  Without enforcement people _think_ there are more
>  important things to do than fixing warnings.
>  
>  Yes, it might mean that an update to a new gcc version means required
>  changes.  But guess what?  Whenever a warning pops up there is likely a
>  good reason for it and it is worthwhile spending time on it.  Just like
>  all these signed vs unsigned warnings in gcc 4.  They almost all the
>  time warrant looking at the code. 
Nothing stops you from doing internal builds with -Werror and then
fixing all the warnings before you make a release.

However, when you ship software that won't compile, your end users are
either going to remove -Werror and rebuild or they're going to switch to
something that works out-of-the-metaphorical-box. You've gained nothing.

-- 
Nicholas Miell <nmiell(a)comcast.net&gt;

[Ulrich Drepper]

Nicholas Miell wrote:
>  Nothing stops you from doing internal builds with -Werror and then
>  fixing all the warnings before you make a release. 
You don't get it.  The people who need to use -Werror are all those
people who rebuild code.  This includes all the package maintainers but
also all everybody who makes changes.  Those are all the people doing
rebuilds since otherwise they'd simply use the binary packages which are
provided.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

[Nicholas Miell]

On Sat, 2005-08-06 at 19:26 -0700, Ulrich Drepper wrote:
>  Nicholas Miell wrote:
>  > Nothing stops you from doing internal builds with -Werror and then
>  > fixing all the warnings before you make a release.
>  
>  You don't get it.  The people who need to use -Werror are all those
>  people who rebuild code.  This includes all the package maintainers but
>  also all everybody who makes changes.  Those are all the people doing
>  rebuilds since otherwise they'd simply use the binary packages which are
>  provided.
>   
In my experience, binary packages are the exception rather than the rule
(especially in impoverished RPM land). Open source projects may include
a somewhat recent binary tarball for one architecture, but those
projects seem to be in the minority.

Most of the people who end up unpacking a tarball and building it are
end users who aren't interested in making any changes (beyond removing
-Werror when necessary).

A more sensible approach would be to include -Werror in CVS/SVN/etc. any
development snapshots, but remove it for official release tarballs
(which, obviously, aren't targeted towards developers).

-- 
Nicholas Miell <nmiell(a)comcast.net&gt;

[Daniel Veillard]

On Sat, Aug 06, 2005 at 07:26:49PM -0700, Ulrich Drepper wrote:
>  Nicholas Miell wrote:
>  > Nothing stops you from doing internal builds with -Werror and then
>  > fixing all the warnings before you make a release.
>  
>  You don't get it.  The people who need to use -Werror are all those
>  people who rebuild code.  This includes all the package maintainers but
>  also all everybody who makes changes.  Those are all the people doing
>  rebuilds since otherwise they'd simply use the binary packages which are
>  provided. 
  Then what you really need to do is make -Wall the default for gcc !

There is only so much the packagers can do over the packages, this is OSS
the key to success is to tap on the masses of people looking at the 
problem, not on a limited few who usually are not in charge.
Get the message back upstream, enable -Wall as the default gcc option
is the only sensible methodology, why on earth are you hiding those
informations if you think they are so crucially important that they
should lead to build errors here.

   -Wall -Werror on Red Hat built will generate a tension between
Red Hat packagers and upstream, on the other hand -Wall activated by
default will bring the problem back directly to the source in most
cases (I assume an overhelming majority of the Fedora packages are
built with gcc upstream).

  What you are aiming at sounds right, but my point is that your solution
is not the most effective, and is likely to just miss its real target.

Daniel

-- 
Daniel Veillard      | Red Hat Desktop team http://redhat.com/
veillard(a)redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

[Enrico Scholz]

drepper(a)redhat.com (Ulrich Drepper) writes:

> > I'd argue that any upstream package which includes -Werror by
> default
> > is broken, considering how often gcc warnings change.
>
>  And I argue that we apparently must come to a state where -Werror is
>  enabled automatically. 
I would really like to use it, but existing APIs (SUSv3) do not support
warning free development. E.g. 'dlsym()' returns an object pointer, but
often a function pointer is wanted. I do not know a way to write

| #include <dlfcn.h>
| 
| int main()
| {
|   void (*foo)() = dlsym(0, "foo");
|   void (*bar)() = (void (*)())dlsym(0, "bar");
| 
|   foo();
|   bar();
| }

warning-free:

| $ gcc -ldl -Wall -pedantic -W -Werror -std=c99 dlsym.c
| cc1: warnings being treated as errors
| dlsym.c: In function 'main':
| dlsym.c:5: warning: ISO C forbids initialization between function pointer and 'void
*'
| dlsym.c:6: warning: ISO C forbids conversion of object pointer to function pointer type


Or, I maintain a library which provides some deprecated functions and
gives out warnings at link-time when they are used. The same project has
some legacy tools (used by some people) which are triggering exactly
these warnings. Enforcing '-Werror' would make it either impossible to
mark obsolete functions (bad for developers), or to build legacy tools
(bad for some users).



Enrico

[Ulrich Drepper]

Enrico Scholz wrote:

>  | $ gcc -ldl -Wall -pedantic -W -Werror -std=c99 dlsym.c 
Nobody suggested using -pedantic, certainly I didn't.  That's a
completely useless option and I really would like for it to be removed.
 Just like -ansi.


>  Or, I maintain a library which provides some deprecated functions
> and
>  gives out warnings at link-time when they are used. The same project has
>  some legacy tools (used by some people) which are triggering exactly
>  these warnings. Enforcing '-Werror' would make it either impossible to
>  mark obsolete functions (bad for developers), or to build legacy tools
>  (bad for some users). 
First of all, warnings by the linker are not covered by -Werror.
Second, there are -Wno-* parameters which can and should be added after
the -Wall to avoid some warnings _for specific files_.  It's just a
matter of setting the Makefiles up to make this easily possible.  The
elfutils makefiles do just that.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

[Owen Taylor]

On Sat, 2005-08-06 at 16:55 -0700, Ulrich Drepper wrote:
>  Nicholas Miell wrote:
>  > I'd argue that any upstream package which includes -Werror by default is
>  > broken, considering how often gcc warnings change. 
>  
>  And I argue that we apparently must come to a state where -Werror is
>  enabled automatically.  The current state, aggravated by adding -Wall,
>  is that warnings are ignored.  The result: bugs the compiler finds are
>  no fixed.  I even found one case where the _FORTIFY_SOURCE magic found a
>  buffer overflow and the maintainer hasn't seen it.
>  
>  It is crucial that packages are changed to have zero warnings.
>  Otherwise these bugs remain unnoticed since people think warnings are OK
>  and don't care.   The "apparent" part is that using -Werror is the only
>  way to do this.  Without enforcement people _think_ there are more
>  important things to do than fixing warnings. 
-Werror here seems like an unnecessarily blunt tool; there are 
plenty of warnings (unused variable warnings for example) that *do not*
typically indicate real breakage, and having to patch around them
in spec files would be wasting the package maintainer's time.

Remember, on some packages we have only very small influence on the
upstream maintainers and its not always easy to convince a maintainer
to work around a false-positive warning, so we could have very
large patch sets for some packages to suppress warnings.

On the other hand, there are some warnings that almost always indicate
serious problems (the warning you get when passing an int * to something
that takes a long * on a 64-bit machine, for example.)

We should easily be able to scan build logs for warnings with either
a white-list or a black-list and fail builds based on that.

Regards,
						Owen

[Jakub Jelinek]

On Sat, Aug 06, 2005 at 03:36:01PM -0700, Roland McGrath wrote:
>  If there was a mention of this change, I overlooked it.
>  
>  The addition of -Wall to the $RPM_OPT_FLAGS value has some unintended
>  consequences.  This broke my latest elfutils build until I worked around it. 
I'd say we should only add -Wall if -Werror is not used, because if -Werror
is used, usually the package Makefiles play games with enabling and
disabling some warnings.  For this we could use specs file as suggested by
Roland earlier:
%rename cc1 cc1rhorig
*cc1: %{!Wall:%{!Werror:-Wall}} %(cc1rhorig)

and use -specs=/usr/lib/rpm/redhat/specs in %{optflags}.
Some warnings like -Wpointer-sign are quite questionable and hit insane
amount of code...

	Jakub