On (25/05/15 17:55), Nikolai Kondrashov wrote:
On 05/25/2015 11:44 AM, Lukas Slebodnik wrote:
>A)
>The 1st patch is not necessary.
>CI passed without 1st patch
>http://sssd-ci.duckdns.org/logs/job/15/89/summary.html (fedora 20 failed due to
>dyndnd-test)
>
>B) You are misusing autoconf macro AM_CONDITIONAL.
>It shoudl be used just in case you want to have "if conditions" in automake
>files. ENABLE_INTGCHECK
>
>Please try to look into documentation of autoconf macros
>AC_SUBST
>AC_DEFINE (AC_DEFINE_UNQUOTED)
>AM_CONDITIONAL
>
>In most cases you just need just AC_SUBST for HAVE_$sth
>or just AC_CHECK_PROG.
Yes this can make things simpler and more reliable. I remade the configuration
to just use shell variables, as AC_SUBST is not necessary, because we don't
use these in any configure-processed files (e.g. Makefile.am's).
Please see the first attached patch to see what I mean.
I did few other improvements in autoconf code.
@see attached diff
>This also reminds me you can squas
>"small external" files into src/external/intgcheck.m4
>It does not worth to have one file just with a line AC_CHECK_PROG
>
> src/external/fakeroot.m4 | 2 +
> src/external/intgcheck.m4 | 23 ++++
> src/external/ldap.m4 | 6 +
> src/external/pytest.m4 | 3 +
Regarding this, wouldn't it make the corresponding macros somewhat harder to
find? We have a few short files under src/external with clear names and
contents that doesn't quite fit anywhere and I thought that rather neat.
I can stuff fakeroot and py.test checks into intgcheck.m4 without much harm, I
think. However, would it really be good to move slapd and ldapmodify checking
out of ldap.m4? Wouldn't it be confusing?
ok, detection of ldap command can stay in ldap.m4
>@see
>diff --git a/src/external/pytest.m4 b/src/external/pytest.m4
>index 00f28e5..b6305d1 100644
>--- a/src/external/pytest.m4
>+++ b/src/external/pytest.m4
>@@ -1,3 +1,2 @@
> AC_PATH_PROG([PYTEST], [py.test])
>-AC_SUBST([PYTEST])
>-AM_CONDITIONAL([HAVE_PYTEST], [test x"$PYTEST" != x])
>+AC_SUBST([HAVE_PYTEST], [test x"$PYTEST" != x])
Hmm, would the last line really work? To me it looks like not, am I missing
something?
>C) We can simplify make target intgcheck
>It significantly improve user experince.
>(the same as "make distcheck" or make tests in samba)
Ah, good catch, yes. Thank you, Lukas. However, leaving the configure option
named "--enable-intgcheck" will be confusing, so I tried to rename it to
something more relevant to what it will be doing now. It still feels a bit
awkward, but perhaps it's good enough.
Please take a look at the second attached patch, for illustration.
You can also change related parts in CI script
>D) please find better name for "src/tests/intg/misc.py"
I think "misc.py" is perfectly fine, but since our previous discussion on
similar matter stalled, please suggest a name and I'll name it as you say and
we'll be done with it.
It's better to do not mix utility functions.
So feel free to split file to subfiles if it will be used among many files
or include helper function directly to module. You can later extract helper
function to another utility module when there will be use case.
(run_shell is not used atm)
And here are proposed names. string_utils | utils_string | utils.string,
shell_utils (python already has shutil) ...
>I will file other tickets with improvements after pushing patch to
master.
Thank you, Lukas.
On (25/05/15 18:26), Nikolai Kondrashov wrote:
On 05/25/2015 05:55 PM, Nikolai Kondrashov wrote:
>On 05/25/2015 11:44 AM, Lukas Slebodnik wrote:
>>BTW there is a failed intgtest
>>http://sssd-ci.duckdns.org/logs/job/15/90/fedora20/ci-build-debug/ci-make-intgcheck.log
>>It would be good to prevent such fialures. I've already sent patches to fix
>>negcache-test and SSSDConfig-test. We do not need to have another problematic
test
>
>Argh, I've seen this, wanted to fix it and forgot. Can't reproduce it at the
>moment, but I'll keep trying. Meanwhile, do you have any ideas of what might
>be causing this?
Ah, nevermind, got confused, figured it out now. Thanks.
I have no idea what caused failure, but I'm glad you fiure it out.
It might be better idea to do nat catch excetion (or rethrow exception)
to see full stack trace with failure.
From 9978cc3b56e6e664384deeff2a497ccb377e06d3 Mon Sep 17 00:00:00
2001
From: Nikolai Kondrashov <Nikolai.Kondrashov(a)redhat.com>
Date: Mon, 25 May 2015 15:39:08 +0300
Subject: [PATCH 1/2] intgcheck: Use shell variables directly for dep checks
---
configure.ac | 3 ---
src/external/cwrap.m4 | 2 ++
src/external/fakeroot.m4 | 3 +--
src/external/intgcheck.m4 | 4 ++--
src/external/ldap.m4 | 9 ++++++---
src/external/pytest.m4 | 6 +++++-
6 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/src/external/ldap.m4 b/src/external/ldap.m4
index be71f4c..6ef8f09 100644
--- a/src/external/ldap.m4
+++ b/src/external/ldap.m4
@@ -93,6 +93,9 @@ LIBS=$SAVE_LIBS
AC_PATH_PROG([SLAPD], [slapd], ,
[$PATH$PATH_SEPARATOR/usr/sbin$PATH_SEPARATOR])
AC_SUBST([SLAPD])
AC_PATH_PROG uses AC_SUBST so you needn't it here.
-AM_CONDITIONAL([HAVE_SLAPD], [test x"$SLAPD" != x])
-AC_CHECK_PROG([HAVE_LDAPMODIFY], [ldapmodify], [true], [false])
-AM_CONDITIONAL([HAVE_LDAPMODIFY], [$HAVE_LDAPMODIFY])
+if test -n "$SLAPD"; then
+ HAVE_SLAPD=yes
+else
+ HAVE_SLAPD=no
+fi
From cbd044012654be11991f8af0ff5d97658eaa5841 Mon Sep 17 00:00:00
2001
From: Nikolai Kondrashov <Nikolai.Kondrashov(a)redhat.com>
Date: Mon, 25 May 2015 17:21:06 +0300
Subject: [PATCH 2/2] intgcheck: Rename --enable-intgcheck to
--enable-intgcheck-reqs
---
Makefile.am | 3 +--
configure.ac | 2 +-
src/external/intgcheck.m4 | 11 +++++------
3 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/external/intgcheck.m4 b/src/external/intgcheck.m4
index b1efd30..bd2c71b 100644
--- a/src/external/intgcheck.m4
+++ b/src/external/intgcheck.m4
@@ -1,12 +1,12 @@
AC_DEFUN([AM_INTGCHECK_REQ], [
if test x"$$1" != xyes; then
- AC_MSG_ERROR([cannot enable integration tests: $2 not found])
+ AC_MSG_ERROR([cannot run integration tests: $2 not found])
fi
])
-AC_DEFUN([AM_ENABLE_INTGCHECK], [
Feel free to rename macro but it shoudl not have
a prefix AC_ or
AM_. It is a "namespace" for macros from autoconf or automake.
- AC_ARG_ENABLE(intgcheck,
- [AS_HELP_STRING([--enable-intgcheck],
- [enable integration test support (intgcheck target)
[default=no]])],
+AC_DEFUN([AM_ENABLE_INTGCHECK_REQS], [
+ AC_ARG_ENABLE(intgcheck-reqs,
+ [AS_HELP_STRING([--enable-intgcheck-reqs],
+ [enable checking for integration test requirements
[default=no]])],
enable_intgcheck="$enableval",
enable_intgcheck="no")
if test x"$enable_intgcheck" == xyes; then
@@ -19,5 +19,4 @@ AC_DEFUN([AM_ENABLE_INTGCHECK], [
AM_INTGCHECK_REQ([HAVE_PYTEST], [pytest])
AM_INTGCHECK_REQ([HAVE_PY2MOD_LDAP], [python-ldap])
^^^^
Such might look good but if there is a longer 1st argument
you will need to reindent it.
BTW it causes many pep8 warnings in python code.