[SSSD] [PATCH v3] Remove trailing whitespace

Lukas Slebodnik lslebodn at redhat.com
Mon Aug 31 06:59:45 UTC 2015


On (27/08/15 10:56), Lukas Slebodnik wrote:
>On (18/08/15 15:31), Nikolai Kondrashov wrote:
>>On 08/17/2015 08:42 PM, Lukas Slebodnik wrote:
>>>----- Original Message -----
>>>>From: "Nikolai Kondrashov" <Nikolai.Kondrashov at redhat.com>
>>>>To: "Development of the System Security Services Daemon" <sssd-devel at lists.fedorahosted.org>
>>>>Sent: Monday, August 17, 2015 6:40:36 PM
>>>>Subject: [SSSD] [PATCH v3] Remove trailing whitespace
>>>>
>>>>On 08/17/2015 09:37 AM, Lukas Slebodnik wrote:
>>>>>On (13/08/15 15:48), Nikolai Kondrashov wrote:
>>>>>>On 08/13/2015 02:57 PM, Lukas Slebodnik wrote:
>>>>>>>On (13/08/15 14:47), Nikolai Kondrashov wrote:
>>>>>>>>+if GIT_CHECKOUT
>>>>>>>>+export GIT_DIR=$(abs_top_srcdir)/.git
>>>>>>>>+export GIT_WORK_TREE=$(abs_top_srcdir)
>>>>>>>>+endif
>>>>>>>It's not good idea to export such in general code.
>>>>>>>you might greak something. I didn't try but you might break
>>>>>>>something in fedpkg + local build.
>>>>>>>
>>>>>>>Better solution would to to export make variable abs_top_srcdir
>>>>>>>as ABS_TOP_SRCDIR in TESTS_ENVIRONMENT
>>>>>>>
>>>>>>>and print warnig/error from test if this variable is not defined.
>>>>>>
>>>>>>Yeah, it's not nice. I doubt that it will break anything as it is very
>>>>>>specific as to where the git repo is, but I wouldn't vouch for it.
>>>>>>
>>>>>But on the other hand ABS_TOP_SRCDIR is more abstract
>>>>>and there is higher probability it will be used in other tests.
>>>>>
>>>>>>We'll need to pass it both to the TESTS_ENVIRONMENT and to *srpm targets
>>>>>>as
>>>>>>well, which will make it uglier, but I'll do it.
>>>>>>
>>>>>You can still export GIT_DIR and GIT_WORK_TREE inside script if
>>>>>ABS_TOP_SRCDIR is defined. Otherwise test can be skipped
>>>>>https://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html
>>>>
>>>>Alright here's another version.
>>>>
>>>>I didn't change it much. Simply moved export of GIT_DIR and GIT_WORK_TREE
>>>>lower, to TESTS_ENVIRONMENT and *rpm* targets. I didn't add exporting of
>>>>ABS_TOP_SRCDIR, because that way the whitespace test would always require it,
>>>>making manual execution inconvenient, or would need additional logic with
>>>>assumptions there. This way we avoid having another variable and a way to
>>>>pass
>>>>build parameters around.
>>>>
>>>>BTW, Lukas, could you please describe what kind of problems you expect in
>>>>"fedpkg + local build"? Perhaps it's not a problem after all?
>>>>
>>>
>>>fedpkg compile
>>>
>>>usage: fedpkg compile [-h] [--arch ARCH] [--short-circuit]
>>>
>>>This command calls rpmbuild to compile the source. By default the prep and configure
>>>stages will be done as well, unless the short-circuit option is used
>>>
>>>And where I can see a problem.
>>>fedpkg is a git wrapper. The git contains patches + spec file.
>>>
>>>If you call "fedpkg compile" then rpmbuild will extract tarball and apply patches.
>>>configure will detect there is a git and the test will fail because
>>>patches contains trailing whitespaces.
>>>
>>>So GIT_WORK_TREE is not an equivalent to ABS_TOP_SRCDIR.
>>>This is an example of for fedora.
>>>There might be other distribution where it can cause troubles as well.
>>>So it's better to be defensive.
>>
>>Thank you, Lukas. I understand your concern. However, the second patch in the
>>series specifically makes configure detect git repo root at the srcdir only.
>>So, if srcdir is in a subdirectory of a git repo and is not a git repo itself
>>(which would be fine, BTW), configure won't detect it.
>>
>I haven't noticed the 2nd patch.
>IMHO it's just an unnecessary complication and just decrease readability
>of Makefile.
>
>The solution witn ABS_TOP_SRCDIR is much simpler and more elegant.
>Adding ABS_TOP_SRCDIR to TESTS_ENVIRONMENT is just one line change
>and does not require changes in configure.ac. The main purpose of detecting git
>is to obtain git hash for prerelase version. We should try to avoid
>(ab)using GIT features in makefile.
>
>Enviroment variable ABS_TOP_SRCDIR is more abstract and reusable.
>It can be used even in other tests where git would not be used/required.
>
>BTW if git is not available the test should not PASS. It should be skipped.
>https://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html#Scripts_002dbased-Testsuites
>
>"""
>When no test protocol is in use, an exit status of 0 from a test script
>will denote a success, an exit status of 77 a skipped test, an exit status
>of 99 an hard error, and any other exit status will denote a failure.
>"""
>
>>The only problem I see at the moment is Debian. They store the packaging
>>together with the source repo [1]. However, we can't do anything about that
>>regardless whether we use ABS_TOP_SRCDIR or not, and they should be able to
>>add "debian/.*" to the exclude patterns of the whitespace test.
>>
>There should not be complication with ordinary files in directory debian.
>The problem is with patches and diff files.
>
>Just try.
>sh$ git format-patch -1
>0001-NSS-Fix-use-after-free.patch
>sh$ grep "\s\+$" *.patch | wc -l
>4
>
>We decided to have blacklist for problematic files and not whitelist
>which would be longer. We should add "**/*.patch and **/*.diff" to blacklist
>as well. It's not just a problem of debian. We had patches in git as part of
>experimental features. @see commit ac54a88b4b510289a411f334e371282d00e1538d
>
It seems that Nikolai is busy and does not have a time to update patches.

Changes:
* simplified build system
* diff files and patches were added to balck list

LS
-------------- next part --------------
>From dd2863f562ae9c7118ecd7ad9c99f4fb0c4735f5 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <preichl at redhat.com>
Date: Tue, 11 Aug 2015 04:54:50 -0400
Subject: [PATCH 1/2] Remove trailing whitespace

---
 README                              | 2 +-
 src/conf_macros.m4                  | 4 ++--
 src/external/glib.m4                | 4 ++--
 src/external/pkg.m4                 | 6 +++---
 src/man/sssd.conf.5.xml             | 8 ++++----
 src/sss_client/ssh/sss_ssh_client.c | 6 +++---
 src/tests/pyhbac-test.py            | 1 -
 src/util/dlinklist.h                | 8 ++++----
 src/util/signal.c                   | 8 ++++----
 9 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/README b/README
index ed08970cee4652626067b628b3a17f3370a73e56..189f66fe5b20c85728ac2640d734e3b490e23817 100644
--- a/README
+++ b/README
@@ -9,7 +9,7 @@
   an NSS and PAM interface toward the system and a pluggable backend system
   to connect to multiple different account sources.
 
-  More information about SSSD can be found on its project page - 
+  More information about SSSD can be found on its project page -
   <http://fedorahosted.org/sssd/>
 
   Building and installation
diff --git a/src/conf_macros.m4 b/src/conf_macros.m4
index c6ce00059094aa0359f6af9efefe7c7438c1e28f..cd731e605b153f69931d3cb0764f10801a596f96 100644
--- a/src/conf_macros.m4
+++ b/src/conf_macros.m4
@@ -596,11 +596,11 @@ AC_DEFUN([WITH_UNICODE_LIB],
     if test x"$with_unicode_lib" != x; then
         unicode_lib=$with_unicode_lib
     fi
-    
+
     if test x"$unicode_lib" != x"libunistring" -a x"$unicode_lib" != x"glib2"; then
 		AC_MSG_ERROR([Unsupported unicode library])
     fi
-    
+
     AM_CONDITIONAL([WITH_LIBUNISTRING], test x"$unicode_lib" = x"libunistring")
     AM_CONDITIONAL([WITH_GLIB], test x"$unicode_lib" = x"glib2")
   ])
diff --git a/src/external/glib.m4 b/src/external/glib.m4
index 8cec763263cd86d4ff1c45a7a3e2afe12954fe1c..3db25136b685bb6be05d1e9b648f032eaeaa1ec5 100644
--- a/src/external/glib.m4
+++ b/src/external/glib.m4
@@ -3,9 +3,9 @@ PKG_CHECK_MODULES([GLIB2],[glib-2.0])
 if test x$has_glib2 != xno; then
     SAFE_LIBS="$LIBS"
     LIBS="$GLIB2_LIBS"
-    
+
     AC_CHECK_FUNC([g_utf8_validate],
                   AC_DEFINE([HAVE_G_UTF8_VALIDATE], [1],
                             [Define if g_utf8_validate exists]))
     LIBS="$SAFE_LIBS"
-fi
\ No newline at end of file
+fi
diff --git a/src/external/pkg.m4 b/src/external/pkg.m4
index a8b3d06c8192229aab14cdc643a8be8f789376fd..568127f104de92fc81fa9b9ccf2fbd2d470840a1 100644
--- a/src/external/pkg.m4
+++ b/src/external/pkg.m4
@@ -1,5 +1,5 @@
 # pkg.m4 - Macros to locate and utilise pkg-config.            -*- Autoconf -*-
-# 
+#
 # Copyright © 2004 Scott James Remnant <scott at netsplit.com>.
 #
 # This program is free software; you can redistribute it and/or modify
@@ -38,7 +38,7 @@ if test -n "$PKG_CONFIG"; then
 		AC_MSG_RESULT([no])
 		PKG_CONFIG=""
 	fi
-		
+
 fi[]dnl
 ])# PKG_PROG_PKG_CONFIG
 
@@ -119,7 +119,7 @@ if test $pkg_failed = yes; then
         _PKG_SHORT_ERRORS_SUPPORTED
         if test $_pkg_short_errors_supported = yes; then
 	        $1[]_PKG_ERRORS=`$PKG_CONFIG --short-errors --errors-to-stdout --print-errors "$2"`
-        else 
+        else
 	        $1[]_PKG_ERRORS=`$PKG_CONFIG --errors-to-stdout --print-errors "$2"`
         fi
 	# Put the nasty error message in config.log where it belongs
diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
index 3f57862c61be662f4c06caa2022d3b49460ee7e1..b858347f8f51cfb512a740fa0f31027d7b7ecebd 100644
--- a/src/man/sssd.conf.5.xml
+++ b/src/man/sssd.conf.5.xml
@@ -1304,7 +1304,7 @@ pam_account_expired_message = Account expired, please call help desk.
                         </para>
                     </listitem>
                 </varlistentry>
-                
+
                 <varlistentry>
                     <term>entry_cache_user_timeout (integer)</term>
                     <listitem>
@@ -1317,7 +1317,7 @@ pam_account_expired_message = Account expired, please call help desk.
                         </para>
                     </listitem>
                 </varlistentry>
-                
+
                 <varlistentry>
                     <term>entry_cache_group_timeout (integer)</term>
                     <listitem>
@@ -1330,7 +1330,7 @@ pam_account_expired_message = Account expired, please call help desk.
                         </para>
                     </listitem>
                 </varlistentry>
-                
+
                 <varlistentry>
                     <term>entry_cache_netgroup_timeout (integer)</term>
                     <listitem>
@@ -1343,7 +1343,7 @@ pam_account_expired_message = Account expired, please call help desk.
                         </para>
                     </listitem>
                 </varlistentry>
-                
+
                 <varlistentry>
                     <term>entry_cache_service_timeout (integer)</term>
                     <listitem>
diff --git a/src/sss_client/ssh/sss_ssh_client.c b/src/sss_client/ssh/sss_ssh_client.c
index 245a0205685abfa553f6264942e97b3d556262bb..a24c5195da7a60d2fe2349e6826f2f70cfb9068a 100644
--- a/src/sss_client/ssh/sss_ssh_client.c
+++ b/src/sss_client/ssh/sss_ssh_client.c
@@ -69,7 +69,7 @@ int set_locale(void)
 }
 
 /* SSH public key request:
- * 
+ *
  * header:
  *   0..3: flags (unsigned int, must be combination of SSS_SSH_REQ_* flags)
  *   4..7: name length (unsigned int)
@@ -80,9 +80,9 @@ int set_locale(void)
  * domain (ony included if flags & SSS_SSH_REQ_DOMAIN):
  *   0..3: domain length (unsigned int, 0 means default domain)
  *   4..X: domain (null-terminated UTF-8 string)
- * 
+ *
  * SSH public key reply:
- * 
+ *
  * header:
  *   0..3: number of results (unsigned int)
  *   4..7: reserved (unsigned int, must be 0)
diff --git a/src/tests/pyhbac-test.py b/src/tests/pyhbac-test.py
index 83958d7bffcccea375c79166ee7dfca6f9956cff..9d8fd1a333bf54ecf21d14d3b6293f7294a0d53e 100755
--- a/src/tests/pyhbac-test.py
+++ b/src/tests/pyhbac-test.py
@@ -137,7 +137,6 @@ class PyHbacRuleElementTest(unittest.TestCase):
         el.names = ['foo']
         el.groups = ['bar, baz']
         self.assertEquals(el.__repr__(), u'<category 1 names [foo] groups [bar, baz]>')
-        
 
 class PyHbacRuleTest(unittest.TestCase):
     def testRuleGetSetName(self):
diff --git a/src/util/dlinklist.h b/src/util/dlinklist.h
index 413084888b8dbdb9faef4b0011d030766b8a2965..ef09661f4be4d6e973173887f7790f214bf0f1dd 100644
--- a/src/util/dlinklist.h
+++ b/src/util/dlinklist.h
@@ -1,18 +1,18 @@
-/* 
+/*
    Unix SMB/CIFS implementation.
    some simple double linked list macros
    Copyright (C) Andrew Tridgell 1998
-   
+
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
    the Free Software Foundation; either version 3 of the License, or
    (at your option) any later version.
-   
+
    This program is distributed in the hope that it will be useful,
    but WITHOUT ANY WARRANTY; without even the implied warranty of
    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
    GNU General Public License for more details.
-   
+
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */
diff --git a/src/util/signal.c b/src/util/signal.c
index bb8f8bef7681c0140671a3076a950a773f991b66..8acdccaeb11ee171050db3aec4095459d9574a2c 100644
--- a/src/util/signal.c
+++ b/src/util/signal.c
@@ -1,19 +1,19 @@
-/* 
+/*
    Unix SMB/CIFS implementation.
    signal handling functions
 
    Copyright (C) Andrew Tridgell 1998
-   
+
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
    the Free Software Foundation; either version 3 of the License, or
    (at your option) any later version.
-   
+
    This program is distributed in the hope that it will be useful,
    but WITHOUT ANY WARRANTY; without even the implied warranty of
    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
    GNU General Public License for more details.
-   
+
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */
-- 
2.5.0

-------------- next part --------------
>From 5bfe59fcd13f76e2b4ac2b1e27d158f18620c044 Mon Sep 17 00:00:00 2001
From: Nikolai Kondrashov <Nikolai.Kondrashov at redhat.com>
Date: Mon, 10 Aug 2015 13:05:37 -0400
Subject: [PATCH 2/2] TESTS: Add trailing whitespace test

Signed-off-by: Lukas Slebodnik <lslebodn at redhat.com>
---
 Makefile.am               |  6 +++++-
 src/tests/whitespace_test | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100755 src/tests/whitespace_test

diff --git a/Makefile.am b/Makefile.am
index f153ab0adf390880672a1681b386ea26426465cb..b10555919120aaf0bf601d62242539d3a5d0cd6d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -284,7 +284,9 @@ TEST_EXTENSIONS = .sh
 TESTS = \
     $(PYTHON_TESTS) \
     $(non_interactive_cmocka_based_tests) \
-    $(non_interactive_check_based_tests)
+    $(non_interactive_check_based_tests) \
+    src/tests/whitespace_test \
+    $(NULL)
 
 sssdlib_LTLIBRARIES = \
     libsss_ldap.la \
@@ -367,6 +369,7 @@ dist_noinst_SCRIPTS = \
     src/tests/pysss_murmur-test.py2.sh \
     src/tests/pysss_murmur-test.py3.sh \
     src/tests/python-test.py \
+    src/tests/whitespace_test \
     src/tests/krb5_proxy_check_test_data.conf \
     $(NULL)
 
@@ -1379,6 +1382,7 @@ endif
 #################
 TESTS_ENVIRONMENT = LDB_MODULES_PATH=$(abs_top_builddir)/ldb_mod_test_dir \
                     SSS_TEST_DIR=$(TEST_DIR) \
+                    ABS_TOP_SRCDIR=$(abs_top_srcdir) \
                     $(AUX_TESTS_ENVIRONMENT)
 
 ldb_mod_test_dir: memberof.la
diff --git a/src/tests/whitespace_test b/src/tests/whitespace_test
new file mode 100755
index 0000000000000000000000000000000000000000..508fe2d532cc6a3967af8eaa5c7dc4dccdc653f3
--- /dev/null
+++ b/src/tests/whitespace_test
@@ -0,0 +1,32 @@
+#!/bin/bash
+
+set -e -u -o pipefail
+
+# An AWK regex matching tracked file paths to be excluded from the search.
+# Example: '.*\.po|README'
+PATH_EXCLUDE_REGEX='.*\.po|.*\.patch|.*\.diff'
+
+export GIT_DIR="$ABS_TOP_SRCDIR/.git"
+export GIT_WORK_TREE="$ABS_TOP_SRCDIR"
+
+if [ ! -d "$GIT_DIR" ]; then
+    echo "Git repository is required for this test!"
+    exit 77
+fi
+
+git grep -n -I '\s\+$' -- "$(git rev-parse --show-toplevel)" |
+    awk -- "
+        BEGIN {
+            found = 0
+        }
+        ! /^($PATH_EXCLUDE_REGEX):/ {
+            if (!found) {
+                print \"Trailing whitespace found:\"
+                found = 1
+            }
+            print
+        }
+        END {
+            exit found
+        }
+    "
-- 
2.5.0



More information about the sssd-devel mailing list