Changing policy about -Werror

classic Classic list List threaded Threaded
119 messages Options
123456
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Matt Turner-5
On Mon, Sep 10, 2018 at 1:34 PM Chí-Thanh Christopher Nguyễn
<[hidden email]> wrote:

>
> Jason Zaman schrieb:
> >> No. With -Werror, upstream indicates that if a warning occurs, the build
> >> should fail and the resulting code not be installed on user systems.
> >>
> >> Instead, someone knowledgeable should look at the situation *first* and
> >> determine whether it is a bogus warning, a trivial issue, or something which
> >> warrants further attention.
> >>
> >> I have long disagreed with QA policy on this, and think that ebuilds should
> >> respect upstream here. Of course giving users the ability to override.
> >
> > I disagree. -Werror means that upstream wants it to build without
> > warnings on their distro with their version of the compiler with their
> > versions of all the libraries.
>
> It means, upstream wants it to build without warnings everywhere. And if a
> warning occurs (due to change in compiler, libraries, architecture, etc.),
> have a developer look at it first before installing the code on user systems.

This sounds good in theory, but I think it's pretty well established
that in practice this isn't effective and instead is a large waste of
time. In fact, the foundational premise that it's possible to build
without warnings everywhere is simply wrong.

Consider again the bug that started this. The maintainer had not built
this configuration. None of the arch teams had built this
configuration until I did for the last architecture Cc'd. The patch
committed doesn't change anything installed on the system, if not for
Werror preventing the code from building.

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Kristian Fiskerstrand-2
On 9/10/18 10:51 PM, Matt Turner wrote:
> Consider again the bug that started this. The maintainer had not built
> this configuration. None of the arch teams had built this
> configuration until I did for the last architecture Cc'd. The patch
> committed doesn't change anything installed on the system, if not for
> Werror preventing the code from building.

one way to look at it though, is that it is a valuable upstream
contribution that this configuration produces the error, so Gentoo is
contributing to upstream development because of it.

--
Kristian Fiskerstrand
OpenPGP keyblock reachable at hkp://pool.sks-keyservers.net
fpr:94CB AFDD 3034 5109 5618 35AA 0B7F 8B60 E3ED FAE3


signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Mart Raudsepp-2
Ühel kenal päeval, E, 10.09.2018 kell 22:56, kirjutas Kristian
Fiskerstrand:

> On 9/10/18 10:51 PM, Matt Turner wrote:
> > Consider again the bug that started this. The maintainer had not
> > built
> > this configuration. None of the arch teams had built this
> > configuration until I did for the last architecture Cc'd. The patch
> > committed doesn't change anything installed on the system, if not
> > for
> > Werror preventing the code from building.
>
> one way to look at it though, is that it is a valuable upstream
> contribution that this configuration produces the error, so Gentoo is
> contributing to upstream development because of it.
And losing users and thus relevance in the process. Not everyone goes
to bugzilla always and then waits for a fix, or fixes it themselves.
Normal people wipe that stuff away and install an operating
system/distribution that doesn't cause them grief in its place.

signature.asc (1000 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Kristian Fiskerstrand-2
In reply to this post by Kristian Fiskerstrand-2
On 9/10/18 10:56 PM, Kristian Fiskerstrand wrote:

> On 9/10/18 10:51 PM, Matt Turner wrote:
>> Consider again the bug that started this. The maintainer had not built
>> this configuration. None of the arch teams had built this
>> configuration until I did for the last architecture Cc'd. The patch
>> committed doesn't change anything installed on the system, if not for
>> Werror preventing the code from building.
>
> one way to look at it though, is that it is a valuable upstream
> contribution that this configuration produces the error, so Gentoo is
> contributing to upstream development because of it.
>
Adding to this, that arch testing for stabilization didn't catch this
might say more about our stabilization procedures than the quality of
the upstream package (that in this case routinely includes patches to
fix these issues). And they are mostly cought in testing (~arch)

--
Kristian Fiskerstrand
OpenPGP keyblock reachable at hkp://pool.sks-keyservers.net
fpr:94CB AFDD 3034 5109 5618 35AA 0B7F 8B60 E3ED FAE3


signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Mike Gilbert-2
In reply to this post by Kristian Fiskerstrand-2
On Mon, Sep 10, 2018 at 4:56 PM Kristian Fiskerstrand <[hidden email]> wrote:

>
> On 9/10/18 10:51 PM, Matt Turner wrote:
> > Consider again the bug that started this. The maintainer had not built
> > this configuration. None of the arch teams had built this
> > configuration until I did for the last architecture Cc'd. The patch
> > committed doesn't change anything installed on the system, if not for
> > Werror preventing the code from building.
>
> one way to look at it though, is that it is a valuable upstream
> contribution that this configuration produces the error, so Gentoo is
> contributing to upstream development because of it.

As an end user of Gentoo, I may not care about "contributing to
upstream"; I just want the software to compile and install.

As has been previously stated, people who care can add -Werror to
their own CFLAGS.

It's quite a bit harder for a user to remove -Werror from the build
system, assuming they can even interpret the error output.

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Kristian Fiskerstrand-2
On 9/10/18 11:01 PM, Mike Gilbert wrote:

> It's quite a bit harder for a user to remove -Werror from the build
> system, assuming they can even interpret the error output.
>

Sure, but at some point it matters whether this is a leaf package or
something that is a core dependency.

That it wasn't caught before being stabilized on several arches was
indeed bad, but that likely says more about our stabilization procedures
than the quality of the underlying package's upstream choices.

--
Kristian Fiskerstrand
OpenPGP keyblock reachable at hkp://pool.sks-keyservers.net
fpr:94CB AFDD 3034 5109 5618 35AA 0B7F 8B60 E3ED FAE3


signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Kristian Fiskerstrand-2
On 9/10/18 11:04 PM, Kristian Fiskerstrand wrote:

> On 9/10/18 11:01 PM, Mike Gilbert wrote:
>
>> It's quite a bit harder for a user to remove -Werror from the build
>> system, assuming they can even interpret the error output.
>>
>
> Sure, but at some point it matters whether this is a leaf package or
> something that is a core dependency.
>
> That it wasn't caught before being stabilized on several arches was
> indeed bad, but that likely says more about our stabilization procedures
> than the quality of the underlying package's upstream choices.
>
What I'm saying here is mostly that more tinderboxing is a good thing to
capture all kinds of issues, and for upstreams being responsive Gentoo
is a good way of providing valuable input.

--
Kristian Fiskerstrand
OpenPGP keyblock reachable at hkp://pool.sks-keyservers.net
fpr:94CB AFDD 3034 5109 5618 35AA 0B7F 8B60 E3ED FAE3


signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Chí-Thanh Christopher Nguyễn
In reply to this post by Fabian Groffen-2
Fabian Groffen schrieb:
> On 09-09-2018 11:22:41 -0400, Richard Yao wrote:
>> -Werror has caught bugs that could have resulted in data loss in ZFS in the past thanks to it being built in userspace as part of zdb. So it is useful for integrity too, not just security (although arguably, integrity is part of security).
>
> This is a misconception, as jer already pointed out.  Instead:
>
> -Werror has forced you to take notice of problems that could have
> resulted in data loss in ZFS ...

But in general it doesn't say when or how the problems became acute. Assuming
that the warning isn't bogus, it could have been the code relying on
undefined behavior, and the compiler changing the semantics of such behavior,
and introducing an accompanying warning at the same time.


Best regards,
Chí-Thanh Christopher Nguyễn

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Chí-Thanh Christopher Nguyễn
In reply to this post by Matt Turner-5
Matt Turner schrieb:
> This sounds good in theory, but I think it's pretty well established
> that in practice this isn't effective and instead is a large waste of
> time.

I think even the thread starter stated that -Werror is unnecessary in the
vast majority of cases.

> In fact, the foundational premise that it's possible to build
> without warnings everywhere is simply wrong.
>
> Consider again the bug that started this. The maintainer had not built
> this configuration. None of the arch teams had built this
> configuration until I did for the last architecture Cc'd. The patch
> committed doesn't change anything installed on the system, if not for
> Werror preventing the code from building.

It is indeed an insurmountable task to write code that is warning-free from
the beginning across architectures, compiler versions, etc. But that is not
the goal anyway. It is examining the situation and taking appropriate action,
and then applying a change to no longer cause that particular warning (or
make it non-fatal if the warning is bogus/harmless).


Best regards,
Chí-Thanh Christopher Nguyễn

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Kristian Fiskerstrand-2
On 9/10/18 11:19 PM, Chí-Thanh Christopher Nguyễn wrote:
> It is indeed an insurmountable task to write code that is warning-free
> from the beginning across architectures, compiler versions, etc. But
> that is not the goal anyway. It is examining the situation and taking
> appropriate action, and then applying a change to no longer cause that
> particular warning (or make it non-fatal if the warning is bogus/harmless).

sure, but for upstreams that make this an explicit goal, do we really
want to apply additional downstream pataches with the additional
complexity that carries for build system (autotools re-generation that
might make it unsupported upstream etc) ?

--
Kristian Fiskerstrand
OpenPGP keyblock reachable at hkp://pool.sks-keyservers.net
fpr:94CB AFDD 3034 5109 5618 35AA 0B7F 8B60 E3ED FAE3


signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Chí-Thanh Christopher Nguyễn
In reply to this post by Mart Raudsepp-2
Mart Raudsepp schrieb:
>> one way to look at it though, is that it is a valuable upstream
>> contribution that this configuration produces the error, so Gentoo is
>> contributing to upstream development because of it.
>
> And losing users and thus relevance in the process. Not everyone goes
> to bugzilla always and then waits for a fix, or fixes it themselves.
> Normal people wipe that stuff away and install an operating
> system/distribution that doesn't cause them grief in its place.

This is indeed a problem with -Werror (I mentioned it in a previous message).
Tinderboxing as k_f suggested could not possibly cover the vast number of
configurations that users have either.

An alternative to -Werror could be maybe some kind of "package health" status
that would alert users if warnings happened in supposedly warning-free code.
Portage already has a "Package triggers severe warnings" QA notice which
could be extended for this purpose.


Best regards,
Chí-Thanh Christopher Nguyễn

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Kristian Fiskerstrand-2
In reply to this post by Kristian Fiskerstrand-2
On 9/10/18 11:21 PM, Kristian Fiskerstrand wrote:

> On 9/10/18 11:19 PM, Chí-Thanh Christopher Nguyễn wrote:
>> It is indeed an insurmountable task to write code that is warning-free
>> from the beginning across architectures, compiler versions, etc. But
>> that is not the goal anyway. It is examining the situation and taking
>> appropriate action, and then applying a change to no longer cause that
>> particular warning (or make it non-fatal if the warning is bogus/harmless).
>
> sure, but for upstreams that make this an explicit goal, do we really
> want to apply additional downstream pataches with the additional
> complexity that carries for build system (autotools re-generation that
> might make it unsupported upstream etc) ?
>
in all fairness, for one of my upstream packages, SKS, we make -Werror
part of non-release versions but remove it for releases. But there are
certain crypto related packages where you want the ensure it is properly
handle altogether, in particular where RNG is concerned as there isn't
really a proper way to test for it afterwards.. for other packages the
test suite is of great importance.. if the tests are proper there isn't
a great need, but sadly packages today doesn't really come with proper
test suits

--
Kristian Fiskerstrand
OpenPGP keyblock reachable at hkp://pool.sks-keyservers.net
fpr:94CB AFDD 3034 5109 5618 35AA 0B7F 8B60 E3ED FAE3


signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Rich Freeman
In reply to this post by Mike Gilbert-2
On Mon, Sep 10, 2018 at 5:01 PM Mike Gilbert <[hidden email]> wrote:

>
> On Mon, Sep 10, 2018 at 4:56 PM Kristian Fiskerstrand <[hidden email]> wrote:
> >
> > On 9/10/18 10:51 PM, Matt Turner wrote:
> > > Consider again the bug that started this. The maintainer had not built
> > > this configuration. None of the arch teams had built this
> > > configuration until I did for the last architecture Cc'd. The patch
> > > committed doesn't change anything installed on the system, if not for
> > > Werror preventing the code from building.
> >
> > one way to look at it though, is that it is a valuable upstream
> > contribution that this configuration produces the error, so Gentoo is
> > contributing to upstream development because of it.
>
> As an end user of Gentoo, I may not care about "contributing to
> upstream"; I just want the software to compile and install.
>

For more critical packages (like the example of zfs) whether it
compiles and installs isn't 1/10th as important as whether it eats my
data...

--
Rich

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Kristian Fiskerstrand-2
On 9/10/18 11:31 PM, Rich Freeman wrote:
> For more critical packages (like the example of zfs) whether it
> compiles and installs isn't 1/10th as important as whether it eats my
> data...

exactly

--
Kristian Fiskerstrand
OpenPGP keyblock reachable at hkp://pool.sks-keyservers.net
fpr:94CB AFDD 3034 5109 5618 35AA 0B7F 8B60 E3ED FAE3


signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Chí-Thanh Christopher Nguyễn
In reply to this post by Kristian Fiskerstrand-2
Kristian Fiskerstrand schrieb:

> On 9/10/18 11:19 PM, Chí-Thanh Christopher Nguyễn wrote:
>> It is indeed an insurmountable task to write code that is warning-free
>> from the beginning across architectures, compiler versions, etc. But
>> that is not the goal anyway. It is examining the situation and taking
>> appropriate action, and then applying a change to no longer cause that
>> particular warning (or make it non-fatal if the warning is bogus/harmless).
>
> sure, but for upstreams that make this an explicit goal, do we really
> want to apply additional downstream pataches with the additional
> complexity that carries for build system (autotools re-generation that
> might make it unsupported upstream etc) ?

I fully understand why in the general case this is considered undesirable.

But in very specific cases it can make sense to err on the side of caution,
and the rigid -Werror policy gets in the way. This is what the initial
message by bircoph suggested.


Best regards,
Chí-Thanh Christopher Nguyễn

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Kristian Fiskerstrand-2
On 9/10/18 11:35 PM, Chí-Thanh Christopher Nguyễn wrote:
>
> I fully understand why in the general case this is considered undesirable.
>
> But in very specific cases it can make sense to err on the side of
> caution, and the rigid -Werror policy gets in the way. This is what the
> initial message by bircoph suggested.

I would argue that this depends on the upstream; some upstreams doesn't
know the downstream implications of this and are slow at responding at
issues. Others are very clear about it and this is the desired behavior;
and they actually fix it quickly when reported. There is a difference
here, and on some level that is up to maintainer privilege to evaluate
the difference.

--
Kristian Fiskerstrand
OpenPGP keyblock reachable at hkp://pool.sks-keyservers.net
fpr:94CB AFDD 3034 5109 5618 35AA 0B7F 8B60 E3ED FAE3


signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Richard Yao-2
In reply to this post by Fabian Groffen-2


> On Sep 10, 2018, at 10:19 AM, Fabian Groffen <[hidden email]> wrote:
>
>> On 09-09-2018 11:22:41 -0400, Richard Yao wrote:
>> -Werror has caught bugs that could have resulted in data loss in ZFS in the past thanks to it being built in userspace as part of zdb. So it is useful for integrity too, not just security (although arguably, integrity is part of security).
>
> This is a misconception, as jer already pointed out.  Instead:
>
> -Werror has forced you to take notice of problems that could have
> resulted in data loss in ZFS ...
It did. That is why it is used as a debug feature only when USE=debug is set. USE=-debug does not use -Werror. USE=debug on that package is meant for people who want to help upstream catch bugs.
>
> Also, consider that for -Werror to be "better", you also need -O3 in
> order to activate the "proper" compiler checks like "variable set but
> never used" ones.
I have had “set but never used” errors on -O2.
>
>> Perhaps we could have another USE flag for -Werror where it is a security feature. e.g. USE=strict-compile-checks
>
> You better run a static code analyser, such as the one you can hook up
> with Travis.  It usually points out real security problems such as
> races, which GCC doesn't do yet, as far as I'm aware.  Let alone
> trigger with -Werror.
We are using Coverity, but there is no one tool that catches all issues such that the compiler’s checks are redundant.
>
> Fabian
>
>
> --
> Fabian Groffen
> Gentoo on a different level


Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Richard Yao-2
In reply to this post by Mart Raudsepp-2


> On Sep 10, 2018, at 4:59 PM, Mart Raudsepp <[hidden email]> wrote:
>
> Ühel kenal päeval, E, 10.09.2018 kell 22:56, kirjutas Kristian
> Fiskerstrand:
>>> On 9/10/18 10:51 PM, Matt Turner wrote:
>>> Consider again the bug that started this. The maintainer had not
>>> built
>>> this configuration. None of the arch teams had built this
>>> configuration until I did for the last architecture Cc'd. The patch
>>> committed doesn't change anything installed on the system, if not
>>> for
>>> Werror preventing the code from building.
>>
>> one way to look at it though, is that it is a valuable upstream
>> contribution that this configuration produces the error, so Gentoo is
>> contributing to upstream development because of it.
>
> And losing users and thus relevance in the process. Not everyone goes
> to bugzilla always and then waits for a fix, or fixes it themselves.
> Normal people wipe that stuff away and install an operating
> system/distribution that doesn't cause them grief in its place.

I would like to point out that if -Werror is set by say USE=debug, this is not an issue.

Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Richard Yao-2
In reply to this post by Chí-Thanh Christopher Nguyễn


> On Sep 10, 2018, at 5:18 PM, Chí-Thanh Christopher Nguyễn <[hidden email]> wrote:
>
> Fabian Groffen schrieb:
>>> On 09-09-2018 11:22:41 -0400, Richard Yao wrote:
>>> -Werror has caught bugs that could have resulted in data loss in ZFS in the past thanks to it being built in userspace as part of zdb. So it is useful for integrity too, not just security (although arguably, integrity is part of security).
>> This is a misconception, as jer already pointed out.  Instead:
>> -Werror has forced you to take notice of problems that could have
>> resulted in data loss in ZFS ...
>
> But in general it doesn't say when or how the problems became acute. Assuming that the warning isn't bogus, it could have been the code relying on undefined behavior, and the compiler changing the semantics of such behavior, and introducing an accompanying warning at the same time.
This is precisely why every warning is considered a bug and -Werror is set when USE=debug is in use.
>
>
> Best regards,
> Chí-Thanh Christopher Nguyễn
>


Reply | Threaded
Open this post in threaded view
|

Re: Changing policy about -Werror

Richard Yao-2
In reply to this post by Kristian Fiskerstrand-2


> On Sep 10, 2018, at 5:27 PM, Kristian Fiskerstrand <[hidden email]> wrote:
>
>> On 9/10/18 11:21 PM, Kristian Fiskerstrand wrote:
>>> On 9/10/18 11:19 PM, Chí-Thanh Christopher Nguyễn wrote:
>>> It is indeed an insurmountable task to write code that is warning-free
>>> from the beginning across architectures, compiler versions, etc. But
>>> that is not the goal anyway. It is examining the situation and taking
>>> appropriate action, and then applying a change to no longer cause that
>>> particular warning (or make it non-fatal if the warning is bogus/harmless).
>>
>> sure, but for upstreams that make this an explicit goal, do we really
>> want to apply additional downstream pataches with the additional
>> complexity that carries for build system (autotools re-generation that
>> might make it unsupported upstream etc) ?
>>
>
> in all fairness, for one of my upstream packages, SKS, we make -Werror
> part of non-release versions but remove it for releases.
This has been what sys-fs/zfs has been doing for years. The USE=-debug builds get -Werror while USE=-debug builds omit it. I think this is probably the solution here. USE=debug is meant to help catch bugs, even if some reports might be false positives. What it means varies on a per-package basis. I would call catching a security issue helping to catch bugs.

> But there are
> certain crypto related packages where you want the ensure it is properly
> handle altogether, in particular where RNG is concerned as there isn't
> really a proper way to test for it afterwards.. for other packages the
> test suite is of great importance.. if the tests are proper there isn't
> a great need, but sadly packages today doesn't really come with proper
> test suits
>
> --
> Kristian Fiskerstrand
> OpenPGP keyblock reachable at hkp://pool.sks-keyservers.net
> fpr:94CB AFDD 3034 5109 5618 35AA 0B7F 8B60 E3ED FAE3
>


123456