[ROADMAP] Catalyst's focus on Samba

classic Classic list List threaded Threaded
44 messages Options
123
Reply | Threaded
Open this post in threaded view
|

lmdb dn2id code (was: Re: [ROADMAP] Catalyst's focus on Samba)

Samba - samba-technical mailing list
On Tue, 2017-03-21 at 12:44 +0100, Arvid Requate via samba-technical
wrote:

> Hi Andrew & Metze,
>
> Am Dienstag, 21. März 2017, 11:49:21 schrieb Stefan Metzmacher via
> samba-
> technical:
> >
> Please also note that increasing the number of named databases in
> LMDB comes 
> at a price for startup, see mdb_env_set_maxdbs in the API:
>
> http://www.lmdb.tech/doc/group__mdb.html#gaa2fc2f1f37cb1115e733b62cab
> 2fcdbc
>
> OpenLDAP back-mdb currently limits this to 128 at compile time. You
> could 
> probably change that limit at runtime too though, when opening the 
> environment.
>
> The other limit too look out for are the 511 byte default key size
> limit. See 
> dn2id of back-mdb for a way around that.

Thanks for the heads-up on that.

> I've isolated and re-implemented that 
> algorithm here: https://github.com/reqa/ldap-lmdb-dntree

I don't know if that is any use, but AGPLv3 is really awkward for a
licence on that code.  I'm curious why did you re-licence it away from
the OpenLDAP licence for a toy demo?

I any case, LDB needs to be LGPLv3 or later.  I don't expect we will
want to re-use that code, but you never know and I wanted to give you a
heads-up about the conflict.

Thanks,

Andrew Bartlett

--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba





Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, 2017-04-07 at 14:49 +1200, Andrew Bartlett wrote:

> On Fri, 2017-04-07 at 09:39 +1200, Andrew Bartlett via samba-
> technical
> wrote:
> > On Thu, 2017-04-06 at 17:04 +1200, Andrew Bartlett via samba-
> > technical
> > wrote:
> > >
> > > I thought I was using that, I tried to install a copy from
> > > source. 
> > >
> > > As correct cmocka versions are not yet widely installed in
> > > distributions, would it make sense to put it in third_party?  It
> > > would
> > > be very helpful if the right version was in the tree to test
> > > with.  
> > >
> > > I don't like us having to bundle a lot of software, but if we
> > > want
> > > to
> > > write tests in a framework, we should have the framework in our
> > > tree
> > > or
> > > available on all of current debian/ubuntu LTS/fedora.
> >
> > I had tried to install cmokca from Debian Unstable, but that isn't
> > recent enough.  However that was enough to break the test, as it
> > doesn't link strongly against the version in /usr/local/lib.  I had
> > to
> > use LD_LIBRARY_PATH to override it. 
> >
> > Andrew Bartlett
>
> Can you look at this cmocka test for me?  I've been writing one to
> show
> the ldb_tdb locking bug in the other thread.  I like cmocka!
>
> I'm not sure what the correct interaction with fork() is meant to be,
> but I've made this work for now.  
>
> I've also added another API test in python, trying to show the same
> issue, but python isn't called back at the right points, so I went to
> C.  
>
> Please review/comment!
>
> Once we sort out how to ensure we always have cmocka, it will be
> great
> to get this all into the tree!
>
> Thanks,
>
> Andrew Bartlett
Now with actual patches :-)

Andrew Bartlett

--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba




0001-ldb-Add-some-tests-to-clarify-the-current-iterator-b.patch (5K) Download Attachment
0002-ldb-Add-test-for-transaction-deadlock-detected-when-.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
On Fri, Apr 07, 2017 at 03:18:02PM +1200, Andrew Bartlett via samba-technical wrote:
> Now with actual patches :-)

Recently I've started to name patches with .txt extension. This makes
mailman show them inline. I find that very helpful :-)

Just my 2ct...

Volker

Reply | Threaded
Open this post in threaded view
|

Re: ldb cmocka tests

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Thursday, 6 April 2017 23:39:44 CEST Andrew Bartlett wrote:

> On Thu, 2017-04-06 at 17:04 +1200, Andrew Bartlett via samba-technical
>
> wrote:
> > On Tue, 2017-04-04 at 17:55 +0200, Andreas Schneider wrote:
> > > On Monday, 3 April 2017 04:57:23 CEST Andrew Bartlett wrote:
> > > > Some more review!
> > >
> > > Thanks for the review.
> > >
> > > > In ldb:tests: Build a ldb test for the tdb backend
> > > >
> > > > Can you clarify this hunk:
> > > >
> > > > +    conf.SET_TARGET_TYPE('cmocka', 'EMPTY')
> > > > +
> > > > +    conf.env.found_cmocka = False
> > > > +    if conf.CHECK_CFG(package='cmocka',
> > > > +                      args='"cmocka >= 1.1.0" --cflags --libs',
> > > > +                      msg='Checking for cmocka >= 1.1.0'):
> > > > +        conf.CHECK_LIB('cmocka', shlib=True)
> > > > +        #conf.CHECK_FUNCS_IN('_cmocka_run_group_tests',
> > > > 'cmocka')
> > > > +        conf.env.found_cmocka = True
> > > > +
> > > >
> > > > I'm presuming we should drop this line:
> > > > #conf.CHECK_FUNCS_IN('_cmocka_run_group_tests', 'cmocka')
> > >
> > > I removed that.
> > >
> > > > In "ldb:tests: Add a test for ldb transactions" I hate to have to
> > > > comment on whitespace given recent discussions, but this isn't
> > > > using
> > > > our standard 8-space tabs, and (even more importantly) isn't in
> > > > line
> > > > with the rest of the file.
> > >
> > > Fixed.
> > >
> > > > In "ldb:tests: unit test for ldb_search()" it really should check
> > > > both
> > > > with and without the checkBaseOnSearch attribute on @OPTIONS, as
> > > > this
> > > > changes the behaviour quite a bit.
> > >
> > > I hope I implemented that correctly with Jakub. There is a new
> > >
> > > test:
> > > test_search_match_basedn
> > >
> > > > In "ldb:tests: Unit test the ldb_rename() operation" I agree with
> > > > the
> > > > FIXME, the rename with no change case really should be checked.
> > >
> > > ...
> > >
> > > > In "ldb:tests: Print subunit output", should we include this in
> > > > the
> > > > Samba test as well?  Otherwise, I'm not sure what the point of
> > > > the
> > > > subunit output is, as ldb is currently only using the return
> > > > value.
> > >
> > > I removed it. If autobuild runs this testsuite the standard cmocka
> > > test output
> > > is a bit nicer.
> > >
> > > > In any case, the tests fail with:
> > > > Python testsuite returned 0
> > > > test: test_connect
> > > > success: test_connect
> > > > test: (null)
> > > > Aborted
> > > > Makefile:15: recipe for target 'test' failed
> > > > make: *** [test] Error 134
> > > >
> > > > for me.
> > >
> > > It works just fine for me. You can also just run:
> > >
> > > ./bin/ldb_tdb_mod_op_test
> > >
> > > gdb ./bin/ldb_tdb_mod_op_test
> > >
> > > if it aborts to get a backtrace. You should use cmocka 1.1.0.
> >
> > I thought I was using that, I tried to install a copy from source.
> >
> > As correct cmocka versions are not yet widely installed in
> > distributions, would it make sense to put it in third_party?  It
> > would
> > be very helpful if the right version was in the tree to test with.  
> >
> > I don't like us having to bundle a lot of software, but if we want to
> > write tests in a framework, we should have the framework in our tree
> > or
> > available on all of current debian/ubuntu LTS/fedora.
>
> I had tried to install cmokca from Debian Unstable, but that isn't
> recent enough.  However that was enough to break the test, as it
> doesn't link strongly against the version in /usr/local/lib.  I had to
> use LD_LIBRARY_PATH to override it.

This sounds like a waf issue, or you need to point pkg-config to the right
location to find the newer version first.

Bundling will work for the cmocka tests we have in testsuite/unittest but I
don't think it will work for a ldb build!


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Friday, 7 April 2017 05:18:02 CEST Andrew Bartlett wrote:
> > Can you look at this cmocka test for me?  I've been writing one to
> > show
> > the ldb_tdb locking bug in the other thread.  I like cmocka!
> >
> > I'm not sure what the correct interaction with fork() is meant to be,
> > but I've made this work for now.  

The test looks fine. I think the original idea of the file is to test the API.

This test you wrote is a special case. I would put that in its own binary.

The setup/teardown functions could be shared.

> > I've also added another API test in python, trying to show the same
> > issue, but python isn't called back at the right points, so I went to
> > C.
> >
> > Please review/comment!
> >
> > Once we sort out how to ensure we always have cmocka, it will be
> > great
> > to get this all into the tree!

Well, we can't/shouldn't put cmocka in ldb. If we put it in third_party it is
available in a Samba build but not in a standalone ldb build!


Cheers,


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: ldb cmocka tests

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, 2017-04-07 at 11:01 +0200, Andreas Schneider wrote:

> On Thursday, 6 April 2017 23:39:44 CEST Andrew Bartlett wrote:
> > On Thu, 2017-04-06 at 17:04 +1200, Andrew Bartlett via samba-
> > technical
> >
> > wrote:
> > >
> > > I thought I was using that, I tried to install a copy from
> > > source. 
> > >
> > > As correct cmocka versions are not yet widely installed in
> > > distributions, would it make sense to put it in third_party?  It
> > > would
> > > be very helpful if the right version was in the tree to test
> > > with.  
> > >
> > > I don't like us having to bundle a lot of software, but if we
> > > want to
> > > write tests in a framework, we should have the framework in our
> > > tree
> > > or
> > > available on all of current debian/ubuntu LTS/fedora.
> >
> > I had tried to install cmokca from Debian Unstable, but that isn't
> > recent enough.  However that was enough to break the test, as it
> > doesn't link strongly against the version in /usr/local/lib.  I had
> > to
> > use LD_LIBRARY_PATH to override it.
>
> This sounds like a waf issue, or you need to point pkg-config to the
> right 
> location to find the newer version first.

I think it is just standard linker rules, as both are 'system' paths.
However I notice that cmocka uses -L but not -rpath:

Name: cmocka
Description: The cmocka unit testing library
Version: 1.1.0
Libs: -L/usr/local/lib -lcmocka
Cflags: -I/usr/local/include

Particularly for non-system paths, you might want to look at this for
what we do for ndr.pc when installed in a custom prefix:

prefix=/data/samba/samba4/prefix
exec_prefix=${prefix}
libdir=${prefix}/lib
includedir=${prefix}/include

Name: ndr
Description: Network Data Representation Core Library
Requires: samba-util talloc
Version: 0.0.8
Libs: -Wl,-rpath,/data/samba/samba4/prefix/lib -L${libdir} -lndr
Cflags: -I${includedir}  -DHAVE_IMMEDIATE_STRUCTURES=1 -D_GNU_SOURCE=1

I hope this helps.

> Bundling will work for the cmocka tests we have in testsuite/unittest
> but I 
> don't think it will work for a ldb build!

I'm not quite sure what you mean, how would it be any different to tdb?

Thanks,

Andrew Bartlett
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, 2017-04-07 at 11:11 +0200, Andreas Schneider wrote:

> On Friday, 7 April 2017 05:18:02 CEST Andrew Bartlett wrote:
> > > Can you look at this cmocka test for me?  I've been writing one
> > > to
> > > show
> > > the ldb_tdb locking bug in the other thread.  I like cmocka!
> > >
> > > I'm not sure what the correct interaction with fork() is meant to
> > > be,
> > > but I've made this work for now.  
>
> The test looks fine. I think the original idea of the file is to test
> the API.
>
> This test you wrote is a special case. I would put that in its own
> binary.
>
> The setup/teardown functions could be shared. 
>
> > > I've also added another API test in python, trying to show the
> > > same
> > > issue, but python isn't called back at the right points, so I
> > > went to
> > > C.
> > >
> > > Please review/comment!
> > >
> > > Once we sort out how to ensure we always have cmocka, it will be
> > > great
> > > to get this all into the tree!
>
> Well, we can't/shouldn't put cmocka in ldb. If we put it in
> third_party it is 
> available in a Samba build but not in a standalone ldb build!

I don't see why it would be any different to popt.

See this in wscript:


samba_dist.DIST_DIRS('''lib/ldb:. lib/replace:lib/replace
lib/talloc:lib/talloc
                        lib/tdb:lib/tdb lib/tdb:lib/tdb
lib/tevent:lib/tevent
                        third_party/popt:third_party/popt
                        buildtools:buildtools
third_party/waf:third_party/waf''')

Thanks,

Andrew Bartlett
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: ldb cmocka tests

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Friday, 7 April 2017 11:42:20 CEST Andrew Bartlett wrote:

> On Fri, 2017-04-07 at 11:01 +0200, Andreas Schneider wrote:
> > On Thursday, 6 April 2017 23:39:44 CEST Andrew Bartlett wrote:
> > > On Thu, 2017-04-06 at 17:04 +1200, Andrew Bartlett via samba-
> > > technical
> > >
> > > wrote:
> > > > I thought I was using that, I tried to install a copy from
> > > > source.
> > > >
> > > > As correct cmocka versions are not yet widely installed in
> > > > distributions, would it make sense to put it in third_party?  It
> > > > would
> > > > be very helpful if the right version was in the tree to test
> > > > with.  
> > > >
> > > > I don't like us having to bundle a lot of software, but if we
> > > > want to
> > > > write tests in a framework, we should have the framework in our
> > > > tree
> > > > or
> > > > available on all of current debian/ubuntu LTS/fedora.
> > >
> > > I had tried to install cmokca from Debian Unstable, but that isn't
> > > recent enough.  However that was enough to break the test, as it
> > > doesn't link strongly against the version in /usr/local/lib.  I had
> > > to
> > > use LD_LIBRARY_PATH to override it.
> >
> > This sounds like a waf issue, or you need to point pkg-config to the
> > right
> > location to find the newer version first.
>
> I think it is just standard linker rules, as both are 'system' paths.
> However I notice that cmocka uses -L but not -rpath:
>
> Name: cmocka
> Description: The cmocka unit testing library
> Version: 1.1.0
> Libs: -L/usr/local/lib -lcmocka
> Cflags: -I/usr/local/include
>
> Particularly for non-system paths, you might want to look at this for
> what we do for ndr.pc when installed in a custom prefix:
>
> prefix=/data/samba/samba4/prefix
> exec_prefix=${prefix}
> libdir=${prefix}/lib
> includedir=${prefix}/include
>
> Name: ndr
> Description: Network Data Representation Core Library
> Requires: samba-util talloc
> Version: 0.0.8
> Libs: -Wl,-rpath,/data/samba/samba4/prefix/lib -L${libdir} -lndr
> Cflags: -I${includedir}  -DHAVE_IMMEDIATE_STRUCTURES=1 -D_GNU_SOURCE=1
>
> I hope this helps.
>
> > Bundling will work for the cmocka tests we have in testsuite/unittest
> > but I
> > don't think it will work for a ldb build!
>
> I'm not quite sure what you mean, how would it be any different to tdb?

I will look into adding it to third_party.

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, 2017-04-07 at 11:11 +0200, Andreas Schneider wrote:

> On Friday, 7 April 2017 05:18:02 CEST Andrew Bartlett wrote:
> > > Can you look at this cmocka test for me?  I've been writing one
> > > to
> > > show
> > > the ldb_tdb locking bug in the other thread.  I like cmocka!
> > >
> > > I'm not sure what the correct interaction with fork() is meant to
> > > be,
> > > but I've made this work for now.  
>
> The test looks fine. I think the original idea of the file is to test
> the API.
>
> This test you wrote is a special case. I would put that in its own
> binary.
>
> The setup/teardown functions could be shared. 

Currently we don't have a good framework for multiple tests in ldb.  To
split it out we need to create that, with a set of test names and a way
to ensure we run them all.

I think we both want to get the concept of cmocka in for now, can we
leave this for the next large test expansion?  

Otherwise, could you show how you would like it split up by splitting
the existing test up, so I can just follow the same pattern?

> > > I've also added another API test in python, trying to show the
> > > same
> > > issue, but python isn't called back at the right points, so I
> > > went to
> > > C.
> > >
> > > Please review/comment!
> > >
> > > Once we sort out how to ensure we always have cmocka, it will be
> > > great
> > > to get this all into the tree!
>
> Well, we can't/shouldn't put cmocka in ldb. If we put it in
> third_party it is 
> available in a Samba build but not in a standalone ldb build!

Thanks for proposing it for third_party.  I've reviewed that.

Andrew Bartlett

--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba





Reply | Threaded
Open this post in threaded view
|

Re: lmdb dn2id code (was: Re: [ROADMAP] Catalyst's focus on Samba)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, 7. April 2017, 14:54:08 Andrew Bartlett wrote:

> On Tue, 2017-03-21 at 12:44 +0100, Arvid Requate via samba-technical
>
> wrote:
> > Hi Andrew & Metze,
> >
> > Am Dienstag, 21. März 2017, 11:49:21 schrieb Stefan Metzmacher via
> > samba-
> > technical:
> >
> > Please also note that increasing the number of named databases in
> > LMDB comes
> > at a price for startup, see mdb_env_set_maxdbs in the API:
> >
> > http://www.lmdb.tech/doc/group__mdb.html#gaa2fc2f1f37cb1115e733b62cab
> > 2fcdbc
> >
> > OpenLDAP back-mdb currently limits this to 128 at compile time. You
> > could
> > probably change that limit at runtime too though, when opening the
> > environment.
> >
> > The other limit too look out for are the 511 byte default key size
> > limit. See
> > dn2id of back-mdb for a way around that.
>
> Thanks for the heads-up on that.
>
> > I've isolated and re-implemented that
> > algorithm here: https://github.com/reqa/ldap-lmdb-dntree
>
> I don't know if that is any use, but AGPLv3 is really awkward for a
> licence on that code.  I'm curious why did you re-licence it away from
> the OpenLDAP licence for a toy demo?

I reimplemented the code for an internal cache used in UCS and just took the
license boilerplate from UCS. Thanks for poking me about that, I've changed it
to be OpenLDAP.

> I any case, LDB needs to be LGPLv3 or later.  I don't expect we will
> want to re-use that code, but you never know and I wanted to give you a
> heads-up about the conflict.

Ah, ok, good to know.

Cheers,
Arvid


--
Arvid Requate
Open Source Software Engineer

Univention GmbH
be open.
Mary-Somerville-Str.1
28359 Bremen
Tel. : +49 421 22232-52
Fax : +49 421 22232-99

Geschäftsführer: Peter H. Ganten
HRB 20755 Amtsgericht Bremen
Steuer-Nr.: 71-597-02876

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Monday, 10 April 2017 00:26:06 CEST Andrew Bartlett wrote:

> On Fri, 2017-04-07 at 11:11 +0200, Andreas Schneider wrote:
> > On Friday, 7 April 2017 05:18:02 CEST Andrew Bartlett wrote:
> > > > Can you look at this cmocka test for me?  I've been writing one
> > > > to
> > > > show
> > > > the ldb_tdb locking bug in the other thread.  I like cmocka!
> > > >
> > > > I'm not sure what the correct interaction with fork() is meant to
> > > > be,
> > > > but I've made this work for now.  
> >
> > The test looks fine. I think the original idea of the file is to test
> > the API.
> >
> > This test you wrote is a special case. I would put that in its own
> > binary.
> >
> > The setup/teardown functions could be shared.
>
> Currently we don't have a good framework for multiple tests in ldb.  To
> split it out we need to create that, with a set of test names and a way
> to ensure we run them all.
>
> I think we both want to get the concept of cmocka in for now, can we
> leave this for the next large test expansion?  
>
> Otherwise, could you show how you would like it split up by splitting
> the existing test up, so I can just follow the same pattern?
Ok, lets first bring the patchset upstream.


Here is a rebased version on the third_party cmocka code which is upstream
now. After this is upstream we can look at your additions to the ldb tests.
Are you OK with that?


Cheers,


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

ldb_tests.patch.txt (70K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
On Mon, 2017-04-10 at 12:13 +0200, Andreas Schneider wrote:
>
> Ok, lets first bring the patchset upstream.

Sure.

> Here is a rebased version on the third_party cmocka code which is
> upstream 
> now. After this is upstream we can look at your additions to the ldb
> tests. 
> Are you OK with that?

That is fine.  I'll review and push this tomorrow once I'm at work.

Thanks!

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, 2017-04-10 at 12:13 +0200, Andreas Schneider wrote:

> On Monday, 10 April 2017 00:26:06 CEST Andrew Bartlett wrote:
> > On Fri, 2017-04-07 at 11:11 +0200, Andreas Schneider wrote:
> > > On Friday, 7 April 2017 05:18:02 CEST Andrew Bartlett wrote:
> > > > > Can you look at this cmocka test for me?  I've been writing
> > > > > one
> > > > > to
> > > > > show
> > > > > the ldb_tdb locking bug in the other thread.  I like cmocka!
> > > > >
> > > > > I'm not sure what the correct interaction with fork() is
> > > > > meant to
> > > > > be,
> > > > > but I've made this work for now.  
> > >
> > > The test looks fine. I think the original idea of the file is to
> > > test
> > > the API.
> > >
> > > This test you wrote is a special case. I would put that in its
> > > own
> > > binary.
> > >
> > > The setup/teardown functions could be shared. 
> >
> > Currently we don't have a good framework for multiple tests in
> > ldb.  To
> > split it out we need to create that, with a set of test names and a
> > way
> > to ensure we run them all.
> >
> > I think we both want to get the concept of cmocka in for now, can
> > we
> > leave this for the next large test expansion?  
> >
> > Otherwise, could you show how you would like it split up by
> > splitting
> > the existing test up, so I can just follow the same pattern?
>
> Ok, lets first bring the patchset upstream.
>
>
> Here is a rebased version on the third_party cmocka code which is
> upstream 
> now. After this is upstream we can look at your additions to the ldb
> tests. 
> Are you OK with that?

Sadly this still fails autobuild.  You we to change autobuild to allow
bundling of cmocka in samba-libs (which disables bundling, something
that was aimed at ldb/tdb/tevent), so we don't have to write 'install'
logic for it etc.

Andrew Bartlett

--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba





Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
On Wednesday, 12 April 2017 03:35:05 CEST Andrew Bartlett via samba-technical
wrote:

> On Mon, 2017-04-10 at 12:13 +0200, Andreas Schneider wrote:
> > On Monday, 10 April 2017 00:26:06 CEST Andrew Bartlett wrote:
> > > On Fri, 2017-04-07 at 11:11 +0200, Andreas Schneider wrote:
> > > > On Friday, 7 April 2017 05:18:02 CEST Andrew Bartlett wrote:
> > > > > > Can you look at this cmocka test for me?  I've been writing
> > > > > > one
> > > > > > to
> > > > > > show
> > > > > > the ldb_tdb locking bug in the other thread.  I like cmocka!
> > > > > >
> > > > > > I'm not sure what the correct interaction with fork() is
> > > > > > meant to
> > > > > > be,
> > > > > > but I've made this work for now.  
> > > >
> > > > The test looks fine. I think the original idea of the file is to
> > > > test
> > > > the API.
> > > >
> > > > This test you wrote is a special case. I would put that in its
> > > > own
> > > > binary.
> > > >
> > > > The setup/teardown functions could be shared.
> > >
> > > Currently we don't have a good framework for multiple tests in
> > > ldb.  To
> > > split it out we need to create that, with a set of test names and a
> > > way
> > > to ensure we run them all.
> > >
> > > I think we both want to get the concept of cmocka in for now, can
> > > we
> > > leave this for the next large test expansion?  
> > >
> > > Otherwise, could you show how you would like it split up by
> > > splitting
> > > the existing test up, so I can just follow the same pattern?
> >
> > Ok, lets first bring the patchset upstream.
> >
> >
> > Here is a rebased version on the third_party cmocka code which is
> > upstream
> > now. After this is upstream we can look at your additions to the ldb
> > tests.
> > Are you OK with that?
>
> Sadly this still fails autobuild.  You we to change autobuild to allow
> bundling of cmocka in samba-libs (which disables bundling, something
> that was aimed at ldb/tdb/tevent), so we don't have to write 'install'
> logic for it etc.

Now you know why I hate bundling! I don't know how to fix that yet ...


        Andreas


--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
On Thu, 2017-04-13 at 12:19 +0200, Andreas Schneider wrote:
> Now you know why I hate bundling! I don't know how to fix that yet

The attached seems to do it.  If you could review it, I'll push it
along with your changes once I get them past a private autobuild.

Hopefully we can find a way to get my cmocka test additions accepted as
well.

Thanks,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba

0001-autobuild-Do-not-require-cmocka-to-be-installed-for-.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
On Wednesday, 19 April 2017 12:49:54 CEST Andrew Bartlett wrote:
> On Thu, 2017-04-13 at 12:19 +0200, Andreas Schneider wrote:
> > Now you know why I hate bundling! I don't know how to fix that yet
>
> The attached seems to do it.  If you could review it, I'll push it
> along with your changes once I get them past a private autobuild.
>
> Hopefully we can find a way to get my cmocka test additions accepted as
> well.

RB+

I will look at your changes as soon as this landed in master :)


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
On Wed, 2017-04-19 at 13:01 +0200, Andreas Schneider wrote:

> On Wednesday, 19 April 2017 12:49:54 CEST Andrew Bartlett wrote:
> > On Thu, 2017-04-13 at 12:19 +0200, Andreas Schneider wrote:
> > > Now you know why I hate bundling! I don't know how to fix that
> > > yet
> >
> > The attached seems to do it.  If you could review it, I'll push it
> > along with your changes once I get them past a private autobuild.
> >
> > Hopefully we can find a way to get my cmocka test additions
> > accepted as
> > well.
>
> RB+
>
> I will look at your changes as soon as this landed in master :)

The autobuild ldb target fails with:

/home/ubuntu/autobuild/b25034/ldb/lib/ldb/ldb-1.1.29/wscript:38: error:
Could not read the file '/home/ubuntu/autobuild/b25034/ldb/lib/ldb/ldb-
1.1.29/third_party/cmocka/wscript'
distcheck failed with code 1
make: *** [distcheck] Error 1

You should be able to reproduce with a make distcheck, or at worst
./script/autobuild.py --testbase=/tmp ldb

Sorry,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
On Wednesday, 19 April 2017 14:01:12 CEST Andrew Bartlett wrote:

> On Wed, 2017-04-19 at 13:01 +0200, Andreas Schneider wrote:
> > On Wednesday, 19 April 2017 12:49:54 CEST Andrew Bartlett wrote:
> > > On Thu, 2017-04-13 at 12:19 +0200, Andreas Schneider wrote:
> > > > Now you know why I hate bundling! I don't know how to fix that
> > > > yet
> > >
> > > The attached seems to do it.  If you could review it, I'll push it
> > > along with your changes once I get them past a private autobuild.
> > >
> > > Hopefully we can find a way to get my cmocka test additions
> > > accepted as
> > > well.
> >
> > RB+
> >
> > I will look at your changes as soon as this landed in master :)
>
> The autobuild ldb target fails with:
>
> /home/ubuntu/autobuild/b25034/ldb/lib/ldb/ldb-1.1.29/wscript:38: error:
> Could not read the file '/home/ubuntu/autobuild/b25034/ldb/lib/ldb/ldb-
> 1.1.29/third_party/cmocka/wscript'
> distcheck failed with code 1
> make: *** [distcheck] Error 1
>
> You should be able to reproduce with a make distcheck, or at worst
> ./script/autobuild.py --testbase=/tmp ldb
>
> Sorry,
>
> Andrew Bartlett

I think you have the patchset which missed the one line which copies the
cmocka directory.

https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/master-ldb


I've pushed that to autobuild as it works here and the directory is copied for
distcheck correctly.



        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Friday, 7 April 2017 05:18:02 CEST Andrew Bartlett via samba-technical
wrote:
> Now with actual patches :-)

Autobuild fails with:

ERR: Invalid attribute syntax : "Invalid attribute value in an @ATTRIBUTES
entry" on DN @ATTRIBUTES at block before line 3
  File "tests/python/api.py", line 1325
    except LdbError, err:
                   ^
SyntaxError: invalid syntax


Can you please fix it, thanks!


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Re: ldb cmocka tests

Samba - samba-technical mailing list
On Thu, 2017-04-20 at 14:32 +0200, Andreas Schneider wrote:

> On Friday, 7 April 2017 05:18:02 CEST Andrew Bartlett via samba-
> technical 
> wrote:
> > Now with actual patches :-)
>
> Autobuild fails with:
>
> ERR: Invalid attribute syntax : "Invalid attribute value in an
> @ATTRIBUTES 
> entry" on DN @ATTRIBUTES at block before line 3
>   File "tests/python/api.py", line 1325
>     except LdbError, err:
>                    ^
> SyntaxError: invalid syntax
>
>
> Can you please fix it, thanks!
Thanks.  It was not python3 compatible.  I'm really glad the autobuild
caught it, as I hadn't specified that option to my configure line while
testing.  

Another good reason why all our patches need to go via some CI system
before wasting your time with the final autobuild I guess :-)

I'll run a private autobuild, but this much passes an autobuild on ldb
only, so should be good.

Thanks,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba

0001-ldb-Add-some-tests-to-clarify-the-current-iterator-b.patch (5K) Download Attachment
0002-ldb-Add-test-for-transaction-deadlock-detected-when-.patch (9K) Download Attachment
123