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 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: """ 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

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

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: """ 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 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 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

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 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