[PATCH] Require explicit 'without flags' for acl, ldap and ads

[Jelmer Vernooij]

On Thu, Dec 19, 2013 at 02:04:11PM +1300, Garming Sam wrote:
> On 18/12/13 02:12, Jelmer Vernooij wrote:
> >On Tue, Dec 17, 2013 at 04:12:07PM +1300, Garming Sam wrote:
> >>On 17/12/13 01:54, Jelmer Vernooij wrote:
> >>>On Mon, Dec 16, 2013 at 04:45:19PM +1300, Andrew Bartlett wrote:
> >>>>On Mon, 2013-12-16 at 16:25 +1300, Garming Sam wrote:
> >>>>>These patches are designed to make it more difficult to build Samba
> >>>>>without ACL, LDAP or ADS support. By default, it is already set to
> >>>>>include support for these and by requiring them to included, it makes it
> >>>>>easier to ensure that the build is fully functional (or in any case,
> >>>>>indicates the packages which should be expected by default).
> >>>>>
> >>>>>We would subsequently have to work with the build farm to update it to
> >>>>>have these additional options set. Otherwise, we could attempt to get
> >>>>>the owners to install the missing packages.
> >>>>Thanks Garming!
> >>>>
> >>>>Reviewed-by: Andrew Bartlett <abartlet at samba.org>
> >>>>
> >>>>Can I get some comment and review from others on the team?  Remember,
> >>>>nothing about this prevents a build working that worked before, but it
> >>>>might just require an explicit --without flag or two.  This is based on
> >>>>what I wrote in BUILD_SYSTEMS.txt back before 4.0, but never had time to
> >>>>implement.
> >>>>
> >>>>This is related to the recent enquiry from Gentoo.
> >>>LGTM. The only reservation I have is about showing this error to users who
> >>>can't install the acl or xattr libraries because they're not available for
> >>>their platforms.
> >>>
> >>>Do we consider platforms that don't have these libraries second-class citizens?
> >>>Can we suggest users migrate to a platform that does have those libraries?
> >>>
> >>Modified the messages to be slightly more informative and why the
> >>support should be included.
> >>
> >>I think it would be a good idea to consider platforms that don't
> >>have these libraries to be second class.
> >>
> >>As for suggesting migration, I think it's a little unreasonable and
> >>likely the best thing to do is just to give them a slight nudge by
> >>saying which features they're actually missing out on.
> >Thanks. Some more comments below:
> >
> >>Signed-off-by: Garming Sam <garming at catalyst.net.nz>
> >>---
> >>  source3/wscript | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/source3/wscript b/source3/wscript
> >>index b09c2db..2155c29 100644
> >>--- a/source3/wscript
> >>+++ b/source3/wscript
> >>@@ -428,7 +428,9 @@ utimensat vsyslog _write __write __xstat
> >>                  conf.DEFINE('HAVE_TRU64_ACLS',1)
> >>                  default_static_modules.extend(TO_LIST('vfs_tru64acl'))
> >>          elif (host_os.rfind('darwin') > -1):
> >>-            Logs.warn('ACLs on Dwarwin currently not supported')
> >>+            Logs.warn('ACLs on Darwin currently not supported')
> >>+            conf.fatal("ACL support not available on Darwin/MacOS. Use --without-acl-support for building without ACL support. " \
> >>+                "ACL support is required to change permissions for Windows clients.")
> >Is this technically true? I thought we'd just fall back to a TDB with severely
> >degraded performance, or was that just using the source4/ SMB server?
> >
> >> From 3be4edb231b1a688dbbbedf0e9e49a290df99d3b Mon Sep 17 00:00:00 2001
> >>From: Garming Sam <garming at catalyst.net.nz>
> >>Date: Mon, 16 Dec 2013 13:31:31 +1300
> >>Subject: [PATCH 2/5] waf: Require ldap support to be specifically disabled
> >>
> >>Signed-off-by: Garming Sam <garming at catalyst.net.nz>
> >>---
> >>  source3/wscript | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >>diff --git a/source3/wscript b/source3/wscript
> >>index 2155c29..a467065 100644
> >>--- a/source3/wscript
> >>+++ b/source3/wscript
> >>@@ -652,6 +652,9 @@ msg.msg_acctrightslen = sizeof(fd);
> >>              if conf.CONFIG_SET('HAVE_BER_SOCKBUF_ADD_IO') and \
> >>                      conf.CONFIG_SET('HAVE_LDAP_OPT_SOCKBUF'):
> >>                  conf.DEFINE('HAVE_LDAP_SASL_WRAPPING', '1')
> >>+        else:
> >>+            conf.fatal("LDAP support not found. Try installing libldap2-dev or openldap-devel. Otherwise, use --without-ldap to build without LDAP support. " \
> >>+                "LDAP support is required for the LDAP passdb backend.")
> >LDAP support is required for more than just the LDAP passdb backend.
> >
> >Cheers,
> >
> >Jelmer
> 
> I added a couple more things to the LDAP message.
> 
> Cheers,
> 
> Garming Sam

> From 5f0b1de5e94e45b86031db4a7b5357eff7c27bae Mon Sep 17 00:00:00 2001
> From: Garming Sam <garming at catalyst.net.nz>
> Date: Mon, 16 Dec 2013 12:45:45 +1300
> Subject: [PATCH 1/5] waf: Require ACL support to be specifically disabled
> 
> Signed-off-by: Garming Sam <garming at catalyst.net.nz>
> ---
>  source3/wscript | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/source3/wscript b/source3/wscript
> index f87a0a8..a128b52 100644
> --- a/source3/wscript
> +++ b/source3/wscript
> @@ -428,7 +428,9 @@ utimensat vsyslog _write __write __xstat
>                  conf.DEFINE('HAVE_TRU64_ACLS',1)
>                  default_static_modules.extend(TO_LIST('vfs_tru64acl'))
>          elif (host_os.rfind('darwin') > -1):
> -            Logs.warn('ACLs on Dwarwin currently not supported')
> +            Logs.warn('ACLs on Darwin currently not supported')
> +            conf.fatal("ACL support not available on Darwin/MacOS. Use --without-acl-support for building without ACL support. " \
> +                "ACL support is required to change permissions for Windows clients.")
>          else:
>              conf.CHECK_FUNCS_IN('acl_get_file', 'acl')
>              if conf.CHECK_CODE('''
> @@ -449,7 +451,9 @@ return acl_get_perm_np(permset_d, perm);
>                          headers='sys/types.h sys/acl.h', link=True,
>                          msg="Checking whether acl_get_perm_np() is available")
>                  default_static_modules.extend(TO_LIST('vfs_posixacl'))
> -
> +            else:
> +                conf.fatal("ACL support not found. Try installing libacl1-dev or libacl-devel.  Otherwise, use --without-acl-support to build without ACL support. " \
> +                    "ACL support is required to change permissions for Windows clients.")
>  
>      if conf.CHECK_FUNCS('dirfd'):
>          conf.DEFINE('HAVE_DIRFD_DECL', 1)
> -- 
> 1.8.3.2
> 

> From 77762d3ded7d3534872ec07740cd5122d56a6147 Mon Sep 17 00:00:00 2001
> From: Garming Sam <garming at catalyst.net.nz>
> Date: Mon, 16 Dec 2013 13:31:31 +1300
> Subject: [PATCH 2/5] waf: Require ldap support to be specifically disabled
> 
> Signed-off-by: Garming Sam <garming at catalyst.net.nz>
> ---
>  source3/wscript | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/source3/wscript b/source3/wscript
> index a128b52..c1f722e 100644
> --- a/source3/wscript
> +++ b/source3/wscript
> @@ -652,6 +652,10 @@ msg.msg_acctrightslen = sizeof(fd);
>              if conf.CONFIG_SET('HAVE_BER_SOCKBUF_ADD_IO') and \
>                      conf.CONFIG_SET('HAVE_LDAP_OPT_SOCKBUF'):
>                  conf.DEFINE('HAVE_LDAP_SASL_WRAPPING', '1')
> +        else:
> +            conf.fatal("LDAP support not found. Try installing libldap2-dev or openldap-devel. Otherwise, use --without-ldap to build without LDAP support. " \
> +                "LDAP support is required for the LDAP passdb backend, LDAP idmap backends and ADS. " \
> +                "ADS support improves communication with Active Directory domain controllers.")

Please limit lines at 80 characters (especially since you're already breaking this string anyway).

It's not necessary to use a backslash when continuing strings here, since you're already inside parentheses.

Otherwise, this looks good. 

Cheers,

Jelmer