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
On Thu, Aug 11, 2016 at 2:33 PM, Fabiano Fidêncio fidencio@redhat.com 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?
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 fidencio@redhat.com 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!
Jakub,
On Mon, Sep 26, 2016 at 11:35 AM, Jakub Hrozek jhrozek@redhat.com 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 fidencio@redhat.com 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.
Best Regards, -- Fabiano Fidêncio
On (26/09/16 12:14), Fabiano Fidêncio wrote:
Jakub,
On Mon, Sep 26, 2016 at 11:35 AM, Jakub Hrozek jhrozek@redhat.com 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 fidencio@redhat.com 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.
LS
On (26/09/16 21:26), Lukas Slebodnik wrote:
On (26/09/16 12:14), Fabiano Fidêncio wrote:
Jakub,
On Mon, Sep 26, 2016 at 11:35 AM, Jakub Hrozek jhrozek@redhat.com 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 fidencio@redhat.com 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.
BTW Stew Walter wrote similar patch for sssd as well. It was much simpler.
https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.o... https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.o...
LS
On Mon, Sep 26, 2016 at 9:26 PM, Lukas Slebodnik lslebodn@redhat.com wrote:
On (26/09/16 12:14), Fabiano Fidêncio wrote:
Jakub,
On Mon, Sep 26, 2016 at 11:35 AM, Jakub Hrozek jhrozek@redhat.com 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 fidencio@redhat.com 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.
Best Regards, -- Fabiano Fidêncio
On (26/09/16 21:47), Fabiano Fidêncio wrote:
On Mon, Sep 26, 2016 at 9:26 PM, Lukas Slebodnik lslebodn@redhat.com wrote:
On (26/09/16 12:14), Fabiano Fidêncio wrote:
Jakub,
On Mon, Sep 26, 2016 at 11:35 AM, Jakub Hrozek jhrozek@redhat.com 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 fidencio@redhat.com 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
LS
On Mon, Sep 26, 2016 at 9:58 PM, Lukas Slebodnik lslebodn@redhat.com wrote:
On (26/09/16 21:47), Fabiano Fidêncio wrote:
On Mon, Sep 26, 2016 at 9:26 PM, Lukas Slebodnik lslebodn@redhat.com wrote:
On (26/09/16 12:14), Fabiano Fidêncio wrote:
Jakub,
On Mon, Sep 26, 2016 at 11:35 AM, Jakub Hrozek jhrozek@redhat.com 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 fidencio@redhat.com 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. 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, IMO, mainly if the reason is 100 seconds in a process that already takes 40+ minutes.
Best Regards, -- Fabiano Fidêncio
On (26/09/16 23:32), Fabiano Fidêncio wrote:
On Mon, Sep 26, 2016 at 9:58 PM, Lukas Slebodnik lslebodn@redhat.com wrote:
On (26/09/16 21:47), Fabiano Fidêncio wrote:
On Mon, Sep 26, 2016 at 9:26 PM, Lukas Slebodnik lslebodn@redhat.com wrote:
On (26/09/16 12:14), Fabiano Fidêncio wrote:
Jakub,
On Mon, Sep 26, 2016 at 11:35 AM, Jakub Hrozek jhrozek@redhat.com 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 fidencio@redhat.com 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.
LS
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 lslebodn@redhat.com wrote:
On (26/09/16 21:47), Fabiano Fidêncio wrote:
On Mon, Sep 26, 2016 at 9:26 PM, Lukas Slebodnik lslebodn@redhat.com wrote:
On (26/09/16 12:14), Fabiano Fidêncio wrote:
Jakub,
On Mon, Sep 26, 2016 at 11:35 AM, Jakub Hrozek jhrozek@redhat.com 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 fidencio@redhat.com 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@lists.fedorahosted.org
-
Fabiano Fidêncio -
Jakub Hrozek -
Lukas Slebodnik