[sssd PR#50][opened] [RFC] Use GNULIB's compiler warning code

[fidencio]

   URL: https://github.com/SSSD/sssd/pull/50
Author: fidencio
 Title: #50: [RFC] Use GNULIB's compiler warning code
Action: opened

PR body:
"""
This patch series was sent to the sssd-devel and some discussions
already happened there[0]. I've decided to open the PR because there
are some few patches that can be pushed even if we decide to not use
the "many warnings" patches.

Let's keep track of those patches (and discussions related to them) in
the github, in this way we can avoid them to get lost. :-)

[0]:
https://lists.fedorahosted.org/archives/list/[e-mail]...

Best Regards,

Changes:

683f72d (Fabiano Fidêncio, 10 weeks ago)
   BUILD: Make use of GNULIB's compiler warning code

   As GNULIB has the 'manywarnings' module, which basically turns on every GCC
   warning, let's make use of it. We can easily blacklist the warnings we
   cannot cope with, but the main goal should be to have enabled every
   possible GCC warning.

   When new GCC warnings are created the 'manywarnings' file can be refreshed
   from upstream GNULIB.

   Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt;

f59828a (Fabiano Fidêncio, 5 days ago)
   NSS: Fix "old-style-definition" warning caught by GCC

   Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt;

f22aff7 (Fabiano Fidêncio, 5 days ago)
   SIFP: Fix a "jump-misses-init" warning caught by GCC

   Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt;

58609d3 (Fabiano Fidêncio, 5 days ago)
   RESOLV: Fix a "-Werror=null-dereference" caught by GCC

   Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt;

bd1d7fd (Fabiano Fidêncio, 5 days ago)
   RESOLV: Simplify reply_weight_rearrange() a little bit

   Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt;
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/50/head:pr50
git checkout pr50

[sssd PR#50][comment] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

lslebodn commented:
"""
It seems that resolv patches broke  `resolv-tests` on fedora 22 and fedora 23.
The test has been running for 30 hours without any result. I've just cancelled it.

BTW CI did not passed on any supported distro.
"""

See the full comment at https://github.com/SSSD/sssd/pull/50#issuecomment-254715255

[sssd PR#50][+Changes requested] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

Label: +Changes requested

[sssd PR#50][synchronized] [RFC] Use GNULIB's compiler warning code

[fidencio]

   URL: https://github.com/SSSD/sssd/pull/50
Author: fidencio
 Title: #50: [RFC] Use GNULIB's compiler warning code
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/50/head:pr50
git checkout pr50

[sssd PR#50][-Changes requested] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

Label: -Changes requested

[sssd PR#50][comment] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

fidencio commented:
"""
I've updated this series and I'm still waiting for CI to finish.
A lot of warnings were just caught when building on RHEL6 machine (which passes gracefully
now).

I'll post the CI results as soon as I have them.

"""

See the full comment at https://github.com/SSSD/sssd/pull/50#issuecomment-259435498

[sssd PR#50][comment] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

fidencio commented:
"""
http://sssd-ci.duckdns.org/logs/job/56/78/summary.html
"""

See the full comment at https://github.com/SSSD/sssd/pull/50#issuecomment-259444704

[sssd PR#50][comment] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

lslebodn commented:
"""
I do not think that initialisation to NULL is the best solution for Wunitialized on
RHLE6.

My experience is that such bugs occurs when there is some suspicious code in functions
which are
inlined.
"""

See the full comment at https://github.com/SSSD/sssd/pull/50#issuecomment-259445650

[sssd PR#50][comment] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

fidencio commented:
"""
@lslebodn, a way easier solution would be to disable -Werror when running CI on RHEL6 than
fixing those issues only caught there. Is there a simple way to do that?
"""

See the full comment at https://github.com/SSSD/sssd/pull/50#issuecomment-259448662

[sssd PR#50][comment] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

lslebodn commented:
"""
On (09/11/16 07:54), fidencio wrote:
> @lslebodn, a way easier solution would be to disable -Werror when
> running CI on RHEL6 than fixing those issues only caught there. Is there a simple way to
> do that?
>  
I agree that it would be better to disable Werror on rhel6.

But your patch "BUILD: Make use of GNULIB's compiler warning code "
introduced Werror. And I am still not satisfied with the patch
because of increased time in CI (10%) caused by detection in manywarnings.

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/50#issuecomment-259450717

[sssd PR#50][comment] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

fidencio commented:
"""
Disabling Werror on RHEL6 will be just a matter of passing "--enable-werror=no"
to configure. It should be **really** simple as long as we agree on doing that.

About the extra extra time, it was 5% last time you reviewed. 100 seconds for a process
that we already have to wait for an hour or so. IMO should not block, at all, introducing
more warnings to our code.
"""

See the full comment at https://github.com/SSSD/sssd/pull/50#issuecomment-259455058

[sssd PR#50][comment] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

fidencio commented:
"""
Lukáš,

I'm afraid you didn't understand how the usage of the GNULIB's compiler
warnings is supposed to be.
There are basically 3 essential files:
- warnings.m4
- manywarnings.m4
- sssd-compile-warnings.m4

The first two you can just get from GNULIB's git repo and update in our repo whenever
there's a update there. So, tweaking those files is something I could do, but it's
wrong per si.
There's the third file then, sssd-compile-warnings.m4, where we disable the warnings
we don't need, want or can't cope with. And in this file I've added all the
warnings we don't need, want or can't cope with. In this very same file, we check
whether a warning is or is not supported.

That's how it's supposed to work. If you think patching Makefile.am is better,
okay, just reject this patch series, open a bug and we will mark as "patches
welcome".

Again, I really don't see a valid point for rejecting this series because it
introduces 100s more in a process that takes an hour or so.

So, what's your suggestion? Can we accept the series? Can you, at least, finish
reviewing the patches that are not RHEL6 specific?
"""

See the full comment at https://github.com/SSSD/sssd/pull/50#issuecomment-259478258

[sssd PR#50][synchronized] [RFC] Use GNULIB's compiler warning code

[fidencio]

   URL: https://github.com/SSSD/sssd/pull/50
Author: fidencio
 Title: #50: [RFC] Use GNULIB's compiler warning code
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/50/head:pr50
git checkout pr50

[sssd PR#50][comment] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

fidencio commented:
"""
@lslebodn:
After some proper checking, seems that all "-Wunitialized" reports are false
positives, so I've removed the patches from this series.
I've added "Reviewed-by: " to the patches you've already ACKed and
dropped the ones you've already NACKed. For the ones you gave some suggestions, I
followed them.

The build is failing in CentOS CI and I'll take a look on it later on. I've
updated my branch based only on my local tests (and they're okay on F25). I
haven't done any test on our CI because it seems to be broken due to lack of space.

Feel free to wait till I solve those issues before starting reviewing. Or feel free to
NACK the series as soon as possible, so I don't spend time on it anymore.
"""

See the full comment at https://github.com/SSSD/sssd/pull/50#issuecomment-259482349

[sssd PR#50][comment] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

fidencio commented:
"""
So, here are some stats about the configure time that you complained before:
So, I've ran the contrib/ci/run script on our F25 machine on Jenkins.

This is what we have **without**  GNULIB patch:
```
-bash-4.3$ ./contrib/ci/run
install-deps:           success  00:00:05 ci-install-deps.log
autoreconf:             success  00:00:22 ci-autoreconf.log
DEBUG BUILD:                              ci-build-debug
configure:              success  00:00:16 ci-build-debug/ci-configure.log
make-tests:             success  00:02:55 ci-build-debug/ci-make-tests.log
make-check-valgrind:    success  00:01:33 ci-build-debug/ci-make-check-valgrind.log
SUCCESS

```

And this is what we have **with** GNULIB patch applied:
```
-bash-4.3$ ./contrib/ci/run 
install-deps:           success  00:00:05 ci-install-deps.log
autoreconf:             success  00:00:23 ci-autoreconf.log
DEBUG BUILD:                              ci-build-debug
configure:              success  00:00:22 ci-build-debug/ci-configure.log
make-tests:             success  00:03:56 ci-build-debug/ci-make-tests.log
make-check-valgrind:    success  00:01:33 ci-build-debug/ci-make-check-valgrind.log
SUCCESS

```

It adds 6 seconds more in the configure time. I really think that dropping those patches
because of 6 seconds on the configure is far, but really really, far away from being
reasonable.

So, can we have the patches reviewed and merged?
"""

See the full comment at https://github.com/SSSD/sssd/pull/50#issuecomment-259738984

[sssd PR#50][comment] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

fidencio commented:
"""
So, here are some stats about the configure time that you complained before:
So, I've ran the contrib/ci/run script on our F25 machine on Jenkins.

This is what we have **without**  GNULIB patch:
```
-bash-4.3$ ./contrib/ci/run
install-deps:           success  00:00:05 ci-install-deps.log
autoreconf:             success  00:00:22 ci-autoreconf.log
DEBUG BUILD:                              ci-build-debug
configure:              success  00:00:16 ci-build-debug/ci-configure.log
make-tests:             success  00:02:55 ci-build-debug/ci-make-tests.log
make-check-valgrind:    success  00:01:33 ci-build-debug/ci-make-check-valgrind.log
SUCCESS

```

And this is what we have **with** GNULIB patch applied:
```
-bash-4.3$ ./contrib/ci/run 
install-deps:           success  00:00:05 ci-install-deps.log
autoreconf:             success  00:00:23 ci-autoreconf.log
DEBUG BUILD:                              ci-build-debug
configure:              success  00:00:22 ci-build-debug/ci-configure.log
make-tests:             success  00:03:56 ci-build-debug/ci-make-tests.log
make-check-valgrind:    success  00:01:33 ci-build-debug/ci-make-check-valgrind.log
SUCCESS

```

It adds 6 seconds more to the configure time. I really think that dropping those patches
because of 6 seconds on the configure is far, but really really, far away from being
reasonable.

So, can we have the patches reviewed and merged?
"""

See the full comment at https://github.com/SSSD/sssd/pull/50#issuecomment-259738984

[sssd PR#50][+Rejected] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

Label: +Rejected

[sssd PR#50][comment] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

fidencio commented:
"""
It's been almost 2 months this code has been around and there's anyone interested
to review.
So, I'm closing the PR (as "rejected") as this patchset doesn't seem to
be useful for the project.
"""

See the full comment at https://github.com/SSSD/sssd/pull/50#issuecomment-269952957

[sssd PR#50][closed] [RFC] Use GNULIB's compiler warning code

[fidencio]

   URL: https://github.com/SSSD/sssd/pull/50
Author: fidencio
 Title: #50: [RFC] Use GNULIB's compiler warning code
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/50/head:pr50
git checkout pr50

[sssd PR#50][comment] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

fidencio commented:
"""
So, I've re-worked those patches a little bit and here is the time difference when
running reconfing with the patches:
```
real	0m26.047s
user	0m21.318s
sys	0m4.635s
```

And now without:
```
real	0m25.565s
user	0m20.696s
sys	0m4.433s
```

This patch set is rebased on top of PR #377.

I really would appreciate if someone could review and give their opinion.

The reason this PR was blocked is because this time difference has been considered a
"performance issue".

@jhrozek, could you take a look on this?
"""

See the full comment at https://github.com/SSSD/sssd/pull/50#issuecomment-328271933

[sssd PR#50][comment] [RFC] Use GNULIB's compiler warning code

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/50
Title: #50: [RFC] Use GNULIB's compiler warning code

fidencio commented:
"""
Ah, somehow seems that I cannot reopen the PR. So, I'm opening a new one.
"""

See the full comment at https://github.com/SSSD/sssd/pull/50#issuecomment-328271998