Hi,
the attached two patches add a way to detect pre-1.0 cmocka and adds compatible definitions in the first patch and uses them to convert a single unit test.
I found the approach ugly myself, so much that I'm considering converting all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at all unless 1.0 or later is present on the system. It's not functionality after all, "just" tests and for CI we could add cmocka-1.0 to the CI system ourselves..
Opinions?
On (24/02/15 16:55), Jakub Hrozek wrote:
Hi,
the attached two patches add a way to detect pre-1.0 cmocka and adds compatible definitions in the first patch and uses them to convert a single unit test.
I found the approach ugly myself, so much that I'm considering converting all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at all unless 1.0 or later is present on the system. It's not functionality after all, "just" tests and for CI we could add cmocka-1.0 to the CI system ourselves..
Opinions?
+1 for patch. I already see deprecated warnings. cmocka 1.0 is in f21 updates-testing
From e699b9e8b35e680404ce653c986ec9a030dd23aa Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 24 Feb 2015 16:09:12 +0100 Subject: [PATCH 1/2] tests: Add backwards-compatible declarations for pre-1.0 cmocka
src/external/libcmocka.m4 | 12 ++++++++++++ src/tests/cmocka/common_mock.h | 14 ++++++++++++++ 2 files changed, 26 insertions(+)
diff --git a/src/external/libcmocka.m4 b/src/external/libcmocka.m4 index 740d9ad254b50563d4d1cbcab3d10f5920ffd79f..15e5e9294848bca08bd81e288762fae979dcc8b1 100644 --- a/src/external/libcmocka.m4 +++ b/src/external/libcmocka.m4 @@ -15,4 +15,16 @@ AC_DEFUN([AM_CHECK_CMOCKA], [AC_MSG_WARN([No libcmocka library found, cmocka tests will not be built])] ) AM_CONDITIONAL([HAVE_CMOCKA], [test x$have_cmocka = xyes])
- dnl Check if we have the new runner or the legacy one
- SAVE_CFLAGS=$CFLAGS
- SAVE_LIBS=$LIBS
- CFLAGS="$CFLAGS $CMOCKA_CFLAGS"
- LIBS="$LIBS $CMOCKA_LIBS"
- AC_CHECK_FUNCS([_cmocka_run_group_tests])
- CFLAGS=$SAVE_CFLAGS
- LIBS=$SAVE_LIBS
]) diff --git a/src/tests/cmocka/common_mock.h b/src/tests/cmocka/common_mock.h index f305e6ffc45c61db92494c8b50ea11fbc2b10634..d12a77531fcd47149ef335144981ea45eac889cf 100644 --- a/src/tests/cmocka/common_mock.h +++ b/src/tests/cmocka/common_mock.h @@ -53,4 +53,18 @@ enum sss_test_wrapper_call { WRAP_CALL_REAL };
+#ifdef HAVE__CMOCKA_RUN_GROUP_TESTS +#define SSS_CM_FIXTURE_TYPE int +#define SSS_CM_FIXTURE_RET return 0 +#define SSS_CM_TEST struct CMUnitTest +#define SSS_CM_TEST_SETUP_TEARDOWN cmocka_unit_test_setup_teardown +#define SSS_CM_TEST_RUN(tests) cmocka_run_group_tests(tests, NULL, NULL) +#else +#define SSS_CM_FIXTURE_TYPE void +#define SSS_CM_FIXTURE_RET return +#define SSS_CM_TEST UnitTest +#define SSS_CM_TEST_SETUP_TEARDOWN unit_test_setup_teardown
What about? #define cmocka_unit_test_setup_teardown unit_test_setup_teardown
+#define SSS_CM_TEST_RUN(tests) run_tests(tests)
#define cmocka_run_group_tests(tests, NULL, NULL) run_tests(tests)
It will reduce changes in future if we drop support for old cmocka.
LS
On Tue, Feb 24, 2015 at 05:52:26PM +0100, Lukas Slebodnik wrote:
On (24/02/15 16:55), Jakub Hrozek wrote:
Hi,
the attached two patches add a way to detect pre-1.0 cmocka and adds compatible definitions in the first patch and uses them to convert a single unit test.
I found the approach ugly myself, so much that I'm considering converting all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at all unless 1.0 or later is present on the system. It's not functionality after all, "just" tests and for CI we could add cmocka-1.0 to the CI system ourselves..
Opinions?
+1 for patch.
Does +1 for the patch also mean -1 for the proposal to *only* support cmocka-1.0 and later?
I already see deprecated warnings. cmocka 1.0 is in f21 updates-testing
Yes, btw Andreas would update libcmocka on all releases, including private RHEL buildroots. So the "only" systems running pre-1.0 cmocka would be non-RH distributions.
For instance Ubuntu contains 0.4..
On (24/02/15 19:52), Jakub Hrozek wrote:
On Tue, Feb 24, 2015 at 05:52:26PM +0100, Lukas Slebodnik wrote:
On (24/02/15 16:55), Jakub Hrozek wrote:
Hi,
the attached two patches add a way to detect pre-1.0 cmocka and adds compatible definitions in the first patch and uses them to convert a single unit test.
I found the approach ugly myself, so much that I'm considering converting all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at all unless 1.0 or later is present on the system. It's not functionality after all, "just" tests and for CI we could add cmocka-1.0 to the CI system ourselves..
Opinions?
+1 for patch.
Does +1 for the patch also mean -1 for the proposal to *only* support cmocka-1.0 and later?
I already see deprecated warnings. cmocka 1.0 is in f21 updates-testing
Yes, btw Andreas would update libcmocka on all releases, including private RHEL buildroots. So the "only" systems running pre-1.0 cmocka would be non-RH distributions.
For instance Ubuntu contains 0.4..
cmocka is optional dependency but we try to run CI build on debian testing. Debian testing (Jessie) is frozen since 2014-Oct-05. So we will need to wait for next debian release to have cmocka-1.0 in (next) Debian testing)
If we agree we disable cmocka tests in our CI on debian I'm fine with support *only* cmocka-1.0 and later.
LS
On Tue, Feb 24, 2015 at 09:16:09PM +0100, Lukas Slebodnik wrote:
On (24/02/15 19:52), Jakub Hrozek wrote:
On Tue, Feb 24, 2015 at 05:52:26PM +0100, Lukas Slebodnik wrote:
On (24/02/15 16:55), Jakub Hrozek wrote:
Hi,
the attached two patches add a way to detect pre-1.0 cmocka and adds compatible definitions in the first patch and uses them to convert a single unit test.
I found the approach ugly myself, so much that I'm considering converting all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at all unless 1.0 or later is present on the system. It's not functionality after all, "just" tests and for CI we could add cmocka-1.0 to the CI system ourselves..
Opinions?
+1 for patch.
Does +1 for the patch also mean -1 for the proposal to *only* support cmocka-1.0 and later?
I already see deprecated warnings. cmocka 1.0 is in f21 updates-testing
Yes, btw Andreas would update libcmocka on all releases, including private RHEL buildroots. So the "only" systems running pre-1.0 cmocka would be non-RH distributions.
For instance Ubuntu contains 0.4..
cmocka is optional dependency but we try to run CI build on debian testing. Debian testing (Jessie) is frozen since 2014-Oct-05. So we will need to wait for next debian release to have cmocka-1.0 in (next) Debian testing)
If we agree we disable cmocka tests in our CI on debian I'm fine with support *only* cmocka-1.0 and later.
I was proposing to build cmocka-1.0 from source on the Debian CI machines..
On (25/02/15 08:33), Jakub Hrozek wrote:
On Tue, Feb 24, 2015 at 09:16:09PM +0100, Lukas Slebodnik wrote:
On (24/02/15 19:52), Jakub Hrozek wrote:
On Tue, Feb 24, 2015 at 05:52:26PM +0100, Lukas Slebodnik wrote:
On (24/02/15 16:55), Jakub Hrozek wrote:
Hi,
the attached two patches add a way to detect pre-1.0 cmocka and adds compatible definitions in the first patch and uses them to convert a single unit test.
I found the approach ugly myself, so much that I'm considering converting all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at all unless 1.0 or later is present on the system. It's not functionality after all, "just" tests and for CI we could add cmocka-1.0 to the CI system ourselves..
Opinions?
+1 for patch.
Does +1 for the patch also mean -1 for the proposal to *only* support cmocka-1.0 and later?
I already see deprecated warnings. cmocka 1.0 is in f21 updates-testing
Yes, btw Andreas would update libcmocka on all releases, including private RHEL buildroots. So the "only" systems running pre-1.0 cmocka would be non-RH distributions.
For instance Ubuntu contains 0.4..
cmocka is optional dependency but we try to run CI build on debian testing. Debian testing (Jessie) is frozen since 2014-Oct-05. So we will need to wait for next debian release to have cmocka-1.0 in (next) Debian testing)
If we agree we disable cmocka tests in our CI on debian I'm fine with support *only* cmocka-1.0 and later.
I was proposing to build cmocka-1.0 from source on the Debian CI machines.
I haven't seen this proposal yet :-)
IMHO, it's reasonable compromise. If Nikolai agrees let's go with cmocka-1.0+ way
LS
On Wed, Feb 25, 2015 at 09:51:24AM +0100, Lukas Slebodnik wrote:
On (25/02/15 08:33), Jakub Hrozek wrote:
On Tue, Feb 24, 2015 at 09:16:09PM +0100, Lukas Slebodnik wrote:
On (24/02/15 19:52), Jakub Hrozek wrote:
On Tue, Feb 24, 2015 at 05:52:26PM +0100, Lukas Slebodnik wrote:
On (24/02/15 16:55), Jakub Hrozek wrote:
Hi,
the attached two patches add a way to detect pre-1.0 cmocka and adds compatible definitions in the first patch and uses them to convert a single unit test.
I found the approach ugly myself, so much that I'm considering converting all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at all unless 1.0 or later is present on the system. It's not functionality after all, "just" tests and for CI we could add cmocka-1.0 to the CI system ourselves..
Opinions?
+1 for patch.
Does +1 for the patch also mean -1 for the proposal to *only* support cmocka-1.0 and later?
I already see deprecated warnings. cmocka 1.0 is in f21 updates-testing
Yes, btw Andreas would update libcmocka on all releases, including private RHEL buildroots. So the "only" systems running pre-1.0 cmocka would be non-RH distributions.
For instance Ubuntu contains 0.4..
cmocka is optional dependency but we try to run CI build on debian testing. Debian testing (Jessie) is frozen since 2014-Oct-05. So we will need to wait for next debian release to have cmocka-1.0 in (next) Debian testing)
If we agree we disable cmocka tests in our CI on debian I'm fine with support *only* cmocka-1.0 and later.
I was proposing to build cmocka-1.0 from source on the Debian CI machines.
I haven't seen this proposal yet :-)
IMHO, it's reasonable compromise. If Nikolai agrees let's go with cmocka-1.0+ way
Does Nikolai agree? :-)
On 02/25/2015 11:14 AM, Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 09:51:24AM +0100, Lukas Slebodnik wrote:
On (25/02/15 08:33), Jakub Hrozek wrote:
On Tue, Feb 24, 2015 at 09:16:09PM +0100, Lukas Slebodnik wrote:
On (24/02/15 19:52), Jakub Hrozek wrote:
On Tue, Feb 24, 2015 at 05:52:26PM +0100, Lukas Slebodnik wrote:
On (24/02/15 16:55), Jakub Hrozek wrote: > Hi, > > the attached two patches add a way to detect pre-1.0 cmocka and adds > compatible definitions in the first patch and uses them to convert a > single unit test. > > I found the approach ugly myself, so much that I'm considering converting > all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at > all unless 1.0 or later is present on the system. It's not functionality > after all, "just" tests and for CI we could add cmocka-1.0 to the CI > system ourselves.. > > Opinions? +1 for patch.
Does +1 for the patch also mean -1 for the proposal to *only* support cmocka-1.0 and later?
I already see deprecated warnings. cmocka 1.0 is in f21 updates-testing
Yes, btw Andreas would update libcmocka on all releases, including private RHEL buildroots. So the "only" systems running pre-1.0 cmocka would be non-RH distributions.
For instance Ubuntu contains 0.4..
cmocka is optional dependency but we try to run CI build on debian testing. Debian testing (Jessie) is frozen since 2014-Oct-05. So we will need to wait for next debian release to have cmocka-1.0 in (next) Debian testing)
If we agree we disable cmocka tests in our CI on debian I'm fine with support *only* cmocka-1.0 and later.
I was proposing to build cmocka-1.0 from source on the Debian CI machines.
I haven't seen this proposal yet :-)
IMHO, it's reasonable compromise. If Nikolai agrees let's go with cmocka-1.0+ way
Does Nikolai agree? :-)
Sure :) It will do, as long as we support running CI without cmocka-1.0+, i.e. without tests requiring it.
Nick
On Wed, Feb 25, 2015 at 10:14:18AM +0100, Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 09:51:24AM +0100, Lukas Slebodnik wrote:
On (25/02/15 08:33), Jakub Hrozek wrote:
On Tue, Feb 24, 2015 at 09:16:09PM +0100, Lukas Slebodnik wrote:
On (24/02/15 19:52), Jakub Hrozek wrote:
On Tue, Feb 24, 2015 at 05:52:26PM +0100, Lukas Slebodnik wrote:
On (24/02/15 16:55), Jakub Hrozek wrote: >Hi, > >the attached two patches add a way to detect pre-1.0 cmocka and adds >compatible definitions in the first patch and uses them to convert a >single unit test. > >I found the approach ugly myself, so much that I'm considering converting >all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at >all unless 1.0 or later is present on the system. It's not functionality >after all, "just" tests and for CI we could add cmocka-1.0 to the CI >system ourselves.. > >Opinions? +1 for patch.
Does +1 for the patch also mean -1 for the proposal to *only* support cmocka-1.0 and later?
I already see deprecated warnings. cmocka 1.0 is in f21 updates-testing
Yes, btw Andreas would update libcmocka on all releases, including private RHEL buildroots. So the "only" systems running pre-1.0 cmocka would be non-RH distributions.
For instance Ubuntu contains 0.4..
cmocka is optional dependency but we try to run CI build on debian testing. Debian testing (Jessie) is frozen since 2014-Oct-05. So we will need to wait for next debian release to have cmocka-1.0 in (next) Debian testing)
If we agree we disable cmocka tests in our CI on debian I'm fine with support *only* cmocka-1.0 and later.
I was proposing to build cmocka-1.0 from source on the Debian CI machines.
I haven't seen this proposal yet :-)
IMHO, it's reasonable compromise. If Nikolai agrees let's go with cmocka-1.0+ way
Does Nikolai agree? :-)
btw what about the stable branches, do we just remove the test when backporting patches with new tests? I think that would be the easiest way..
Alternatively, we could add the new tests to a new block HAVE_CMOCKA_1_0 in Makefile.am -- that would bring some work when backporting patches, but we wouldn't have to diverge or change the code itself, only the Makefile.am hunk, where the conflict would be minimal.
On (25/02/15 10:55), Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 10:14:18AM +0100, Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 09:51:24AM +0100, Lukas Slebodnik wrote:
On (25/02/15 08:33), Jakub Hrozek wrote:
On Tue, Feb 24, 2015 at 09:16:09PM +0100, Lukas Slebodnik wrote:
On (24/02/15 19:52), Jakub Hrozek wrote:
On Tue, Feb 24, 2015 at 05:52:26PM +0100, Lukas Slebodnik wrote: > On (24/02/15 16:55), Jakub Hrozek wrote: > >Hi, > > > >the attached two patches add a way to detect pre-1.0 cmocka and adds > >compatible definitions in the first patch and uses them to convert a > >single unit test. > > > >I found the approach ugly myself, so much that I'm considering converting > >all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at > >all unless 1.0 or later is present on the system. It's not functionality > >after all, "just" tests and for CI we could add cmocka-1.0 to the CI > >system ourselves.. > > > >Opinions? > +1 for patch.
Does +1 for the patch also mean -1 for the proposal to *only* support cmocka-1.0 and later?
> I already see deprecated warnings. cmocka 1.0 is in f21 updates-testing
Yes, btw Andreas would update libcmocka on all releases, including private RHEL buildroots. So the "only" systems running pre-1.0 cmocka would be non-RH distributions.
For instance Ubuntu contains 0.4..
cmocka is optional dependency but we try to run CI build on debian testing. Debian testing (Jessie) is frozen since 2014-Oct-05. So we will need to wait for next debian release to have cmocka-1.0 in (next) Debian testing)
If we agree we disable cmocka tests in our CI on debian I'm fine with support *only* cmocka-1.0 and later.
I was proposing to build cmocka-1.0 from source on the Debian CI machines.
I haven't seen this proposal yet :-)
IMHO, it's reasonable compromise. If Nikolai agrees let's go with cmocka-1.0+ way
Does Nikolai agree? :-)
btw what about the stable branches, do we just remove the test when backporting patches with new tests? I think that would be the easiest way..
Alternatively, we could add the new tests to a new block HAVE_CMOCKA_1_0 in Makefile.am -- that would bring some work when backporting patches, but we wouldn't have to diverge or change the code itself, only the Makefile.am hunk, where the conflict would be minimal.
I would prefer adding strict requirements to cmocka-1.0 into configure and if it is not detected then cmocka test will not be executed.
We can ignore(disable) deprecated warning in stable branches:-)
LS
On 02/25/2015 03:10 PM, Lukas Slebodnik wrote:
On (25/02/15 10:55), Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 10:14:18AM +0100, Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 09:51:24AM +0100, Lukas Slebodnik wrote:
On (25/02/15 08:33), Jakub Hrozek wrote:
On Tue, Feb 24, 2015 at 09:16:09PM +0100, Lukas Slebodnik wrote:
On (24/02/15 19:52), Jakub Hrozek wrote: > On Tue, Feb 24, 2015 at 05:52:26PM +0100, Lukas Slebodnik wrote: >> On (24/02/15 16:55), Jakub Hrozek wrote: >>> Hi, >>> >>> the attached two patches add a way to detect pre-1.0 cmocka and adds >>> compatible definitions in the first patch and uses them to convert a >>> single unit test. >>> >>> I found the approach ugly myself, so much that I'm considering converting >>> all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at >>> all unless 1.0 or later is present on the system. It's not functionality >>> after all, "just" tests and for CI we could add cmocka-1.0 to the CI >>> system ourselves.. >>> >>> Opinions? >> +1 for patch. > > Does +1 for the patch also mean -1 for the proposal to *only* support > cmocka-1.0 and later? > >> I already see deprecated warnings. cmocka 1.0 is in f21 updates-testing > > Yes, btw Andreas would update libcmocka on all releases, including > private RHEL buildroots. So the "only" systems running pre-1.0 cmocka > would be non-RH distributions. > > For instance Ubuntu contains 0.4.. cmocka is optional dependency but we try to run CI build on debian testing. Debian testing (Jessie) is frozen since 2014-Oct-05. So we will need to wait for next debian release to have cmocka-1.0 in (next) Debian testing)
If we agree we disable cmocka tests in our CI on debian I'm fine with support *only* cmocka-1.0 and later.
I was proposing to build cmocka-1.0 from source on the Debian CI machines.
I haven't seen this proposal yet :-)
IMHO, it's reasonable compromise. If Nikolai agrees let's go with cmocka-1.0+ way
Does Nikolai agree? :-)
btw what about the stable branches, do we just remove the test when backporting patches with new tests? I think that would be the easiest way..
Alternatively, we could add the new tests to a new block HAVE_CMOCKA_1_0 in Makefile.am -- that would bring some work when backporting patches, but we wouldn't have to diverge or change the code itself, only the Makefile.am hunk, where the conflict would be minimal.
I would prefer adding strict requirements to cmocka-1.0 into configure and if it is not detected then cmocka test will not be executed.
We can ignore(disable) deprecated warning in stable branches:-)
LS
+1
On Wed, Feb 25, 2015 at 03:17:54PM +0100, Pavel Březina wrote:
On 02/25/2015 03:10 PM, Lukas Slebodnik wrote:
On (25/02/15 10:55), Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 10:14:18AM +0100, Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 09:51:24AM +0100, Lukas Slebodnik wrote:
On (25/02/15 08:33), Jakub Hrozek wrote:
On Tue, Feb 24, 2015 at 09:16:09PM +0100, Lukas Slebodnik wrote: >On (24/02/15 19:52), Jakub Hrozek wrote: >>On Tue, Feb 24, 2015 at 05:52:26PM +0100, Lukas Slebodnik wrote: >>>On (24/02/15 16:55), Jakub Hrozek wrote: >>>>Hi, >>>> >>>>the attached two patches add a way to detect pre-1.0 cmocka and adds >>>>compatible definitions in the first patch and uses them to convert a >>>>single unit test. >>>> >>>>I found the approach ugly myself, so much that I'm considering converting >>>>all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at >>>>all unless 1.0 or later is present on the system. It's not functionality >>>>after all, "just" tests and for CI we could add cmocka-1.0 to the CI >>>>system ourselves.. >>>> >>>>Opinions? >>>+1 for patch. >> >>Does +1 for the patch also mean -1 for the proposal to *only* support >>cmocka-1.0 and later? >> >>>I already see deprecated warnings. cmocka 1.0 is in f21 updates-testing >> >>Yes, btw Andreas would update libcmocka on all releases, including >>private RHEL buildroots. So the "only" systems running pre-1.0 cmocka >>would be non-RH distributions. >> >>For instance Ubuntu contains 0.4.. >cmocka is optional dependency but we try to run CI build on debian testing. >Debian testing (Jessie) is frozen since 2014-Oct-05. So we will need to wait >for next debian release to have cmocka-1.0 in (next) Debian testing) > >If we agree we disable cmocka tests in our CI on debian >I'm fine with support *only* cmocka-1.0 and later.
I was proposing to build cmocka-1.0 from source on the Debian CI machines.
I haven't seen this proposal yet :-)
IMHO, it's reasonable compromise. If Nikolai agrees let's go with cmocka-1.0+ way
Does Nikolai agree? :-)
btw what about the stable branches, do we just remove the test when backporting patches with new tests? I think that would be the easiest way..
Alternatively, we could add the new tests to a new block HAVE_CMOCKA_1_0 in Makefile.am -- that would bring some work when backporting patches, but we wouldn't have to diverge or change the code itself, only the Makefile.am hunk, where the conflict would be minimal.
I would prefer adding strict requirements to cmocka-1.0 into configure and if it is not detected then cmocka test will not be executed.
We can ignore(disable) deprecated warning in stable branches:-)
LS
+1
I'm not sure we understood each other, I was specifically asking about stabe branch. In master, we would add strict requirements for cmocka 1.0+, convert all tests there and don't run any tests if cmocka 1.0+ is not found.
In sssd-1-12, we would keep the existing tests untouched and ignore the deprecation warnings. But what if someone submits a patch that needs to be included in sssd-1-12, too but adds a test that is written using cmocka-1.0 API? We could either: a) backport the patch without the test b) backport the patch as-is, but add the Makefile.am part of the patch into a new block that gets executed only if cmocka 1.0 is available. Currently we only have a global HAVE_CMOCKA if-endif.
I'm fine with a), do other developers agree?
----- Original Message -----
From: "Jakub Hrozek" jhrozek@redhat.com To: sssd-devel@lists.fedorahosted.org Sent: Wednesday, February 25, 2015 4:57:25 PM Subject: Re: [SSSD] [PATCH] RFC: Support both cmocka 1.0 and pre-1.0 API at the same time
On Wed, Feb 25, 2015 at 03:17:54PM +0100, Pavel Březina wrote:
On 02/25/2015 03:10 PM, Lukas Slebodnik wrote:
On (25/02/15 10:55), Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 10:14:18AM +0100, Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 09:51:24AM +0100, Lukas Slebodnik wrote:
On (25/02/15 08:33), Jakub Hrozek wrote: >On Tue, Feb 24, 2015 at 09:16:09PM +0100, Lukas Slebodnik wrote: >>On (24/02/15 19:52), Jakub Hrozek wrote: >>>On Tue, Feb 24, 2015 at 05:52:26PM +0100, Lukas Slebodnik wrote: >>>>On (24/02/15 16:55), Jakub Hrozek wrote: >>>>>Hi, >>>>> >>>>>the attached two patches add a way to detect pre-1.0 cmocka and >>>>>adds >>>>>compatible definitions in the first patch and uses them to convert >>>>>a >>>>>single unit test. >>>>> >>>>>I found the approach ugly myself, so much that I'm considering >>>>>converting >>>>>all cmocka-based tests to cmocka-1.0 and don't compile the cmocka >>>>>tests at >>>>>all unless 1.0 or later is present on the system. It's not >>>>>functionality >>>>>after all, "just" tests and for CI we could add cmocka-1.0 to the >>>>>CI >>>>>system ourselves.. >>>>> >>>>>Opinions? >>>>+1 for patch. >>> >>>Does +1 for the patch also mean -1 for the proposal to *only* >>>support >>>cmocka-1.0 and later? >>> >>>>I already see deprecated warnings. cmocka 1.0 is in f21 >>>>updates-testing >>> >>>Yes, btw Andreas would update libcmocka on all releases, including >>>private RHEL buildroots. So the "only" systems running pre-1.0 >>>cmocka >>>would be non-RH distributions. >>> >>>For instance Ubuntu contains 0.4.. >>cmocka is optional dependency but we try to run CI build on debian >>testing. >>Debian testing (Jessie) is frozen since 2014-Oct-05. So we will need >>to wait >>for next debian release to have cmocka-1.0 in (next) Debian testing) >> >>If we agree we disable cmocka tests in our CI on debian >>I'm fine with support *only* cmocka-1.0 and later. > >I was proposing to build cmocka-1.0 from source on the Debian CI >machines. I haven't seen this proposal yet :-)
IMHO, it's reasonable compromise. If Nikolai agrees let's go with cmocka-1.0+ way
Does Nikolai agree? :-)
btw what about the stable branches, do we just remove the test when backporting patches with new tests? I think that would be the easiest way..
Alternatively, we could add the new tests to a new block HAVE_CMOCKA_1_0 in Makefile.am -- that would bring some work when backporting patches, but we wouldn't have to diverge or change the code itself, only the Makefile.am hunk, where the conflict would be minimal.
I would prefer adding strict requirements to cmocka-1.0 into configure and if it is not detected then cmocka test will not be executed.
We can ignore(disable) deprecated warning in stable branches:-)
LS
+1
I'm not sure we understood each other, I was specifically asking about stabe branch. In master, we would add strict requirements for cmocka 1.0+, convert all tests there and don't run any tests if cmocka 1.0+ is not found.
In sssd-1-12, we would keep the existing tests untouched and ignore the deprecation warnings. But what if someone submits a patch that needs to be included in sssd-1-12, too but adds a test that is written using cmocka-1.0 API? We could either: a) backport the patch without the test b) backport the patch as-is, but add the Makefile.am part of the patch into a new block that gets executed only if cmocka 1.0 is available. Currently we only have a global HAVE_CMOCKA if-endif.
I'm fine with a), do other developers agree?
Ah, sorry. The +1 was actualy for HAVE_CMOCKA_1_0 block. I don't think we have to touch the patches at all if we add a new conditional block to Makefile.am in master that will be executed only if new features are available.
On (25/02/15 16:57), Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 03:17:54PM +0100, Pavel Březina wrote:
On 02/25/2015 03:10 PM, Lukas Slebodnik wrote:
On (25/02/15 10:55), Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 10:14:18AM +0100, Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 09:51:24AM +0100, Lukas Slebodnik wrote:
On (25/02/15 08:33), Jakub Hrozek wrote: >On Tue, Feb 24, 2015 at 09:16:09PM +0100, Lukas Slebodnik wrote: >>On (24/02/15 19:52), Jakub Hrozek wrote: >>>On Tue, Feb 24, 2015 at 05:52:26PM +0100, Lukas Slebodnik wrote: >>>>On (24/02/15 16:55), Jakub Hrozek wrote: >>>>>Hi, >>>>> >>>>>the attached two patches add a way to detect pre-1.0 cmocka and adds >>>>>compatible definitions in the first patch and uses them to convert a >>>>>single unit test. >>>>> >>>>>I found the approach ugly myself, so much that I'm considering converting >>>>>all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at >>>>>all unless 1.0 or later is present on the system. It's not functionality >>>>>after all, "just" tests and for CI we could add cmocka-1.0 to the CI >>>>>system ourselves.. >>>>> >>>>>Opinions? >>>>+1 for patch. >>> >>>Does +1 for the patch also mean -1 for the proposal to *only* support >>>cmocka-1.0 and later? >>> >>>>I already see deprecated warnings. cmocka 1.0 is in f21 updates-testing >>> >>>Yes, btw Andreas would update libcmocka on all releases, including >>>private RHEL buildroots. So the "only" systems running pre-1.0 cmocka >>>would be non-RH distributions. >>> >>>For instance Ubuntu contains 0.4.. >>cmocka is optional dependency but we try to run CI build on debian testing. >>Debian testing (Jessie) is frozen since 2014-Oct-05. So we will need to wait >>for next debian release to have cmocka-1.0 in (next) Debian testing) >> >>If we agree we disable cmocka tests in our CI on debian >>I'm fine with support *only* cmocka-1.0 and later. > >I was proposing to build cmocka-1.0 from source on the Debian CI >machines. I haven't seen this proposal yet :-)
IMHO, it's reasonable compromise. If Nikolai agrees let's go with cmocka-1.0+ way
Does Nikolai agree? :-)
btw what about the stable branches, do we just remove the test when backporting patches with new tests? I think that would be the easiest way..
Alternatively, we could add the new tests to a new block HAVE_CMOCKA_1_0 in Makefile.am -- that would bring some work when backporting patches, but we wouldn't have to diverge or change the code itself, only the Makefile.am hunk, where the conflict would be minimal.
I would prefer adding strict requirements to cmocka-1.0 into configure and if it is not detected then cmocka test will not be executed.
We can ignore(disable) deprecated warning in stable branches:-)
LS
+1
I'm not sure we understood each other, I was specifically asking about stabe branch. In master, we would add strict requirements for cmocka 1.0+, convert all tests there and don't run any tests if cmocka 1.0+ is not found.
Agree with strict requirement Agree with converting tests.
In sssd-1-12, we would keep the existing tests untouched and ignore the deprecation warnings. But what if someone submits a patch that needs to be included in sssd-1-12, too but adds a test that is written using cmocka-1.0 API? We could either: a) backport the patch without the test b) backport the patch as-is, but add the Makefile.am part of the patch into a new block that gets executed only if cmocka 1.0 is available. Currently we only have a global HAVE_CMOCKA if-endif.
c) - add strict requirement for cmocka 1.0+ - do not convert tests - ignore warnings caused by deprecated function in cmocka 1.0
Result: * change in stable branch will be small * there will not be problem with backporting new tests * makefile will not be poluted with another if else block * if cmocka-1.0+ is not available then cmocka tests will not be executed
Distributions: arch - does not run tests at all opensuse - does not run tests at all gentoo - does not run cmocka test debian - cmocka tests are disabled (due to failueres on some arch) [1] ubuntu - cmocka tests are disabled (due to failueres on some arch) fedora - cmocka-1.0 will be available epel{6,7} - cmocka-1.0 will be available
[1] http://anonscm.debian.org/cgit/pkg-sssd/sssd.git/commit/?id=ea1575581b86cd32...
cmocka is an optional dependency. So I don't think we need to complicate Makefile in stable branches with with cmocka < 1.0 and cmocka >= 1.0.
LS
On Wed, Feb 25, 2015 at 11:20:45PM +0100, Lukas Slebodnik wrote:
On (25/02/15 16:57), Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 03:17:54PM +0100, Pavel Březina wrote:
On 02/25/2015 03:10 PM, Lukas Slebodnik wrote:
On (25/02/15 10:55), Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 10:14:18AM +0100, Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 09:51:24AM +0100, Lukas Slebodnik wrote: >On (25/02/15 08:33), Jakub Hrozek wrote: >>On Tue, Feb 24, 2015 at 09:16:09PM +0100, Lukas Slebodnik wrote: >>>On (24/02/15 19:52), Jakub Hrozek wrote: >>>>On Tue, Feb 24, 2015 at 05:52:26PM +0100, Lukas Slebodnik wrote: >>>>>On (24/02/15 16:55), Jakub Hrozek wrote: >>>>>>Hi, >>>>>> >>>>>>the attached two patches add a way to detect pre-1.0 cmocka and adds >>>>>>compatible definitions in the first patch and uses them to convert a >>>>>>single unit test. >>>>>> >>>>>>I found the approach ugly myself, so much that I'm considering converting >>>>>>all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at >>>>>>all unless 1.0 or later is present on the system. It's not functionality >>>>>>after all, "just" tests and for CI we could add cmocka-1.0 to the CI >>>>>>system ourselves.. >>>>>> >>>>>>Opinions? >>>>>+1 for patch. >>>> >>>>Does +1 for the patch also mean -1 for the proposal to *only* support >>>>cmocka-1.0 and later? >>>> >>>>>I already see deprecated warnings. cmocka 1.0 is in f21 updates-testing >>>> >>>>Yes, btw Andreas would update libcmocka on all releases, including >>>>private RHEL buildroots. So the "only" systems running pre-1.0 cmocka >>>>would be non-RH distributions. >>>> >>>>For instance Ubuntu contains 0.4.. >>>cmocka is optional dependency but we try to run CI build on debian testing. >>>Debian testing (Jessie) is frozen since 2014-Oct-05. So we will need to wait >>>for next debian release to have cmocka-1.0 in (next) Debian testing) >>> >>>If we agree we disable cmocka tests in our CI on debian >>>I'm fine with support *only* cmocka-1.0 and later. >> >>I was proposing to build cmocka-1.0 from source on the Debian CI >>machines. >I haven't seen this proposal yet :-) > >IMHO, it's reasonable compromise. >If Nikolai agrees let's go with cmocka-1.0+ way
Does Nikolai agree? :-)
btw what about the stable branches, do we just remove the test when backporting patches with new tests? I think that would be the easiest way..
Alternatively, we could add the new tests to a new block HAVE_CMOCKA_1_0 in Makefile.am -- that would bring some work when backporting patches, but we wouldn't have to diverge or change the code itself, only the Makefile.am hunk, where the conflict would be minimal.
I would prefer adding strict requirements to cmocka-1.0 into configure and if it is not detected then cmocka test will not be executed.
We can ignore(disable) deprecated warning in stable branches:-)
LS
+1
I'm not sure we understood each other, I was specifically asking about stabe branch. In master, we would add strict requirements for cmocka 1.0+, convert all tests there and don't run any tests if cmocka 1.0+ is not found.
Agree with strict requirement Agree with converting tests.
In sssd-1-12, we would keep the existing tests untouched and ignore the deprecation warnings. But what if someone submits a patch that needs to be included in sssd-1-12, too but adds a test that is written using cmocka-1.0 API? We could either: a) backport the patch without the test b) backport the patch as-is, but add the Makefile.am part of the patch into a new block that gets executed only if cmocka 1.0 is available. Currently we only have a global HAVE_CMOCKA if-endif.
c) - add strict requirement for cmocka 1.0+ - do not convert tests - ignore warnings caused by deprecated function in cmocka 1.0
Result: * change in stable branch will be small * there will not be problem with backporting new tests * makefile will not be poluted with another if else block * if cmocka-1.0+ is not available then cmocka tests will not be executed
Distributions: arch - does not run tests at all opensuse - does not run tests at all gentoo - does not run cmocka test debian - cmocka tests are disabled (due to failueres on some arch) [1] ubuntu - cmocka tests are disabled (due to failueres on some arch) fedora - cmocka-1.0 will be available epel{6,7} - cmocka-1.0 will be available
OK, I didn't propose this change because it would /decrease/ the test coverage. But seeing the summary above, it does seem like the best solution.
Thanks.
On (25/02/15 23:29), Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 11:20:45PM +0100, Lukas Slebodnik wrote:
On (25/02/15 16:57), Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 03:17:54PM +0100, Pavel Březina wrote:
On 02/25/2015 03:10 PM, Lukas Slebodnik wrote:
On (25/02/15 10:55), Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 10:14:18AM +0100, Jakub Hrozek wrote: >On Wed, Feb 25, 2015 at 09:51:24AM +0100, Lukas Slebodnik wrote: >>On (25/02/15 08:33), Jakub Hrozek wrote: >>>On Tue, Feb 24, 2015 at 09:16:09PM +0100, Lukas Slebodnik wrote: >>>>On (24/02/15 19:52), Jakub Hrozek wrote: >>>>>On Tue, Feb 24, 2015 at 05:52:26PM +0100, Lukas Slebodnik wrote: >>>>>>On (24/02/15 16:55), Jakub Hrozek wrote: >>>>>>>Hi, >>>>>>> >>>>>>>the attached two patches add a way to detect pre-1.0 cmocka and adds >>>>>>>compatible definitions in the first patch and uses them to convert a >>>>>>>single unit test. >>>>>>> >>>>>>>I found the approach ugly myself, so much that I'm considering converting >>>>>>>all cmocka-based tests to cmocka-1.0 and don't compile the cmocka tests at >>>>>>>all unless 1.0 or later is present on the system. It's not functionality >>>>>>>after all, "just" tests and for CI we could add cmocka-1.0 to the CI >>>>>>>system ourselves.. >>>>>>> >>>>>>>Opinions? >>>>>>+1 for patch. >>>>> >>>>>Does +1 for the patch also mean -1 for the proposal to *only* support >>>>>cmocka-1.0 and later? >>>>> >>>>>>I already see deprecated warnings. cmocka 1.0 is in f21 updates-testing >>>>> >>>>>Yes, btw Andreas would update libcmocka on all releases, including >>>>>private RHEL buildroots. So the "only" systems running pre-1.0 cmocka >>>>>would be non-RH distributions. >>>>> >>>>>For instance Ubuntu contains 0.4.. >>>>cmocka is optional dependency but we try to run CI build on debian testing. >>>>Debian testing (Jessie) is frozen since 2014-Oct-05. So we will need to wait >>>>for next debian release to have cmocka-1.0 in (next) Debian testing) >>>> >>>>If we agree we disable cmocka tests in our CI on debian >>>>I'm fine with support *only* cmocka-1.0 and later. >>> >>>I was proposing to build cmocka-1.0 from source on the Debian CI >>>machines. >>I haven't seen this proposal yet :-) >> >>IMHO, it's reasonable compromise. >>If Nikolai agrees let's go with cmocka-1.0+ way > >Does Nikolai agree? :-)
btw what about the stable branches, do we just remove the test when backporting patches with new tests? I think that would be the easiest way..
Alternatively, we could add the new tests to a new block HAVE_CMOCKA_1_0 in Makefile.am -- that would bring some work when backporting patches, but we wouldn't have to diverge or change the code itself, only the Makefile.am hunk, where the conflict would be minimal.
I would prefer adding strict requirements to cmocka-1.0 into configure and if it is not detected then cmocka test will not be executed.
We can ignore(disable) deprecated warning in stable branches:-)
LS
+1
I'm not sure we understood each other, I was specifically asking about stabe branch. In master, we would add strict requirements for cmocka 1.0+, convert all tests there and don't run any tests if cmocka 1.0+ is not found.
Agree with strict requirement Agree with converting tests.
In sssd-1-12, we would keep the existing tests untouched and ignore the deprecation warnings. But what if someone submits a patch that needs to be included in sssd-1-12, too but adds a test that is written using cmocka-1.0 API? We could either: a) backport the patch without the test b) backport the patch as-is, but add the Makefile.am part of the patch into a new block that gets executed only if cmocka 1.0 is available. Currently we only have a global HAVE_CMOCKA if-endif.
c) - add strict requirement for cmocka 1.0+ - do not convert tests - ignore warnings caused by deprecated function in cmocka 1.0
Result: * change in stable branch will be small * there will not be problem with backporting new tests * makefile will not be poluted with another if else block * if cmocka-1.0+ is not available then cmocka tests will not be executed
Distributions: arch - does not run tests at all opensuse - does not run tests at all gentoo - does not run cmocka test debian - cmocka tests are disabled (due to failueres on some arch) [1] ubuntu - cmocka tests are disabled (due to failueres on some arch) fedora - cmocka-1.0 will be available epel{6,7} - cmocka-1.0 will be available
OK, I didn't propose this change because it would /decrease/ the test coverage. But seeing the summary above, it does seem like the best solution.
BTW my current workaround to get rid of deprecated warnings is to modify cmocka.h
#define CMOCKA_DEPRECATED instead of #define CMOCKA_DEPRECATED __attribute__ ((deprecated))
I know it's not nice solution.
LS
On Wed, Feb 25, 2015 at 11:20:45PM +0100, Lukas Slebodnik wrote:
I'm not sure we understood each other, I was specifically asking about stabe branch. In master, we would add strict requirements for cmocka 1.0+, convert all tests there and don't run any tests if cmocka 1.0+ is not found.
Agree with strict requirement Agree with converting tests.
See attached patches. CI - http://sssd-ci.duckdns.org/logs/job/9/24/summary.html
In sssd-1-12, we would keep the existing tests untouched and ignore the deprecation warnings. But what if someone submits a patch that needs to be included in sssd-1-12, too but adds a test that is written using cmocka-1.0 API? We could either: a) backport the patch without the test b) backport the patch as-is, but add the Makefile.am part of the patch into a new block that gets executed only if cmocka 1.0 is available. Currently we only have a global HAVE_CMOCKA if-endif.
c) - add strict requirement for cmocka 1.0+ - do not convert tests - ignore warnings caused by deprecated function in cmocka 1.0
Result: * change in stable branch will be small * there will not be problem with backporting new tests * makefile will not be poluted with another if else block * if cmocka-1.0+ is not available then cmocka tests will not be executed
Distributions: arch - does not run tests at all opensuse - does not run tests at all gentoo - does not run cmocka test debian - cmocka tests are disabled (due to failueres on some arch) [1] ubuntu - cmocka tests are disabled (due to failueres on some arch) fedora - cmocka-1.0 will be available epel{6,7} - cmocka-1.0 will be available
[1] http://anonscm.debian.org/cgit/pkg-sssd/sssd.git/commit/?id=ea1575581b86cd32...
cmocka is an optional dependency. So I don't think we need to complicate Makefile in stable branches with with cmocka < 1.0 and cmocka >= 1.0.
I think we should just push the first and second patch to sssd-1-12, then.
On (10/03/15 14:20), Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 11:20:45PM +0100, Lukas Slebodnik wrote:
I'm not sure we understood each other, I was specifically asking about stabe branch. In master, we would add strict requirements for cmocka 1.0+, convert all tests there and don't run any tests if cmocka 1.0+ is not found.
Agree with strict requirement Agree with converting tests.
See attached patches. CI - http://sssd-ci.duckdns.org/logs/job/9/24/summary.html
In sssd-1-12, we would keep the existing tests untouched and ignore the deprecation warnings. But what if someone submits a patch that needs to be included in sssd-1-12, too but adds a test that is written using cmocka-1.0 API? We could either: a) backport the patch without the test b) backport the patch as-is, but add the Makefile.am part of the patch into a new block that gets executed only if cmocka 1.0 is available. Currently we only have a global HAVE_CMOCKA if-endif.
c) - add strict requirement for cmocka 1.0+ - do not convert tests - ignore warnings caused by deprecated function in cmocka 1.0
Result: * change in stable branch will be small * there will not be problem with backporting new tests * makefile will not be poluted with another if else block * if cmocka-1.0+ is not available then cmocka tests will not be executed
Distributions: arch - does not run tests at all opensuse - does not run tests at all gentoo - does not run cmocka test debian - cmocka tests are disabled (due to failueres on some arch) [1] ubuntu - cmocka tests are disabled (due to failueres on some arch) fedora - cmocka-1.0 will be available epel{6,7} - cmocka-1.0 will be available
[1] http://anonscm.debian.org/cgit/pkg-sssd/sssd.git/commit/?id=ea1575581b86cd32...
cmocka is an optional dependency. So I don't think we need to complicate Makefile in stable branches with with cmocka < 1.0 and cmocka >= 1.0.
I think we should just push the first and second patch to sssd-1-12, then.
From 6cdaf7234c8f1b4e28aeb92129c015ec336c24bb Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 9 Mar 2015 21:25:47 +0100 Subject: [PATCH 1/3] build: Only run cmocka tests if cmocka 1.0 or newer is available
Rename the Makefile.am conditionals to HAVE_CMOCKA_1 to make it clear only the latest release is suported.
Makefile.am | 12 ++++++------ src/external/libcmocka.m4 | 19 +++++++++++++++++-- src/tests/cwrap/Makefile.am | 4 ++-- 3 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/Makefile.am b/Makefile.am index ba24343c3a2be9edc94ec817d3709a0c78599847..0b063e1f357174f1eecc0b37243f53e4e5040eba 100644 --- a/Makefile.am +++ b/Makefile.am @@ -192,7 +192,7 @@ endif # BUILD_DBUS_TESTS
endif # HAVE_CHECK
-if HAVE_CMOCKA +if HAVE_CMOCKA_1 non_interactive_cmocka_based_tests = \ nss-srv-tests \ test-find-uid \ @@ -237,7 +237,7 @@ non_interactive_cmocka_based_tests += \ ad_gpo_tests endif
-endif # HAVE_CMOCKA +endif # HAVE_CMOCKA_1
check_PROGRAMS = \ stress-tests \ @@ -245,9 +245,9 @@ check_PROGRAMS = \ $(non_interactive_cmocka_based_tests) \ $(non_interactive_check_based_tests)
-if HAVE_CMOCKA +if HAVE_CMOCKA_1 check_PROGRAMS += dummy-child -endif # HAVE_CMOCKA +endif # HAVE_CMOCKA_1
PYTHON_TESTS =
@@ -1716,7 +1716,7 @@ sss_config_tests_LDADD = \ endif # BUILD_CONFIG_LIB endif # BUILD_IFP
-if HAVE_CMOCKA +if HAVE_CMOCKA_1
TEST_MOCK_RESP_OBJ = \ src/tests/cmocka/common_mock_resp.c \ @@ -2281,7 +2281,7 @@ test_fo_srv_LDADD = \ libsss_test_common.la \ $(NULL)
-endif # HAVE_CMOCKA +endif # HAVE_CMOCKA_1
noinst_PROGRAMS = pam_test_client if BUILD_SUDO diff --git a/src/external/libcmocka.m4 b/src/external/libcmocka.m4 index 740d9ad254b50563d4d1cbcab3d10f5920ffd79f..b45ce49a56b0690010bbd7fc6356d77bffb3dcd4 100644 --- a/src/external/libcmocka.m4 +++ b/src/external/libcmocka.m4 @@ -10,9 +10,24 @@ AC_DEFUN([AM_CHECK_CMOCKA], ) AS_IF([test x"$cmocka_required_headers" != x"no"], [PKG_CHECK_MODULES([CMOCKA], [cmocka], [have_cmocka="yes"])]
)],
)dnl Check if we have the new runner or the legacy oneSAVE_CFLAGS=$CFLAGSSAVE_LIBS=$LIBSCFLAGS="$CFLAGS $CMOCKA_CFLAGS"LIBS="$LIBS $CMOCKA_LIBS"AS_IF([test x"$have_cmocka" == x"yes"],[AC_CHECK_FUNC([_cmocka_run_group_tests], [have_cmocka_1="yes"]]))CFLAGS=$SAVE_CFLAGSLIBS=$SAVE_LIBS], dnl PKG_CHECK_EXISTS ACTION-IF-NOT-FOUND [AC_MSG_WARN([No libcmocka library found, cmocka tests will not be built])]
- AM_CONDITIONAL([HAVE_CMOCKA], [test x$have_cmocka = xyes])
- AM_CONDITIONAL([HAVE_CMOCKA_1], [test x$have_cmocka_1 = xyes])
We agreed to drop support for older version of cmocka. So we can let pkg-config to detect cmocka 1.0 and we do not need special case makefile with HAVE_CMOCKA_1.
The 1st patch can be reduced to next diff dnl A macro to check presence of cmocka on the system AC_DEFUN([AM_CHECK_CMOCKA], [ - PKG_CHECK_EXISTS(cmocka, + PKG_CHECK_EXISTS(cmocka >= 1.0.0, [AC_CHECK_HEADERS([stdarg.h stddef.h setjmp.h], [], dnl We are only intrested in action-if-not-found [AC_MSG_WARN([Header files stdarg.h stddef.h setjmp.h are required by cmocka]) @@ -12,7 +12,7 @@ AC_DEFUN([AM_CHECK_CMOCKA], [PKG_CHECK_MODULES([CMOCKA], [cmocka], [have_cmocka="yes"])] )], dnl PKG_CHECK_EXISTS ACTION-IF-NOT-FOUND - [AC_MSG_WARN([No libcmocka library found, cmocka tests will not be built])] + [AC_MSG_WARN([No libcmocka-1.0.0 library found, cmocka tests will not be built])] ) AM_CONDITIONAL([HAVE_CMOCKA], [test x$have_cmocka = xyes]) ])
ACK to 2nd and 3rd patch.
LS
On Tue, Mar 10, 2015 at 02:52:56PM +0100, Lukas Slebodnik wrote:
On (10/03/15 14:20), Jakub Hrozek wrote:
On Wed, Feb 25, 2015 at 11:20:45PM +0100, Lukas Slebodnik wrote:
I'm not sure we understood each other, I was specifically asking about stabe branch. In master, we would add strict requirements for cmocka 1.0+, convert all tests there and don't run any tests if cmocka 1.0+ is not found.
Agree with strict requirement Agree with converting tests.
See attached patches. CI - http://sssd-ci.duckdns.org/logs/job/9/24/summary.html
In sssd-1-12, we would keep the existing tests untouched and ignore the deprecation warnings. But what if someone submits a patch that needs to be included in sssd-1-12, too but adds a test that is written using cmocka-1.0 API? We could either: a) backport the patch without the test b) backport the patch as-is, but add the Makefile.am part of the patch into a new block that gets executed only if cmocka 1.0 is available. Currently we only have a global HAVE_CMOCKA if-endif.
c) - add strict requirement for cmocka 1.0+ - do not convert tests - ignore warnings caused by deprecated function in cmocka 1.0
Result: * change in stable branch will be small * there will not be problem with backporting new tests * makefile will not be poluted with another if else block * if cmocka-1.0+ is not available then cmocka tests will not be executed
Distributions: arch - does not run tests at all opensuse - does not run tests at all gentoo - does not run cmocka test debian - cmocka tests are disabled (due to failueres on some arch) [1] ubuntu - cmocka tests are disabled (due to failueres on some arch) fedora - cmocka-1.0 will be available epel{6,7} - cmocka-1.0 will be available
[1] http://anonscm.debian.org/cgit/pkg-sssd/sssd.git/commit/?id=ea1575581b86cd32...
cmocka is an optional dependency. So I don't think we need to complicate Makefile in stable branches with with cmocka < 1.0 and cmocka >= 1.0.
I think we should just push the first and second patch to sssd-1-12, then.
From 6cdaf7234c8f1b4e28aeb92129c015ec336c24bb Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 9 Mar 2015 21:25:47 +0100 Subject: [PATCH 1/3] build: Only run cmocka tests if cmocka 1.0 or newer is available
Rename the Makefile.am conditionals to HAVE_CMOCKA_1 to make it clear only the latest release is suported.
Makefile.am | 12 ++++++------ src/external/libcmocka.m4 | 19 +++++++++++++++++-- src/tests/cwrap/Makefile.am | 4 ++-- 3 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/Makefile.am b/Makefile.am index ba24343c3a2be9edc94ec817d3709a0c78599847..0b063e1f357174f1eecc0b37243f53e4e5040eba 100644 --- a/Makefile.am +++ b/Makefile.am @@ -192,7 +192,7 @@ endif # BUILD_DBUS_TESTS
endif # HAVE_CHECK
-if HAVE_CMOCKA +if HAVE_CMOCKA_1 non_interactive_cmocka_based_tests = \ nss-srv-tests \ test-find-uid \ @@ -237,7 +237,7 @@ non_interactive_cmocka_based_tests += \ ad_gpo_tests endif
-endif # HAVE_CMOCKA +endif # HAVE_CMOCKA_1
check_PROGRAMS = \ stress-tests \ @@ -245,9 +245,9 @@ check_PROGRAMS = \ $(non_interactive_cmocka_based_tests) \ $(non_interactive_check_based_tests)
-if HAVE_CMOCKA +if HAVE_CMOCKA_1 check_PROGRAMS += dummy-child -endif # HAVE_CMOCKA +endif # HAVE_CMOCKA_1
PYTHON_TESTS =
@@ -1716,7 +1716,7 @@ sss_config_tests_LDADD = \ endif # BUILD_CONFIG_LIB endif # BUILD_IFP
-if HAVE_CMOCKA +if HAVE_CMOCKA_1
TEST_MOCK_RESP_OBJ = \ src/tests/cmocka/common_mock_resp.c \ @@ -2281,7 +2281,7 @@ test_fo_srv_LDADD = \ libsss_test_common.la \ $(NULL)
-endif # HAVE_CMOCKA +endif # HAVE_CMOCKA_1
noinst_PROGRAMS = pam_test_client if BUILD_SUDO diff --git a/src/external/libcmocka.m4 b/src/external/libcmocka.m4 index 740d9ad254b50563d4d1cbcab3d10f5920ffd79f..b45ce49a56b0690010bbd7fc6356d77bffb3dcd4 100644 --- a/src/external/libcmocka.m4 +++ b/src/external/libcmocka.m4 @@ -10,9 +10,24 @@ AC_DEFUN([AM_CHECK_CMOCKA], ) AS_IF([test x"$cmocka_required_headers" != x"no"], [PKG_CHECK_MODULES([CMOCKA], [cmocka], [have_cmocka="yes"])]
)],
)dnl Check if we have the new runner or the legacy oneSAVE_CFLAGS=$CFLAGSSAVE_LIBS=$LIBSCFLAGS="$CFLAGS $CMOCKA_CFLAGS"LIBS="$LIBS $CMOCKA_LIBS"AS_IF([test x"$have_cmocka" == x"yes"],[AC_CHECK_FUNC([_cmocka_run_group_tests], [have_cmocka_1="yes"]]))CFLAGS=$SAVE_CFLAGSLIBS=$SAVE_LIBS], dnl PKG_CHECK_EXISTS ACTION-IF-NOT-FOUND [AC_MSG_WARN([No libcmocka library found, cmocka tests will not be built])]
- AM_CONDITIONAL([HAVE_CMOCKA], [test x$have_cmocka = xyes])
- AM_CONDITIONAL([HAVE_CMOCKA_1], [test x$have_cmocka_1 = xyes])
We agreed to drop support for older version of cmocka. So we can let pkg-config to detect cmocka 1.0 and we do not need special case makefile with HAVE_CMOCKA_1.
The 1st patch can be reduced to next diff dnl A macro to check presence of cmocka on the system AC_DEFUN([AM_CHECK_CMOCKA], [
- PKG_CHECK_EXISTS(cmocka,
- PKG_CHECK_EXISTS(cmocka >= 1.0.0, [AC_CHECK_HEADERS([stdarg.h stddef.h setjmp.h], [], dnl We are only intrested in action-if-not-found [AC_MSG_WARN([Header files stdarg.h stddef.h setjmp.h are required by cmocka])
@@ -12,7 +12,7 @@ AC_DEFUN([AM_CHECK_CMOCKA], [PKG_CHECK_MODULES([CMOCKA], [cmocka], [have_cmocka="yes"])] )], dnl PKG_CHECK_EXISTS ACTION-IF-NOT-FOUND
[AC_MSG_WARN([No libcmocka library found, cmocka tests will not be built])]
[AC_MSG_WARN([No libcmocka-1.0.0 library found, cmocka tests will not be built])]])
Ah, true. I'm not sure why my first patch was so complex, I think I tried to support both again..anyway, your version is correct.
ACK to 2nd and 3rd patch.
LS
New patches are attached.
On (11/03/15 09:58), Jakub Hrozek wrote:
New patches are attached.
From 4fb6deab618d82efc9097fbc1d3f59f70612922d Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 10 Mar 2015 18:10:37 +0100 Subject: [PATCH 1/3] build: Only run cmocka tests if cmocka 1.0 or newer is available
src/external/libcmocka.m4 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK
From 11eeff3202a04a61ae7b4d450bd705477f6e512c Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 10 Mar 2015 11:02:30 +0100 Subject: [PATCH 2/3] RPM: BuildRequire libcmocka >= 1.0
contrib/sssd.spec.in | 2 +-
ACK
From e29026b4fe09cfbfa1c090e502464972c18ba623 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 9 Mar 2015 21:48:11 +0100 Subject: [PATCH 3/3] tests: convert all unit tests to cmocka 1.0 or later
All tests now use the cmocka-1.0-compatible API.
src/tests/cmocka/sbus_internal_tests.c | 22 +-- src/tests/cmocka/sss_nss_idmap-tests.c | 8 +- src/tests/cmocka/test_ad_access_filter.c | 86 ++++++------ src/tests/cmocka/test_ad_common.c | 22 +-- src/tests/cmocka/test_ad_gpo.c | 58 ++++---- src/tests/cmocka/test_authtok.c | 29 ++-- src/tests/cmocka/test_be_ptask.c | 12 +- src/tests/cmocka/test_child_common.c | 46 ++++--- src/tests/cmocka/test_copy_ccache.c | 14 +- src/tests/cmocka/test_copy_keytab.c | 22 +-- src/tests/cmocka/test_dp_opts.c | 40 +++--- src/tests/cmocka/test_dyndns.c | 45 ++++--- src/tests/cmocka/test_find_uid.c | 10 +- src/tests/cmocka/test_fo_srv.c | 22 +-- src/tests/cmocka/test_fqnames.c | 80 +++++------ src/tests/cmocka/test_ifp.c | 25 ++-- src/tests/cmocka/test_ipa_idmap.c | 16 ++- src/tests/cmocka/test_negcache.c | 39 +++--- src/tests/cmocka/test_nested_groups.c | 16 ++- src/tests/cmocka/test_nss_srv.c | 160 ++++++++++++---------- src/tests/cmocka/test_resolv_fake.c | 16 ++- src/tests/cmocka/test_responder_cache_req.c | 28 ++-- src/tests/cmocka/test_responder_common.c | 34 ++--- src/tests/cmocka/test_sbus_opath.c | 14 +- src/tests/cmocka/test_sdap.c | 76 +++++------ src/tests/cmocka/test_search_bases.c | 14 +- src/tests/cmocka/test_sss_idmap.c | 67 +++++----- src/tests/cmocka/test_sss_sifp.c | 199 ++++++++++++++-------------- src/tests/cmocka/test_sysdb_views.c | 30 +++-- src/tests/cmocka/test_utils.c | 104 ++++++++------- src/tests/cwrap/test_become_user.c | 8 +- src/tests/cwrap/test_responder_common.c | 22 +-- src/tests/cwrap/test_server.c | 10 +- src/tests/cwrap/test_usertools.c | 10 +- 34 files changed, 753 insertions(+), 651 deletions(-)
I can one unfixed test and thus warning:
src/tests/cmocka/test_io.c: In function ‘main’: src/tests/cmocka/test_io.c:238:5: warning: ‘_run_tests’ is deprecated [-Wdeprecated-declarations] return run_tests(tests); ^ In file included from src/tests/cmocka/test_io.c:33:0: /usr/include/cmocka.h:2001:34: note: declared here CMOCKA_DEPRECATED int _run_tests(const UnitTest * const tests, ^
LS
On Wed, Mar 11, 2015 at 10:17:27AM +0100, Lukas Slebodnik wrote:
I can one unfixed test and thus warning:
src/tests/cmocka/test_io.c: In function ‘main’: src/tests/cmocka/test_io.c:238:5: warning: ‘_run_tests’ is deprecated [-Wdeprecated-declarations] return run_tests(tests); ^ In file included from src/tests/cmocka/test_io.c:33:0: /usr/include/cmocka.h:2001:34: note: declared here CMOCKA_DEPRECATED int _run_tests(const UnitTest * const tests, ^
LS
Let's try this again..
On (11/03/15 10:45), Jakub Hrozek wrote:
On Wed, Mar 11, 2015 at 10:17:27AM +0100, Lukas Slebodnik wrote:
I can one unfixed test and thus warning:
src/tests/cmocka/test_io.c: In function ‘main’: src/tests/cmocka/test_io.c:238:5: warning: ‘_run_tests’ is deprecated [-Wdeprecated-declarations] return run_tests(tests); ^ In file included from src/tests/cmocka/test_io.c:33:0: /usr/include/cmocka.h:2001:34: note: declared here CMOCKA_DEPRECATED int _run_tests(const UnitTest * const tests, ^
LS
Let's try this again..
ACK
CI tests passed. I cannot paste link because "sssd-ci.duckdns.org" is down
LS
On Wed, Mar 11, 2015 at 01:31:36PM +0100, Lukas Slebodnik wrote:
On (11/03/15 10:45), Jakub Hrozek wrote:
On Wed, Mar 11, 2015 at 10:17:27AM +0100, Lukas Slebodnik wrote:
I can one unfixed test and thus warning:
src/tests/cmocka/test_io.c: In function ‘main’: src/tests/cmocka/test_io.c:238:5: warning: ‘_run_tests’ is deprecated [-Wdeprecated-declarations] return run_tests(tests); ^ In file included from src/tests/cmocka/test_io.c:33:0: /usr/include/cmocka.h:2001:34: note: declared here CMOCKA_DEPRECATED int _run_tests(const UnitTest * const tests, ^
LS
Let's try this again..
ACK
CI tests passed. I cannot paste link because "sssd-ci.duckdns.org" is down
LS
master: * 4e5e846de22407f825fe3b4040d79606818a2419 * 0aad066baeed6833cf061b71d72c6bbbb2d2b9e8 * cecee447d41c3ca22e94880a7d0cbd910f230fe5
On (11/03/15 14:42), Jakub Hrozek wrote:
On Wed, Mar 11, 2015 at 01:31:36PM +0100, Lukas Slebodnik wrote:
On (11/03/15 10:45), Jakub Hrozek wrote:
On Wed, Mar 11, 2015 at 10:17:27AM +0100, Lukas Slebodnik wrote:
I can one unfixed test and thus warning:
src/tests/cmocka/test_io.c: In function ‘main’: src/tests/cmocka/test_io.c:238:5: warning: ‘_run_tests’ is deprecated [-Wdeprecated-declarations] return run_tests(tests); ^ In file included from src/tests/cmocka/test_io.c:33:0: /usr/include/cmocka.h:2001:34: note: declared here CMOCKA_DEPRECATED int _run_tests(const UnitTest * const tests, ^
LS
Let's try this again..
ACK
CI tests passed. I cannot paste link because "sssd-ci.duckdns.org" is down
LS
master:
- 4e5e846de22407f825fe3b4040d79606818a2419
Attached is a patch wich applies to 1.12 branch.
- 0aad066baeed6833cf061b71d72c6bbbb2d2b9e8
- cecee447d41c3ca22e94880a7d0cbd910f230fe5
^^^ These two patches applies without any problem.
LS
On Tue, Mar 24, 2015 at 10:39:47AM +0100, Lukas Slebodnik wrote:
master:
- 4e5e846de22407f825fe3b4040d79606818a2419
Attached is a patch wich applies to 1.12 branch.
- 0aad066baeed6833cf061b71d72c6bbbb2d2b9e8
- cecee447d41c3ca22e94880a7d0cbd910f230fe5
^^^ These two patches applies without any problem.
LS
* sssd-1-12: * 8d1ae1f350f5618f1d8f00211d6db6b025e52dd6 * 1acadc807209b24f681a4866812e11d04b5a129f * 2bbf29b268bd2e7f9399ad6d7f5be6ca73ef7223
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Nikolai Kondrashov -
Pavel Březina