[PATCH] Make AD DC build require Jannsson JSON libs, fix fileserver without it
Andrew Bartlett
abartlet at samba.org
Sun Jun 24 18:14:38 UTC 2018
On Sat, 2018-06-23 at 16:15 +0200, Ralph Böhme wrote:
> On Sat, Jun 23, 2018 at 10:05:00AM +1200, Andrew Bartlett wrote:
> > On Fri, 2018-06-22 at 13:57 -0700, Jeremy Allison wrote:
> > I spent around three hours yesterday building the patch attached, which
> > I could have spent on other things if we had just accepted it as a
> > build requirement overall.
>
> ENOPATCH?
The patch was at the top of the thread, attached here for your
enjoyment.
> > - setting up autobuild targets without that feature
> > - wrangling WAF to still link without this feature
> > - fixing the fallback code used in the fileserver
> > - ensuring it still passes tests
>
> My understanding was that libjansson ought to be an optional feature, that is
> also what is documented here:
>
> <https://wiki.samba.org/index.php/Package_Dependencies_Required_to_Build_Samba>
>
> Why did this change? Was this ever discussed? So please calm down, I'm just
> asking if it would be possible. If that causes a big maintanance burden and
> other difficulties then by all means, we can add it as a dependency.
>
> There are already a lot "#ifdef HAVE_JANSSON" all over the place, so code wise
> it just looks like it could be optional and it's just missing a few guards.
This is exactly it. Perhaps you and I will have different weights for
'a big maintenance burden', and you will see I certainly understand the
AD and file-server use cases differ, but please bear with me:
As you know, over recent years I've become a real sticker for a few
concepts (mostly though being personally burnt by them):
- Untested code is broken code
- What is not in autobuild is not tested code
- Optional features in 'make test' become silently missing tests
I can elaborate on all of these if you need, but I'll keep this short
for now.
However the natural conclusion for me of this is that, particularly for
the AD DC, we should have as few './configure --with{,out}-XXX'
optional features as possible and what optional features we have
autobuild/--enable-selftest should require.
Therefore when Volker raised the issue that a said-to-be-supported
build of the AD DC not building without JSON, I looked not to the
missing #ifdef, but to why our systems didn't find it, and how we could
ensure then next bug wasn't just 'fails at runtime'. That is, the
simple extra compile job Jeremy suggested wasn't enough in my view.
In doing that I made my proposal, not previously discussed, to lock in
the (new) status quo for the AD DC (Jannsson required), but to relax
the requirement for the fileserver (make Jannsson optional for --
without-ad-dc even with --enable-selftest).
This isn't actually ideal, as by virtue of the layering there are JSON
logs emitted, if so built, in the fileserver, but that code is only
tested in the AD DC use case.
However it does remove untested (currently broken) build combinations
and does ensure that a --without-json-audit Samba fileserver is built
and run in autobuild.
I hope this clarifies things,
Andrew Bartlett
CI: https://gitlab.com/catalyst-samba/samba/pipelines/24328549
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba
-------------- next part --------------
From 0ce73318378aa655f33b36856faf4f3abb64bcca Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Fri, 22 Jun 2018 04:47:10 +1200
Subject: [PATCH 1/5] build: Move --without-json-audit and json lib detection
to lib/audit_logging
This is the common location of the audit logging code now
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
{auth => lib/audit_logging}/wscript | 1 -
wscript | 5 ++---
2 files changed, 2 insertions(+), 4 deletions(-)
rename {auth => lib/audit_logging}/wscript (99%)
diff --git a/auth/wscript b/lib/audit_logging/wscript
similarity index 99%
rename from auth/wscript
rename to lib/audit_logging/wscript
index 7b2c65effe0..7818ed0ecfa 100644
--- a/auth/wscript
+++ b/lib/audit_logging/wscript
@@ -32,4 +32,3 @@ def configure(conf):
raise Utils.WafError('jansson JSON library required for '
'--enable-selftest when building the AD DC')
Logs.info("Building without jansson json log support")
-
diff --git a/wscript b/wscript
index dbfc1210f7f..b1b69c15a20 100644
--- a/wscript
+++ b/wscript
@@ -35,7 +35,7 @@ def system_mitkrb5_callback(option, opt, value, parser):
def set_options(opt):
opt.BUILTIN_DEFAULT('NONE')
opt.PRIVATE_EXTENSION_DEFAULT('samba4')
- opt.RECURSE('auth')
+ opt.RECURSE('lib/audit_logging')
opt.RECURSE('lib/replace')
opt.RECURSE('dynconfig')
opt.RECURSE('packaging')
@@ -213,7 +213,7 @@ def configure(conf):
# system-provided or embedded Heimdal build
if conf.CONFIG_GET('KRB5_VENDOR') in (None, 'heimdal'):
conf.RECURSE('source4/heimdal_build')
- conf.RECURSE('auth')
+ conf.RECURSE('lib/audit_logging')
conf.RECURSE('source4/lib/tls')
conf.RECURSE('source4/dsdb/samdb/ldb_modules')
conf.RECURSE('source4/ntvfs/sysdep')
@@ -251,7 +251,6 @@ def configure(conf):
if conf.env.with_ctdb:
conf.RECURSE('ctdb')
conf.RECURSE('lib/socket')
- conf.RECURSE('auth')
conf.RECURSE('packaging')
conf.SAMBA_CHECK_UNDEFINED_SYMBOL_FLAGS()
--
2.14.4
From 5e6edb06e308cfe4788753ea09b53796e818b14c Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Fri, 22 Jun 2018 04:50:09 +1200
Subject: [PATCH 2/5] lib/audit_logging: Require jansson JSON library for
building the AD DC
This combination is untested and it is reasonable to require this
broadly available library for the AD DC build.
Doing so keeps the combinational complexity down and ensures we test
what we ship. (It was failing to compile).
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
lib/audit_logging/wscript | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/lib/audit_logging/wscript b/lib/audit_logging/wscript
index 7818ed0ecfa..d40fc6f079f 100644
--- a/lib/audit_logging/wscript
+++ b/lib/audit_logging/wscript
@@ -21,14 +21,16 @@ def configure(conf):
if not conf.CONFIG_GET('HAVE_JSON_OBJECT'):
if Options.options.with_json_audit != False:
- conf.fatal("JSON support not found. "
+ conf.fatal("Jansson JSON support not found. "
"Try installing libjansson-dev or jansson-devel. "
"Otherwise, use --without-json-audit to build without "
"JSON support. "
"JSON support is required for the JSON "
- "formatted audit log feature")
- if conf.CONFIG_GET('ENABLE_SELFTEST') and \
- (not Options.options.without_ad_dc):
- raise Utils.WafError('jansson JSON library required for '
- '--enable-selftest when building the AD DC')
- Logs.info("Building without jansson json log support")
+ "formatted audit log feature and the AD DC")
+ if not Options.options.without_ad_dc:
+ raise Utils.WafError('--without-json-audit requires '
+ '--without-ad-dc. '
+ 'Jansson JSON library is required for '
+ 'building the AD DC')
+ Logs.info("Building without Jansson JSON log support")
+
--
2.14.4
From fd58ecd5c0a8ddbc9d22baf502a2759d2443cebb Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Fri, 22 Jun 2018 05:39:08 +1200
Subject: [PATCH 3/5] lib/audit_logging: Only build audit_logging_test for
--enable-selftest on the AD DC
This allows a --without-ad-dc --enable-selftest build to compile, still testing some
fileserver-only features.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
lib/audit_logging/wscript_build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/audit_logging/wscript_build b/lib/audit_logging/wscript_build
index fff683788d6..24ac8bb4789 100644
--- a/lib/audit_logging/wscript_build
+++ b/lib/audit_logging/wscript_build
@@ -9,7 +9,7 @@ bld.SAMBA_SUBSYSTEM(
source='audit_logging.c'
)
-if bld.CONFIG_SET('ENABLE_SELFTEST'):
+if bld.AD_DC_BUILD_IS_ENABLED() and bld.CONFIG_GET('ENABLE_SELFTEST'):
bld.SAMBA_BINARY(
'audit_logging_test',
source='tests/audit_logging_test.c',
--
2.14.4
From 6cb8208c1868e73658888d1baf38534c83da41fb Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Fri, 22 Jun 2018 05:18:52 +1200
Subject: [PATCH 4/5] dsdb: Ensure a build --without-json-audit --without-ad-dc
compiles
We still build some of the ldb_modules even when we are not a DC, so we must
split up the DSDB_MODULE_HELPERS.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
source4/dsdb/samdb/ldb_modules/audit_log.c | 1 +
source4/dsdb/samdb/ldb_modules/audit_util.c | 1 +
source4/dsdb/samdb/ldb_modules/group_audit.c | 1 +
source4/dsdb/samdb/ldb_modules/wscript_build | 46 +-----------------
.../dsdb/samdb/ldb_modules/wscript_build_server | 55 +++++++++++++++++++++-
5 files changed, 58 insertions(+), 46 deletions(-)
diff --git a/source4/dsdb/samdb/ldb_modules/audit_log.c b/source4/dsdb/samdb/ldb_modules/audit_log.c
index 80914cb8350..fc2eb502361 100644
--- a/source4/dsdb/samdb/ldb_modules/audit_log.c
+++ b/source4/dsdb/samdb/ldb_modules/audit_log.c
@@ -29,6 +29,7 @@
#include "dsdb/samdb/samdb.h"
#include "dsdb/samdb/ldb_modules/util.h"
+#include "dsdb/samdb/ldb_modules/audit_util_proto.h"
#include "libcli/security/dom_sid.h"
#include "auth/common_auth.h"
#include "param/param.h"
diff --git a/source4/dsdb/samdb/ldb_modules/audit_util.c b/source4/dsdb/samdb/ldb_modules/audit_util.c
index be2c522ace7..766c34c1e23 100644
--- a/source4/dsdb/samdb/ldb_modules/audit_util.c
+++ b/source4/dsdb/samdb/ldb_modules/audit_util.c
@@ -33,6 +33,7 @@
#include "auth/common_auth.h"
#include "param/param.h"
#include "dsdb/samdb/ldb_modules/util.h"
+#include "dsdb/samdb/ldb_modules/audit_util_proto.h"
#define MAX_LENGTH 1024
diff --git a/source4/dsdb/samdb/ldb_modules/group_audit.c b/source4/dsdb/samdb/ldb_modules/group_audit.c
index dc586777736..bbd124af156 100644
--- a/source4/dsdb/samdb/ldb_modules/group_audit.c
+++ b/source4/dsdb/samdb/ldb_modules/group_audit.c
@@ -28,6 +28,7 @@
#include "dsdb/samdb/samdb.h"
#include "dsdb/samdb/ldb_modules/util.h"
+#include "dsdb/samdb/ldb_modules/audit_util_proto.h"
#include "libcli/security/dom_sid.h"
#include "auth/common_auth.h"
#include "param/param.h"
diff --git a/source4/dsdb/samdb/ldb_modules/wscript_build b/source4/dsdb/samdb/ldb_modules/wscript_build
index 1216a1fd99f..9e0ac281cc6 100644
--- a/source4/dsdb/samdb/ldb_modules/wscript_build
+++ b/source4/dsdb/samdb/ldb_modules/wscript_build
@@ -7,9 +7,9 @@ bld.SAMBA_LIBRARY('dsdb-module',
grouping_library=True)
bld.SAMBA_SUBSYSTEM('DSDB_MODULE_HELPERS',
- source='util.c acl_util.c schema_util.c netlogon.c audit_util.c',
+ source='util.c acl_util.c schema_util.c netlogon.c',
autoproto='util_proto.h',
- deps='ldb ndr samdb-common samba-security audit_logging'
+ deps='ldb ndr samdb-common samba-security'
)
bld.SAMBA_SUBSYSTEM('DSDB_MODULE_HELPER_RIDALLOC',
@@ -41,47 +41,5 @@ bld.SAMBA_BINARY('test_encrypted_secrets',
''',
install=False)
-#
-# These tests require JANSSON, so we only build them if we are doing a selftest
-# build.
-#
-if bld.CONFIG_GET('ENABLE_SELFTEST'):
- bld.SAMBA_BINARY('test_audit_util',
- source='tests/test_audit_util.c',
- deps='''
- talloc
- samba-util
- samdb-common
- samdb
- cmocka
- audit_logging
- DSDB_MODULE_HELPERS
- ''',
- install=False)
- bld.SAMBA_BINARY('test_audit_log',
- source='tests/test_audit_log.c',
- deps='''
- talloc
- samba-util
- samdb-common
- samdb
- cmocka
- audit_logging
- DSDB_MODULE_HELPERS
- ''',
- install=False)
- bld.SAMBA_BINARY('test_group_audit',
- source='tests/test_group_audit.c',
- deps='''
- talloc
- samba-util
- samdb-common
- samdb
- cmocka
- audit_logging
- DSDB_MODULE_HELPERS
- ''',
- install=False)
-
if bld.AD_DC_BUILD_IS_ENABLED():
bld.PROCESS_SEPARATE_RULE("server")
diff --git a/source4/dsdb/samdb/ldb_modules/wscript_build_server b/source4/dsdb/samdb/ldb_modules/wscript_build_server
index e5c503239df..39c9477db80 100644
--- a/source4/dsdb/samdb/ldb_modules/wscript_build_server
+++ b/source4/dsdb/samdb/ldb_modules/wscript_build_server
@@ -1,5 +1,56 @@
#!/usr/bin/env python
+bld.SAMBA_SUBSYSTEM('DSDB_MODULE_HELPERS_AUDIT',
+ source='audit_util.c',
+ autoproto='audit_util_proto.h',
+ deps='DSDB_MODULE_HELPERS audit_logging')
+
+#
+# These tests require JANSSON, so we only build them if we are doing a
+# build on the AD DC (where Jansson is required).
+#
+
+bld.SAMBA_BINARY('test_audit_util',
+ source='tests/test_audit_util.c',
+ deps='''
+ talloc
+ samba-util
+ samdb-common
+ samdb
+ cmocka
+ audit_logging
+ DSDB_MODULE_HELPERS
+ ''',
+ install=False)
+
+bld.SAMBA_BINARY('test_audit_log',
+ source='tests/test_audit_log.c',
+ deps='''
+ talloc
+ samba-util
+ samdb-common
+ samdb
+ cmocka
+ audit_logging
+ DSDB_MODULE_HELPERS
+ DSDB_MODULE_HELPERS_AUDIT
+ ''',
+ install=False)
+
+bld.SAMBA_BINARY('test_group_audit',
+ source='tests/test_group_audit.c',
+ deps='''
+ talloc
+ samba-util
+ samdb-common
+ samdb
+ cmocka
+ audit_logging
+ DSDB_MODULE_HELPERS
+ DSDB_MODULE_HELPERS_AUDIT
+ ''',
+ install=False)
+
bld.SAMBA_MODULE('ldb_samba_dsdb',
source='samba_dsdb.c',
subsystem='ldb',
@@ -437,7 +488,7 @@ bld.SAMBA_MODULE('ldb_audit_log',
talloc
samba-util
samdb-common
- DSDB_MODULE_HELPERS
+ DSDB_MODULE_HELPERS_AUDIT
samdb
'''
)
@@ -453,7 +504,7 @@ bld.SAMBA_MODULE('ldb_group_audit_log',
talloc
samba-util
samdb-common
- DSDB_MODULE_HELPERS
+ DSDB_MODULE_HELPERS_AUDIT
samdb
'''
)
--
2.14.4
From 65bee7689ab67cd7d03260cc7b75b2ad7901d08f Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Fri, 22 Jun 2018 05:32:29 +1200
Subject: [PATCH 5/5] autobuild: Build samba-fileserver --without-json-audit
This build target is already --without-ad-dc and is the one we need to ensure is
compatible with a host without the Jansson JSON library.
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
---
script/autobuild.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/script/autobuild.py b/script/autobuild.py
index 429d6443792..a13b731b008 100755
--- a/script/autobuild.py
+++ b/script/autobuild.py
@@ -131,7 +131,7 @@ tasks = {
# We split out this so the isolated ad_dc tests do not wait for ad_dc_ntvfs tests (which are long)
"samba-fileserver" : [ ("random-sleep", "../script/random-sleep.sh 60 600", "text/plain"),
- ("configure", "./configure.developer --without-ad-dc --without-ldap --without-ads --with-selftest-prefix=./bin/ab" + samba_configure_params, "text/plain"),
+ ("configure", "./configure.developer --without-ad-dc --without-ldap --without-ads --without-json-audit --with-selftest-prefix=./bin/ab" + samba_configure_params, "text/plain"),
("make", "make -j", "text/plain"),
("test", "make test FAIL_IMMEDIATELY=1 TESTS='--include-env=fileserver'", "text/plain"),
("check-clean-tree", "script/clean-source-tree.sh", "text/plain")],
--
2.14.4
More information about the samba-technical
mailing list