[SSSD] Re: [sssd PR#10] UTIL: Remove support to libini older than 1.0.0 (comment)
On Wed, Sep 7, 2016 at 9:03 AM, Lukas Slebodnik wrote: > On (07/09/16 08:46), Fabiano Fidêncio wrote: >>On Wed, Sep 7, 2016 at 8:34 AM, Lukas Slebodnik wrote: >>> On (06/09/16 21:38), Fabiano Fidêncio wrote: On Tue, Sep 6, 2016 at 8:49 PM, lslebodn wrote: > lslebodn commented on a pull request > > """ > IMHO, it might be better to close this PR. > If we decide to dor support for libini_config < 1.1 or 1.2 > then it will be a different patch anyway. @see my previous comment > """ Please, take a look on Jakub's comment and see what he proposed. >>> Do you mean: jhrozek commented on a pull request """ > - Ubuntu 12.04 LTS and older 12.04 is often used (for example travis-ci still offers only this distro) and ends its life in 2017. On one hand, it's unlikely that users of LTS distributions will run master, they will probably only run sssd-1-13 or some PPAs, on the other hand, I don't see too much value except for cleaner code. So all in all I suggest we nack this patchset for now and push it when Ubuntu 12.04 goes EOL. Does that sound like a reasonable compromise? """ >>> If we decide to drop support for older versions of libini_config >>> then we should also remove unnecessary wrappers in src/util/sss_ini.c. >>> and replace sss_ini* functions with direct usage of ini_*. >>> >>> Therefore this patchset will need to be changed. >>> IMHO, it will be much simpler to create new patch-set then. >>> >>> Anyways, I'm fine with closing this PR and opening a new one when we can drop support to libini_config < 1.3. Just for the record, from this whole discussion I could see that dropping support to augeas in order to use libini is something that shouldn't and is not going to happen any time soon >>> I do not understand how is this patch-set related to replacing augeas >>> with libini_config. >> >>Well, this patch set is _not_ related to replacing augeas with >>libini_config, we just will fall under the same issue in the future >>when doing that. >>If we introduce a new code that will deal with configuration files the >>least I would expect is that old systems could have some benefit from it >>in the same way they can have benefit from the current code. > Maybe you did not get my point and therefore you get wrong expectations. > I do not expect that old systems will benefit from new features. > e.g. > Currently rhel6 cannot: > * validate configuration file because it has old version of libini_config > * be integrated with "CIFS Client"[1] because it does not have necessary > dependencies > ... > >>So, if I'm the one writing or reviewing the code with this discussion in mind >>I'd opt for something that is present in the major distros and for >>sure libini > 1.3 is not. So, why not use augeas instead of it? :-) >> > Augeas is optional dependency libini_config-1.2 is optional dependency. > I cannot see a conflict for replacing it :-) > But it also does not make a sense > because we should drop manipulation with sssd.conf Okay, okay, > >>> >>> a) augeas is an optional dependency >>>if BUILD_IFP >>>if BUILD_CONFIG_LIB >>>pkglib_LTLIBRARIES += libsss_config.la >>> b) replacing augeas with libini_config requires minimal version 1.2 >>>which is already optionally used in master. >>> c) IIRC discussion from devel meeting. Some developers prefer >>>to drop feature for manipulation of sssd.conf and write from >>>scratch if there will be new requirements. Current version is very >>>limited. >>> as well and for the very same reasons that this patch wasn't accepted. We could, in the best (or worst?) case scenario support/depend on both due to compatibility with elder systems, but I can't see any advantage on that. >>> I do not suggest to support all features in old systems. >>> But I would like to support at least minimal feature set (ldap+krb5). >>> And parsing configuration file is required for minimal feature set :-) >>> >> >>Sure, sure. >> >>We wasted a lot of time on this patch set that could have been used >>reviewing more useful patches on the queue. >>Please, close the PR if you want to. >> > I'm sorry if you think we wasted a lot of time. There's no reason to be sorry for. > I did not want to close PR without proper explanation why I would > prefer to keep this optional code in sssd. It would be impolite > to close PR without general agreement. I don't think we are going to agree here. So, please, just close the PR. Best Regards, -- Fabiano Fidêncio ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [sssd PR#10] UTIL: Remove support to libini older than 1.0.0 (comment)
On (07/09/16 08:46), Fabiano Fidêncio wrote: >On Wed, Sep 7, 2016 at 8:34 AM, Lukas Slebodnik wrote: >> On (06/09/16 21:38), Fabiano Fidêncio wrote: >>>On Tue, Sep 6, 2016 at 8:49 PM, lslebodn >>> wrote: lslebodn commented on a pull request """ IMHO, it might be better to close this PR. If we decide to dor support for libini_config < 1.1 or 1.2 then it will be a different patch anyway. @see my previous comment """ >>> >>>Please, take a look on Jakub's comment and see what he proposed. >>> >> Do you mean: >>> >>>jhrozek commented on a pull request >>> >>>""" - Ubuntu 12.04 LTS and older >>> >>>12.04 is often used (for example travis-ci still offers only this >>>distro) and ends its life in 2017. >>> >>>On one hand, it's unlikely that users of LTS distributions will run >>>master, they will probably only run sssd-1-13 or some PPAs, on the other >>>hand, I don't see too much value except for cleaner code. >>> >>>So all in all I suggest we nack this patchset for now and push it when >>>Ubuntu 12.04 goes EOL. >>> >>>Does that sound like a reasonable compromise? >>> >>>""" >> If we decide to drop support for older versions of libini_config >> then we should also remove unnecessary wrappers in src/util/sss_ini.c. >> and replace sss_ini* functions with direct usage of ini_*. >> >> Therefore this patchset will need to be changed. >> IMHO, it will be much simpler to create new patch-set then. >> >> >>>Anyways, I'm fine with closing this PR and opening a new one when we >>>can drop support to libini_config < 1.3. >>> >>>Just for the record, from this whole discussion I could see that >>>dropping support to augeas in order to use libini is something that >>>shouldn't and is not going to happen any time soon >> I do not understand how is this patch-set related to replacing augeas >> with libini_config. > >Well, this patch set is _not_ related to replacing augeas with >libini_config, we just will fall under the same issue in the future >when doing that. >If we introduce a new code that will deal with configuration files the >least I would expect is that old systems could have some benefit from it >in the same way they can have benefit from the current code. Maybe you did not get my point and therefore you get wrong expectations. I do not expect that old systems will benefit from new features. e.g. Currently rhel6 cannot: * validate configuration file because it has old version of libini_config * be integrated with "CIFS Client"[1] because it does not have necessary dependencies ... >So, if I'm the one writing or reviewing the code with this discussion in mind >I'd opt for something that is present in the major distros and for >sure libini > 1.3 is not. So, why not use augeas instead of it? :-) > Augeas is optional dependency libini_config-1.2 is optional dependency. I cannot see a conflict for replacing it :-) But it also does not make a sense because we should drop manipulation with sssd.conf >> >> a) augeas is an optional dependency >>if BUILD_IFP >>if BUILD_CONFIG_LIB >>pkglib_LTLIBRARIES += libsss_config.la >> b) replacing augeas with libini_config requires minimal version 1.2 >>which is already optionally used in master. >> c) IIRC discussion from devel meeting. Some developers prefer >>to drop feature for manipulation of sssd.conf and write from >>scratch if there will be new requirements. Current version is very >>limited. >> >>>as well and for the >>>very same reasons that this patch wasn't accepted. We could, in the >>>best (or worst?) case scenario support/depend on both due to >>>compatibility with elder systems, but I can't see any advantage on >>>that. >>> >> I do not suggest to support all features in old systems. >> But I would like to support at least minimal feature set (ldap+krb5). >> And parsing configuration file is required for minimal feature set :-) >> > >Sure, sure. > >We wasted a lot of time on this patch set that could have been used >reviewing more useful patches on the queue. >Please, close the PR if you want to. > I'm sorry if you think we wasted a lot of time. I did not want to close PR without proper explanation why I would prefer to keep this optional code in sssd. It would be impolite to close PR without general agreement. LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [sssd PR#10] UTIL: Remove support to libini older than 1.0.0 (comment)
On Wed, Sep 7, 2016 at 8:34 AM, Lukas Slebodnik wrote: > On (06/09/16 21:38), Fabiano Fidêncio wrote: >>On Tue, Sep 6, 2016 at 8:49 PM, lslebodn >> wrote: >>> lslebodn commented on a pull request >>> >>> """ >>> IMHO, it might be better to close this PR. >>> If we decide to dor support for libini_config < 1.1 or 1.2 >>> then it will be a different patch anyway. @see my previous comment >>> """ >> >>Please, take a look on Jakub's comment and see what he proposed. >> > Do you mean: >> >>jhrozek commented on a pull request >> >>""" >>> - Ubuntu 12.04 LTS and older >> >>12.04 is often used (for example travis-ci still offers only this >>distro) and ends its life in 2017. >> >>On one hand, it's unlikely that users of LTS distributions will run >>master, they will probably only run sssd-1-13 or some PPAs, on the other >>hand, I don't see too much value except for cleaner code. >> >>So all in all I suggest we nack this patchset for now and push it when >>Ubuntu 12.04 goes EOL. >> >>Does that sound like a reasonable compromise? >> >>""" > If we decide to drop support for older versions of libini_config > then we should also remove unnecessary wrappers in src/util/sss_ini.c. > and replace sss_ini* functions with direct usage of ini_*. > > Therefore this patchset will need to be changed. > IMHO, it will be much simpler to create new patch-set then. > > >>Anyways, I'm fine with closing this PR and opening a new one when we >>can drop support to libini_config < 1.3. >> >>Just for the record, from this whole discussion I could see that >>dropping support to augeas in order to use libini is something that >>shouldn't and is not going to happen any time soon > I do not understand how is this patch-set related to replacing augeas > with libini_config. Well, this patch set is _not_ related to replacing augeas with libini_config, we just will fall under the same issue in the future when doing that. If we introduce a new code that will deal with configuration files the least I would expect is that old systems could have some benefit from it in the same way they can have benefit from the current code. So, if I'm the one writing or reviewing the code with this discussion in mind I'd opt for something that is present in the major distros and for sure libini > 1.3 is not. So, why not use augeas instead of it? :-) > > a) augeas is an optional dependency >if BUILD_IFP >if BUILD_CONFIG_LIB >pkglib_LTLIBRARIES += libsss_config.la > b) replacing augeas with libini_config requires minimal version 1.2 >which is already optionally used in master. > c) IIRC discussion from devel meeting. Some developers prefer >to drop feature for manipulation of sssd.conf and write from >scratch if there will be new requirements. Current version is very >limited. > >>as well and for the >>very same reasons that this patch wasn't accepted. We could, in the >>best (or worst?) case scenario support/depend on both due to >>compatibility with elder systems, but I can't see any advantage on >>that. >> > I do not suggest to support all features in old systems. > But I would like to support at least minimal feature set (ldap+krb5). > And parsing configuration file is required for minimal feature set :-) > Sure, sure. We wasted a lot of time on this patch set that could have been used reviewing more useful patches on the queue. Please, close the PR if you want to. Best Regards, -- Fabiano Fidêncio ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [sssd PR#10] UTIL: Remove support to libini older than 1.0.0 (comment)
On (06/09/16 21:38), Fabiano Fidêncio wrote: >On Tue, Sep 6, 2016 at 8:49 PM, lslebodn > wrote: >> lslebodn commented on a pull request >> >> """ >> IMHO, it might be better to close this PR. >> If we decide to dor support for libini_config < 1.1 or 1.2 >> then it will be a different patch anyway. @see my previous comment >> """ > >Please, take a look on Jakub's comment and see what he proposed. > Do you mean: > >jhrozek commented on a pull request > >""" >> - Ubuntu 12.04 LTS and older > >12.04 is often used (for example travis-ci still offers only this >distro) and ends its life in 2017. > >On one hand, it's unlikely that users of LTS distributions will run >master, they will probably only run sssd-1-13 or some PPAs, on the other >hand, I don't see too much value except for cleaner code. > >So all in all I suggest we nack this patchset for now and push it when >Ubuntu 12.04 goes EOL. > >Does that sound like a reasonable compromise? > >""" If we decide to drop support for older versions of libini_config then we should also remove unnecessary wrappers in src/util/sss_ini.c. and replace sss_ini* functions with direct usage of ini_*. Therefore this patchset will need to be changed. IMHO, it will be much simpler to create new patch-set then. >Anyways, I'm fine with closing this PR and opening a new one when we >can drop support to libini_config < 1.3. > >Just for the record, from this whole discussion I could see that >dropping support to augeas in order to use libini is something that >shouldn't and is not going to happen any time soon I do not understand how is this patch-set related to replacing augeas with libini_config. a) augeas is an optional dependency if BUILD_IFP if BUILD_CONFIG_LIB pkglib_LTLIBRARIES += libsss_config.la b) replacing augeas with libini_config requires minimal version 1.2 which is already optionally used in master. c) IIRC discussion from devel meeting. Some developers prefer to drop feature for manipulation of sssd.conf and write from scratch if there will be new requirements. Current version is very limited. >as well and for the >very same reasons that this patch wasn't accepted. We could, in the >best (or worst?) case scenario support/depend on both due to >compatibility with elder systems, but I can't see any advantage on >that. > I do not suggest to support all features in old systems. But I would like to support at least minimal feature set (ldap+krb5). And parsing configuration file is required for minimal feature set :-) LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [sssd PR#10] UTIL: Remove support to libini older than 1.0.0 (comment)
On Tue, Sep 6, 2016 at 8:49 PM, lslebodn wrote: > lslebodn commented on a pull request > > """ > IMHO, it might be better to close this PR. > If we decide to dor support for libini_config < 1.1 or 1.2 > then it will be a different patch anyway. @see my previous comment > """ Please, take a look on Jakub's comment and see what he proposed. Anyways, I'm fine with closing this PR and opening a new one when we can drop support to libini_config < 1.3. Just for the record, from this whole discussion I could see that dropping support to augeas in order to use libini is something that shouldn't and is not going to happen any time soon as well and for the very same reasons that this patch wasn't accepted. We could, in the best (or worst?) case scenario support/depend on both due to compatibility with elder systems, but I can't see any advantage on that. Best Regards, -- Fabiano Fidêncio ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [sssd PR#10] UTIL: Remove support to libini older than 1.0.0 (comment)
On Tue, Sep 6, 2016 at 2:30 PM, jhrozek wrote: > jhrozek commented on a pull request > > """ > On Tue, Sep 06, 2016 at 05:16:00AM -0700, fidencio wrote: >> The distributions that would break with this patch are: >> - RHEL/CentOS 5 and older > > I don't think we care about RHEL-5 with master and I'm not sure sssd > master even builds there. > >> - Debian Wheezy (from 2013) and older > > OK, this one I think we care about for the basic functionality: > 7.0 Wheezy May 4th 2013 April 26th 2016 (full) / May 2018 (LTS) > But LTS means bug fixes and for those I think sssd-1-13 would be OK. > >> - Ubuntu 12.04 LTS and older > > 12.04 is often used (for example travis-ci still offers only this > distro) and ends its life in 2017. > > On one hand, it's unlikely that users of LTS distributions will run > master, they will probably only run sssd-1-13 or some PPAs, on the other > hand, I don't see too much value except for cleaner code. > > So all in all I suggest we nack this patchset for now and push it when > Ubuntu 12.04 goes EOL. > > Does that sound like a reasonable compromise? Works for me. > > """ > > See the full comment at > https://github.com/SSSD/sssd/pull/10#issuecomment-244935329 > > ___ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org > ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
