On Tue, Sep 27, 2016 at 12:06:32AM +0200, Lukas Slebodnik wrote:
> On (26/09/16 23:32), Fabiano Fidêncio wrote:
> >On Mon, Sep 26, 2016 at 9:58 PM, Lukas Slebodnik <[email protected]> wrote:
> >> On (26/09/16 21:47), Fabiano Fidêncio wrote:
> >>>On Mon, Sep 26, 2016 at 9:26 PM, Lukas Slebodnik <[email protected]>
> >>>wrote:
> >>>> On (26/09/16 12:14), Fabiano Fidêncio wrote:
> >>>>>Jakub,
> >>>>>
> >>>>>On Mon, Sep 26, 2016 at 11:35 AM, Jakub Hrozek <[email protected]>
> >>>>>wrote:
> >>>>>> On Mon, Sep 05, 2016 at 03:39:19PM +0200, Fabiano Fidêncio wrote:
> >>>>>>> On Thu, Aug 11, 2016 at 2:33 PM, Fabiano Fidêncio
> >>>>>>> <[email protected]> wrote:
> >>>>>>> > Howdy!
> >>>>>>> >
> >>>>>>> > I've suggested, a long time ago, that we could start making use of
> >>>>>>> > GNULIB's compiler warnings from 'manywarnings' module. This is
> >>>>>>> > basically what we have been doing in a few projects that I (used to
> >>>>>>> > and still) maintain (like spice-gtk and libosinfo, for instance).
> >>>>>>> >
> >>>>>>> > For now I didn't try to fix any of the warnings that we cannot cope
> >>>>>>> > with, mainly because I'm not sure whether you guys will agree on
> >>>>>>> > using
> >>>>>>> > it or not.
> >>>>>>> >
> >>>>>>> > Here is an experimental patch that works properly on Fedora 24. I
> >>>>>>> > still have to make some tests on RHEL-6, RHEL-7 and a few other
> >>>>>>> > systems (Debian, at least) in order to make sure that we won't break
> >>>>>>> > the build because of the patch.
> >>>>>>> >
> >>>>>>> > If you are okay with the change, I'll start going through the
> >>>>>>> > warnings
> >>>>>>> > that we cannot cope with and slowly start fixing them. Although, I
> >>>>>>> > have the feeling that fixing some of them would cause a lot of
> >>>>>>> > undesired changes, which will just bring troubles for ourselves when
> >>>>>>> > backporting fixes downstream (and here I'm talking about
> >>>>>>> > -Wformat-signedess, -Wsign-compare, -Wunused-parameter, ... for
> >>>>>>> > instance).
> >>>>>>> >
> >>>>>>> > I'm looking forward to hear some feedback!
> >>>>>>> >
> >>>>>>> > Best Regards,
> >>>>>>> > --
> >>>>>>> > Fabiano Fidêncio
> >>>>>>>
> >>>>>>> ping?
> >>>>>>
> >>>>>> I'm sorry this patch totally stalled.
> >>>>>>
> >>>>>> In general I'm all for adding more checks and let a machine help us
> >>>>>> write safer code. I'm not sure if adding all warnings on all platforms
> >>>>>> will ever be possible, though. For example, there was a warning in
> >>>>>> krb5_child because an old libkrb5 release used a "char *" parameter
> >>>>>> where a "const char *" was more appropriate and we said we'd never fix
> >>>>>> this because a newer version fixed the API (or used a different
> >>>>>> function? Not sure..)
> >>>>>>
> >>>>>> But I wonder how to move this patch forward the best. Are there any
> >>>>>> warnings that you think are more important to enable than others?
> >>>>>>
> >>>>>> What about enabling a single warning, then running SSSD build and
> >>>>>> creating a ticket with the warnings (running make 1</dev/null might be
> >>>>>> a
> >>>>>> good way to start..). Then we can see what needs fixing in SSSD for
> >>>>>> each
> >>>>>> of the additional warnings or if we decide this is not worth our time,
> >>>>>> we can either close or defer that ticket.
> >>>>>>
> >>>>>> These tickets might also be a good way for a newcomer to contribute
> >>>>>> some
> >>>>>> code to SSSD!
> >>>>>
> >>>>>So, general comments here.
> >>>>>
> >>>>>We have a list of the current warnings that we cannot deal with in
> >>>>>src/sssd-compile-warnings.m4. From this list we can, already, have a
> >>>>>good idea about what is worth to fix or not.
> >>>>>
> >>>>>About the idea to try to build SSSD ... well, it can be done.
> >>>>>I'll take some time later on and prepare a bug for each of the
> >>>>>warnings that we can't cope with and them we can discuss there whether
> >>>>>would make sense to fix those or not.
> >>>>>
> >>>> Here are few of my observation.
> >>>> The patch added 255 new configure time checks.
> >>>>
> >>>> The configure tooks 5 extra seconds in my case (everything in ram-disk).
> >>>> But in our CI machines it added +20 seconds in average.
> >>>> The configure scriot is execute 5 times in our CI-script
> >>>> (debug build, distcheck, intgcheck, mock, coverage build)
> >>>>
> >>>> In Summary, the patch would add unnecessary 100 seconds.
> >>>>
> >>>> I would prefer if all compile time warnings were part of AM_CFLAGS
> >>>> and we shoul add compile time checks only for warnings which are
> >>>> supported by
> >>>> new compilers or are supported only by gcc or only by clang.
> >>>
> >>>I'm completely fine dropping the patch.
> >>>
> >> I am not against patch.
> >> I want enabling warnings But I would prefer more efficient
> >> version if possible.
> >> Reducing 255 configure time checks to 25 could be reasonable IMHO.
> >> The extra time overhead at configure time would be reasonable.
> >>
> >> e.g. We could enable warning -Wnull-dereference on fedora 25+ (i didn't try
> >> older versions of fedora) but it's not supported by gcc on el6. It cannot
> >> be
> >> added into AM_CFLAGS
> >
> >I'd prefer to just drop the patch than do something else that wouldn't
> >take advantage of the manywarnings' scripts.
> manywarnings script need to be updated from time to time.
> At least after adding new warnings there.
> That would be the same as explicitely ading new warnings to whitelist
> or to the AM_CFLAGS.
>
> >The whole point of the scripts is that you update them every now and
> >then _avoiding_ any kind of work apart from downloading and pushing a
> >file.
> >Any kind of thing that would bring us more work is not worth,
> That's not realy true. We would need to disable new warnings which
> we decides to ignore (does not worth to fix / is not simple to fix)
>
> >IMO, mainly if the reason is 100 seconds in a process that already
> >takes 40+ minutes.
> It takes just 30 minutes in average
> and extra 100 seconds is + 5% of build time.
>
> If you do not like different approach for enabling compiler
> warnings then please at least file a ticket for fixing
> warnings which were disabled in src/sssd-compile-warnings.m4
> and should be possible/simple to fix them.
If the only reason to not accept the patch is 5% slowdown of build time,
would it help to remove some configure checks that are not needed
because we don't care about so old platforms anymore?
I just quickly scanned configure.ac and I was wondering if we still
need:
- checks for libdbus older than 1.0
- checks for dbus_watch_get_unix_fd()
- removing the augeas support would remove some config checks
- there is a check for old pcre versions
- there is check for nsupdate's realm option which new nsupdate
versions have
- there are checks for many krb5 functions that probably exist on
newer releases
...etc...
_______________________________________________
sssd-devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]
