Re: [gentoo-dev] Changing policy about -Werror

[Richard Yao]

On 09/14/2018 12:40 PM, Alon Bar-Lev wrote:
> On Fri, Sep 14, 2018 at 12:34 AM Sergei Trofimovich <slyfox@g.o> wrote:
>>
>> On Tue, 11 Sep 2018 12:44:38 +0300
>> Alon Bar-Lev <alonbl@g.o> wrote:
>>
>> I'm personally in favour of not allowing -Werror
>> to be in build system unconditionally.
>>
>> Maintainer is free to implement --enable-werror any way
>> it's convenient to run on their own extended sanity checks
>> and optionally expose it to users. Be it USE flag or
>> EXTRA_ECONF option.
>
> This discussion is not for downstream to have a more strict policy
> than upstream. People try to hijack discussion and introduce noise to
> de-focus the discussion.
>
> Downstream policy cannot be more strict than upstream as then every
> change upstream is doing downstream need to rebase and invest in even
> more changes.
>
> This discussion is to follow upstream strict policy if upstream proves
> that it stands behind it and downstream is willing to follow.
I don't think we should do that unless we provide a USE flag for users
to opt into the behavior. Forcing it on users is problematic for the
reasons others stated. However, letting them opt into the behavior is
reasonable.

In the case of sys-fs/zfs, enabling -Werror (which includes -Wall) on
USE=debug is following upstream's wishes to build debug builds with -Werror.
>
> For your question: No. Downstream should not add -Werror to upstream
> package, not in a parameter or USE flag, as this will probably break
> and cause a queue of downstream patches.
>
>>> I would like to suggest a modify our policy with the following
>>> exception clause: Package maintainer may decide to keep upstream
>>> -Werror as long as he is willing to resolve all issues resulting from
>>> -Werror as if it was a blocker bug.
>>
>> Do you intend to keep -Werror for case when ebuild goes
>> stable as well?
>
> Correct.
>
>> If you do it then what is your workflow to fix breakages
>> appearing in stable packages caused by minor environment
>> changes like toolchain tweaks?
>
> Correct.
>
>> Add another round of stabilization on each arch team? It
>> sounds like your default assumption that code needs to be
>> changed in a semantically significant way: add annotations
>> that might not like some toolchains, remove unused code.
>
> No dependency of toolchain nor annotations.
> A strict policy of no warnings will require changes as dependencies
> including toolchain are progressing.
> This creates an overhead for selected packages.
> A maintainer and the non-stable team should be able to know the package status.
> Preferably this may be done by automation, I appreciate the work of
> Toralf Förster with tinderbox to automate check various cases.
> When a new slot of toolchain is available, maintainers should check
> their packages in any case, there are other issues and side affects
> that we experienced, especially in C++ packages.
> For these packages usually there are 3 for each slot: the current
> stable, the next stable and the non-stable, so the overhead is
> constrained.
>
>> Or patch package in-place without bumping? None of options
>> sound optimal compared to dropping -Werror.
>
> Success of build is not the only concern although I see people here
> that are interested only in that.
> Patching upstream package and/or change upstream quality policy is
> something that we should avoid as well to maintain upstream warranty.
>
>>> The package maintainer decision may be based on the package state and
>>> the relationship with upstream, but basically, it is his decision as
>>> long as issues are fixed in timely manner, it is his overhead after
>>> all.
>>
>> I agree that maintainer's will and upstream's will should be
>> respected and it's not something needs to be absolutely
>> enforced all the time.
>>
>> Personally I disagree -Werror on end-user machine is worth
>> it (or cppcheck, or coverity round-trip run is worth running
>> on user's machine unconditionally).
>>
>> Unused variable is a good example of typical benign warning:
>> it was there all the time, it's not a new bug and does not
>> warrant need for an immediate fix.
>
> Unused variable is a good example of CRITICAL potential issue, the bug
> that triggered this this discussion has a return code that was not
> used. The permutation was not tested by upstream as it rarely used, it
> was not tested by me either by the same reason, both is a mistake.
> Fortunately, someone else tested this permutation and his build
> failed, triggered a bug. If -Werror has not been used we would not
> have known about this issue. In many cases these happen in
> architecture that maintainer nor upstream have access to. In this
> specific case I went over the code history to the time the return code
> have been used and determined that this indeed should be ignored,
> imagine the opposite. A patch was submitted to upstream to confirm
> resolution as any issue in code, upstream confirmed and merged this in
> timely fashion. Bottom line we all (Gentoo, upstream and any other
> distribution) enjoy better quality.
>
>> Toolchain just happend to get better at control flow graph
>> analysis. Fix can easily wait for next release and save
>> everyone's time.
>
> Once again, the number of permutation of build and architecture may
> reveal issues that are cannot be detected on maintainer machine.
> If a fix is a real issue that is found in stable package, do you
> suggest to wait for next release to save whose time?
>
>> Not every user is willing to create bugzilla account and figure
>> the path of reporting the breakage. Especially if there are
>> many breakages like that. Getting multiple various errors is
>> probable if one imagines -Werror to be commonly allowed.
>> This is user's overhead. Not just maintainer's.
>
> Most of these issues are detected early at process by unstable users
> which are opening bugs.
>
> Once again I outlined the cases in which -Werror can be preserved, I
> will repeat... (a) upstream has strict policy of no-warnings, (b)
> upstream added -Werror, (c) downstream opinion is that upstream is
> following the policy, (d) upstream is friendly, (e) downstream accepts
> the potential maintenance overhead.
>
>> Gentoo does not normally run tests on user's systems either.
>> Tests are arguably a lot more precise in pointing out real
>> problems in software.
>
> Correct. I believe that this may be revisit as well, for selected
> packages in which tests are stable run them on user machines. There is
> no reason why we cannot add a directive to ebuild to enable tests by
> default and let user to disable this to save CPU/time of build.
>
>>> ARGUMENT: If a package compiled in the past using toolchain X then it
>>> must continue to do so with any future toolchain.
>>>
>>> I do not understand when "build" is the test for runtime
>>
>> The argument was about "keep compiling". Runtime
>> behaviour is a separate issue and (in my opinion) is an
>> orthogonal topic.
>>
>> On another note I occasionally like to build Gentoo with
>> clang's -Weverything (or equivalent set of gcc CFLAGS)
>> to get the idea if there is a good useful warning out there
>> to put into -Werror= list.
>>
>> Explicit -Werror= list allows code not to regress. But even
>> that is prone to harmless infelicities in libc headers that
>> can't be easily fixed.
>>
>> In case of opensc it just does not compile even if I pass -Wno-error:
>>     $ CC=clang CFLAGS="-O2 -Weverything -Wno-error" emerge -v1 opensc
>>
>> I don't expect 'opensc' upstream to fix this use for me
>> and don't see it worth reporting to bugzilla.
>>
>> But maybe I'm wrong?
>
> When downstream maintainer create healthy relationship with upstream,
> and when mutual interest is to support clang then working together to
> improve upstream support of clang instead doing that at downstream is
> much better solution to the entire open source eco-system.
>
> Regards,
> Alon
>