| Message ID | 20170707022242.20128-2-casantos@datacom.ind.br |
|---|---|
| State | Rejected |
| Headers | show |
Hi Carlos, On Thu, Jul 06, 2017 at 11:22:36PM -0300, Carlos Santos wrote: > This will allow us to use the hidden option in dependent packages, > instead of repeating the dependencies in each Config.in file. > > Signed-off-by: Carlos Santos <casantos@datacom.ind.br> > --- > package/linux-pam/Config.in | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/package/linux-pam/Config.in b/package/linux-pam/Config.in > index 33e5154..0daffe4 100644 > --- a/package/linux-pam/Config.in > +++ b/package/linux-pam/Config.in > @@ -1,9 +1,20 @@ > -config BR2_PACKAGE_LINUX_PAM > - bool "linux-pam" > +# Use this option instead of duplicating the dependencies in each > +# dependent package. > +# > +# If you change these dependencies then update the comment below > +# and the # corresponding ones in other Config.in files. > +# > +config BR2_PACKAGE_LINUX_PAM_ARCH_SUPPORTS > + bool > + default y > depends on (BR2_ENABLE_LOCALE && BR2_USE_WCHAR) > depends on !BR2_STATIC_LIBS > depends on !BR2_TOOLCHAIN_USES_MUSL > depends on BR2_USE_MMU # fork() These are feature dependencies, not arch dependencies, so the config name is misleading. But regardless of that, we want to see this list in all dependent packages, IMO. This allows us to (more) easily see and grep for direct and indirect dependencies. It also makes dependencies comments easier to maintain as you noted. baruch > + > +config BR2_PACKAGE_LINUX_PAM > + bool "linux-pam" > + depends on BR2_PACKAGE_LINUX_PAM_ARCH_SUPPORTS > select BR2_PACKAGE_FLEX > help > A Security Framework that Provides Authentication for Applications > @@ -11,5 +22,4 @@ config BR2_PACKAGE_LINUX_PAM > http://linux-pam.org > > comment "linux-pam needs a uClibc or glibc toolchain w/ wchar, locale, dynamic library" > - depends on !(BR2_ENABLE_LOCALE && BR2_USE_WCHAR) \ > - || BR2_STATIC_LIBS || BR2_TOOLCHAIN_USES_MUSL > + depends on !BR2_PACKAGE_LINUX_PAM_ARCH_SUPPORTS
Hello, On Fri, 7 Jul 2017 06:36:26 +0300, Baruch Siach wrote: > > diff --git a/package/linux-pam/Config.in b/package/linux-pam/Config.in > > index 33e5154..0daffe4 100644 > > --- a/package/linux-pam/Config.in > > +++ b/package/linux-pam/Config.in > > @@ -1,9 +1,20 @@ > > -config BR2_PACKAGE_LINUX_PAM > > - bool "linux-pam" > > +# Use this option instead of duplicating the dependencies in each > > +# dependent package. > > +# > > +# If you change these dependencies then update the comment below > > +# and the # corresponding ones in other Config.in files. > > +# > > +config BR2_PACKAGE_LINUX_PAM_ARCH_SUPPORTS > > + bool > > + default y > > depends on (BR2_ENABLE_LOCALE && BR2_USE_WCHAR) > > depends on !BR2_STATIC_LIBS > > depends on !BR2_TOOLCHAIN_USES_MUSL > > depends on BR2_USE_MMU # fork() > > These are feature dependencies, not arch dependencies, so the config name is > misleading. But regardless of that, we want to see this list in all dependent > packages, IMO. This allows us to (more) easily see and grep for direct and > indirect dependencies. It also makes dependencies comments easier to maintain > as you noted. Correct: the <foo>_ARCH_SUPPORTS hidden options should really only be used for architecture dependencies. So in the list above, that would be just BR2_USE_MMU. All the other dependencies are *not* architecture dependencies, and we want to repeat them explicitly, because each package anyway needs to have a Config.in comment that tells the user about such toolchain dependencies. Therefore, I'm afraid the patch series is not correct :/ Thomas
> To: "Baruch Siach" <baruch@tkos.co.il> > Cc: "Carlos Santos" <casantos@datacom.ind.br>, "Karoly Kasza" <kaszak@gmail.com>, "Ezequiel Garcia" > <ezequiel@vanguardiasur.com.ar>, buildroot@buildroot.org > Sent: Friday, July 7, 2017 4:50:35 AM > Subject: Re: [Buildroot] [PATCH 1/7] linux-pam: introduce BR2_PACKAGE_LINUX_PAM_ARCH_SUPPORTS > Hello, > > On Fri, 7 Jul 2017 06:36:26 +0300, Baruch Siach wrote: > >> > diff --git a/package/linux-pam/Config.in b/package/linux-pam/Config.in >> > index 33e5154..0daffe4 100644 >> > --- a/package/linux-pam/Config.in >> > +++ b/package/linux-pam/Config.in >> > @@ -1,9 +1,20 @@ >> > -config BR2_PACKAGE_LINUX_PAM >> > - bool "linux-pam" >> > +# Use this option instead of duplicating the dependencies in each >> > +# dependent package. >> > +# >> > +# If you change these dependencies then update the comment below >> > +# and the # corresponding ones in other Config.in files. >> > +# >> > +config BR2_PACKAGE_LINUX_PAM_ARCH_SUPPORTS >> > + bool >> > + default y >> > depends on (BR2_ENABLE_LOCALE && BR2_USE_WCHAR) >> > depends on !BR2_STATIC_LIBS >> > depends on !BR2_TOOLCHAIN_USES_MUSL >> > depends on BR2_USE_MMU # fork() >> >> These are feature dependencies, not arch dependencies, so the config name is >> misleading. But regardless of that, we want to see this list in all dependent >> packages, IMO. This allows us to (more) easily see and grep for direct and >> indirect dependencies. It also makes dependencies comments easier to maintain >> as you noted. How would repeating the list in all dependent packages make dependencies comments *easier* to maintain? I didn't meant to say that. > Correct: the <foo>_ARCH_SUPPORTS hidden options should really only be > used for architecture dependencies. So in the list above, that would be > just BR2_USE_MMU. All the other dependencies are *not* architecture > dependencies, and we want to repeat them explicitly, because each > package anyway needs to have a Config.in comment that tells the user > about such toolchain dependencies. What could be used instead of <foo>_ARCH_SUPPORTS in cases like this?
Hello, On Fri, 7 Jul 2017 08:11:26 -0300 (BRT), Carlos Santos wrote: > >> These are feature dependencies, not arch dependencies, so the config name is > >> misleading. But regardless of that, we want to see this list in all dependent > >> packages, IMO. This allows us to (more) easily see and grep for direct and > >> indirect dependencies. It also makes dependencies comments easier to maintain > >> as you noted. > > How would repeating the list in all dependent packages make dependencies > comments *easier* to maintain? I didn't meant to say that. It makes maintaining the comments easier to maintain, because they are always next to the list of dependencies, in the same Config.in file. With your patches, to know what the Config.in comment in package/nodm/Config.in should be, you need to recursively look into package/linux-pam/Config.in to find what BR2_PACKAGE_LINUX_PAM_ARCH_SUPPORTS actually means. > > Correct: the <foo>_ARCH_SUPPORTS hidden options should really only be > > used for architecture dependencies. So in the list above, that would be > > just BR2_USE_MMU. All the other dependencies are *not* architecture > > dependencies, and we want to repeat them explicitly, because each > > package anyway needs to have a Config.in comment that tells the user > > about such toolchain dependencies. > > What could be used instead of <foo>_ARCH_SUPPORTS in cases like this? I don't think we want to change how things are done today. Since we anyway wants to have this Config.in comment that details what the dependencies are, I believe keeping the full list of dependencies is the most reasonable solution. Yes, it's not perfect, but we simply work within the limitations of kconfig. Best regards, Thomas
diff --git a/package/linux-pam/Config.in b/package/linux-pam/Config.in index 33e5154..0daffe4 100644 --- a/package/linux-pam/Config.in +++ b/package/linux-pam/Config.in @@ -1,9 +1,20 @@ -config BR2_PACKAGE_LINUX_PAM - bool "linux-pam" +# Use this option instead of duplicating the dependencies in each +# dependent package. +# +# If you change these dependencies then update the comment below +# and the # corresponding ones in other Config.in files. +# +config BR2_PACKAGE_LINUX_PAM_ARCH_SUPPORTS + bool + default y depends on (BR2_ENABLE_LOCALE && BR2_USE_WCHAR) depends on !BR2_STATIC_LIBS depends on !BR2_TOOLCHAIN_USES_MUSL depends on BR2_USE_MMU # fork() + +config BR2_PACKAGE_LINUX_PAM + bool "linux-pam" + depends on BR2_PACKAGE_LINUX_PAM_ARCH_SUPPORTS select BR2_PACKAGE_FLEX help A Security Framework that Provides Authentication for Applications @@ -11,5 +22,4 @@ config BR2_PACKAGE_LINUX_PAM http://linux-pam.org comment "linux-pam needs a uClibc or glibc toolchain w/ wchar, locale, dynamic library" - depends on !(BR2_ENABLE_LOCALE && BR2_USE_WCHAR) \ - || BR2_STATIC_LIBS || BR2_TOOLCHAIN_USES_MUSL + depends on !BR2_PACKAGE_LINUX_PAM_ARCH_SUPPORTS
This will allow us to use the hidden option in dependent packages, instead of repeating the dependencies in each Config.in file. Signed-off-by: Carlos Santos <casantos@datacom.ind.br> --- package/linux-pam/Config.in | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)