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/sssd-devel@lists.fedorahosted.o...
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@redhat.com
f59828a (Fabiano Fidêncio, 5 days ago) NSS: Fix "old-style-definition" warning caught by GCC
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
f22aff7 (Fabiano Fidêncio, 5 days ago) SIFP: Fix a "jump-misses-init" warning caught by GCC
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
58609d3 (Fabiano Fidêncio, 5 days ago) RESOLV: Fix a "-Werror=null-dereference" caught by GCC
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
bd1d7fd (Fabiano Fidêncio, 5 days ago) RESOLV: Simplify reply_weight_rearrange() a little bit
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com """
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
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
URL: https://github.com/SSSD/sssd/pull/50 Title: #50: [RFC] Use GNULIB's compiler warning code
Label: +Changes requested
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
URL: https://github.com/SSSD/sssd/pull/50 Title: #50: [RFC] Use GNULIB's compiler warning code
Label: -Changes requested
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
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
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
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
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
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
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
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
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
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
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
URL: https://github.com/SSSD/sssd/pull/50 Title: #50: [RFC] Use GNULIB's compiler warning code
Label: +Rejected
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
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
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
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
sssd-devel@lists.fedorahosted.org
-
fidencio