[PATCH] Remove trailing whitespace

[Pavel Reichl]

Hello,

please see trivial patch attached. I noticed some whitespace in source 
file, so I did some grepping and decided to remove it as we already were 
almost 100 % trailing whitespace free.

Thanks

[Lukas Slebodnik]

On (10/08/15 14:44), Pavel Reichl wrote:
> Hello,
>
> please see trivial patch attached. I noticed some whitespace in source file,
> so I did some grepping and decided to remove it as we already were almost 100
> % trailing whitespace free.

I would prefer if you could write a test.
So it will be checked as a part of "make check"

LS

[Pavel Reichl]

On 08/10/2015 02:52 PM, Lukas Slebodnik wrote:
> On (10/08/15 14:44), Pavel Reichl wrote:
> > Hello,
> >
> > please see trivial patch attached. I noticed some whitespace in source file,
> > so I did some grepping and decided to remove it as we already were almost 100
> > % trailing whitespace free.
>
> I would prefer if you could write a test.
> So it will be checked as a part of "make check"

Yes, I already put on my todo list, but I consider it as independent task.
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (10/08/15 14:54), Pavel Reichl wrote:
> On 08/10/2015 02:52 PM, Lukas Slebodnik wrote:
> > On (10/08/15 14:44), Pavel Reichl wrote:
> > > Hello,
> > >
> > > please see trivial patch attached. I noticed some whitespace in source file,
> > > so I did some grepping and decided to remove it as we already were almost 100
> > > % trailing whitespace free.
> >
> > I would prefer if you could write a test.
> > So it will be checked as a part of "make check"
>
> Yes, I already put on my todo list, but I consider it as independent task.

Yes,

but patch is not very useful without the test.
Because we can still accidentally add trailing white spaces.

I would rather wait until you write the test then we can push them
together.

Conditional NACK

LS

[Pavel Reichl]

On 08/10/2015 03:00 PM, Lukas Slebodnik wrote:
> On (10/08/15 14:54), Pavel Reichl wrote:
> > On 08/10/2015 02:52 PM, Lukas Slebodnik wrote:
> > > On (10/08/15 14:44), Pavel Reichl wrote:
> > > > Hello,
> > > >
> > > > please see trivial patch attached. I noticed some whitespace in source file,
> > > > so I did some grepping and decided to remove it as we already were almost 100
> > > > % trailing whitespace free.
> > >
> > > I would prefer if you could write a test.
> > > So it will be checked as a part of "make check"
> >
> > Yes, I already put on my todo list, but I consider it as independent task.
>
> Yes,
>
> but patch is not very useful without the test.
> Because we can still accidentally add trailing white spaces.
>
> I would rather wait until you write the test then we can push them
> together.
>
> Conditional NACK
>
> LS
> _________

Please see updated patch set attached.

[Lukas Slebodnik]

On (10/08/15 19:31), Pavel Reichl wrote:
> On 08/10/2015 03:00 PM, Lukas Slebodnik wrote:
> > On (10/08/15 14:54), Pavel Reichl wrote:
> > > On 08/10/2015 02:52 PM, Lukas Slebodnik wrote:
> > > > On (10/08/15 14:44), Pavel Reichl wrote:
> > > > > Hello,
> > > > >
> > > > > please see trivial patch attached. I noticed some whitespace in source file,
> > > > > so I did some grepping and decided to remove it as we already were almost 100
> > > > > % trailing whitespace free.
> > > >
> > > > I would prefer if you could write a test.
> > > > So it will be checked as a part of "make check"
> > >
> > > Yes, I already put on my todo list, but I consider it as independent task.
> >
> > Yes,
> >
> > but patch is not very useful without the test.
> > Because we can still accidentally add trailing white spaces.
> >
> > I would rather wait until you write the test then we can push them
> > together.
> >
> > Conditional NACK
> >
> > LS
> > _________
>
> Please see updated patch set attached.
> From 108b0f676b5b57df2559eaa106527ce005921eef Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Mon, 10 Aug 2015 13:06:37 -0400
> Subject: [PATCH 1/2] Remove trailing whitespace
>
> README                              | 2 +-
> po/ca.po                            | 2 +-

We should not manually edit translation files.
Please and exception for these files.
> 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 ++++----
> 10 files changed, 24 insertions(+), 25 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/po/ca.po b/po/ca.po
> index 9acddec2cd545f70cd63325d2a20912ca7684418..9d55fc5333bf5d151295ab7df0808ce77d1c27d4 100644
> --- a/po/ca.po
> +++ b/po/ca.po
> @@ -1,7 +1,7 @@
> # SOME DESCRIPTIVE TITLE.
> # Copyright (C) YEAR Red Hat, Inc.
> # This file is distributed under the same license as the PACKAGE package.
> -# 
> +#
> # Translators:
> # muzzol , 2012
> # muzzol , 2012
> 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 .
> #
> # 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.4.3
> From fed22f881311c3809bfc32b1aa45c04b4362f20a Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Mon, 10 Aug 2015 13:05:37 -0400
> Subject: [PATCH 2/2] TESTS: trailing whitespace test
>
> Makefile.am                  |  8 +++++++-
> src/tests/whitespace_test.sh | 11 +++++++++++
> 2 files changed, 18 insertions(+), 1 deletion(-)
> create mode 100755 src/tests/whitespace_test.sh

Trivial test should be part of bug fix.
So we will not forget to backport it.

It make sense to have a test in separate patch for big features
which are tracked by upstream ticket.
> diff --git a/Makefile.am b/Makefile.am
> index 8b64317d6dce9a1ee8614916395b9afd9f11f382..b5af3e665d3bad32d01911ee1a50c348c1256a84 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -264,6 +264,9 @@ if HAVE_CMOCKA
> check_PROGRAMS += dummy-child
> endif # HAVE_CMOCKA
>
> +CODE_STYLE_TESTS = src/tests/whitespace_test.sh \
>               $(NULL)
>
> PYTHON_TESTS =
>
> if BUILD_PYTHON2_BINDINGS
> @@ -283,7 +286,9 @@ TEST_EXTENSIONS = .sh
> TESTS = \
>     $(PYTHON_TESTS) \
>     $(non_interactive_cmocka_based_tests) \
> $(non_interactive_check_based_tests)
> $(non_interactive_check_based_tests) \
> $(CODE_STYLE_TESTS) \
> $(NULL)
>
> sssdlib_LTLIBRARIES = \
>     libsss_ldap.la \
> @@ -366,6 +371,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.sh \
> src/tests/krb5_proxy_check_test_data.conf \
> $(NULL)
>
> diff --git a/src/tests/whitespace_test.sh b/src/tests/whitespace_test.sh
> new file mode 100755
> index 0000000000000000000000000000000000000000..33e695b470e40a0b93f7320a2f4a77503e0b78e8
> --- /dev/null
> +++ b/src/tests/whitespace_test.sh
> @@ -0,0 +1,11 @@
> +#!/bin/sh
>
> +N=$(git grep -n -I -P '^.*[ ]+$' -- $(git rev-parse --show-toplevel) | wc -l)
>
> +if [ "$N" -ne 0 ]; then
> echo "Trailing whitespace found:"
> git grep -n -I -P '^.*[ ]+$' -- $(git rev-parse --show-toplevel)
> exit 1
>
> +fi
>
> +exit 0

Please ask Nikolai for review of bash script.

LS

[Pavel Reichl]

On 08/11/2015 06:45 AM, Lukas Slebodnik wrote:
> On (10/08/15 19:31), Pavel Reichl wrote:
> [snip]
> > From: Pavel Reichl 
> > Date: Mon, 10 Aug 2015 13:06:37 -0400
> > Subject: [PATCH 1/2] Remove trailing whitespace
> >
> > README                              | 2 +-
> > po/ca.po                            | 2 +-
>
> We should not manually edit translation files.
> Please and exception for these files.

Thanks, I will do as you propose.
> [snip]
> Trivial test should be part of bug fix.

I'm not sure, I discussed this matter yesterday with Michal and then 
decided for 2 patches. Pusher can always squeeze the patches if he wants.
> So we will not forget to backport it.

I really don't think we are going to back port this.
> It make sense to have a test in separate patch for big features
> which are tracked by upstream ticket.

[snip]
> Please ask Nikolai for review of bash script.

OK, I will.

Lukas, do you know if there should be a preamble in this file? I noticed 
several other files in git suffixed with .sh that miss preamble as well. 
So I'm not sure.
Thanks!
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (11/08/15 10:47), Pavel Reichl wrote:
> On 08/11/2015 06:45 AM, Lukas Slebodnik wrote:
> > On (10/08/15 19:31), Pavel Reichl wrote:
> > [snip]
> > > From: Pavel Reichl 
> > > Date: Mon, 10 Aug 2015 13:06:37 -0400
> > > Subject: [PATCH 1/2] Remove trailing whitespace
> > >
> > > README                              | 2 +-
> > > po/ca.po                            | 2 +-
> >
> > We should not manually edit translation files.
> > Please and exception for these files.
>
> Thanks, I will do as you propose.
> > [snip]
> > Trivial test should be part of bug fix.
>
> I'm not sure, I discussed this matter yesterday with Michal and then decided
> for 2 patches. Pusher can always squeeze the patches if he wants.
> > So we will not forget to backport it.
>
> I really don't think we are going to back port this.
> > It make sense to have a test in separate patch for big features
> > which are tracked by upstream ticket.
>
> [snip]
> > Please ask Nikolai for review of bash script.
>
> OK, I will.
>
> Lukas, do you know if there should be a preamble in this file? I noticed
> several other files in git suffixed with .sh that miss preamble as well. So
> I'm not sure.

IANAL

LS

[Pavel Reichl]

On 08/11/2015 12:08 PM, Lukas Slebodnik wrote:
> On (11/08/15 10:47), Pavel Reichl wrote:
> > On 08/11/2015 06:45 AM, Lukas Slebodnik wrote:
> > > On (10/08/15 19:31), Pavel Reichl wrote:
> > > [snip]
> > > > From: Pavel Reichl 
> > > > Date: Mon, 10 Aug 2015 13:06:37 -0400
> > > > Subject: [PATCH 1/2] Remove trailing whitespace
> > > >
> > > > README                              | 2 +-
> > > > po/ca.po                            | 2 +-
> > >
> > > We should not manually edit translation files.
> > > Please and exception for these files.
> >
> > Thanks, I will do as you propose.
> > > [snip]
> > > Trivial test should be part of bug fix.
> >
> > I'm not sure, I discussed this matter yesterday with Michal and then decided
> > for 2 patches. Pusher can always squeeze the patches if he wants.
> > > So we will not forget to backport it.
> >
> > I really don't think we are going to back port this.
> > > It make sense to have a test in separate patch for big features
> > > which are tracked by upstream ticket.
> >
> > [snip]
> > > Please ask Nikolai for review of bash script.
> >
> > OK, I will.
> >
> > Lukas, do you know if there should be a preamble in this file? I noticed
> > several other files in git suffixed with .sh that miss preamble as well. So
> > I'm not sure.
>
> IANAL

Yes, I thought so but thanks for confirmation anyway.

Updated patch attached.
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Nikolai Kondrashov]

Hi Pavel,

It's a good idea to have such a test, thank you! Please see my comments
regarding the script (as you requested) below.

On 08/11/2015 01:46 PM, Pavel Reichl wrote:
> diff --git a/src/tests/whitespace_test.sh b/src/tests/whitespace_test.sh
> new file mode 100755

We don't really need an extension in an executable file name. It will make it
that bit difficult to change the implementation language if we ever need that.

We don't put ".elf" on binary files, after all.

It's not a big deal, of course, just ranting a little.
> index 0000000000000000000000000000000000000000..4f34bdf981996440b89a3e7e5f99ad3b6b3eae34
> --- /dev/null
> +++ b/src/tests/whitespace_test.sh
> @@ -0,0 +1,13 @@
> +#!/bin/sh

Did you intend /bin/sh here? Most people expect Bash to run their scripts so
it might be safer to just specify it explicitly if you don't really need to
support the vanilla shell. It might prevent some unexpected failures on
systems where /bin/sh isn't Bash (e.g. Debian with Dash installed) in the
future, even though it works fine now.
> +# example: EXCLUDE_FILES="path1|path2"
> +EXCLUDE_FILES="po/ca.po"

The name of the variable is somewhat confusing: it's not just files, it's a
regex, so there's more syntax, and care must be taken to escape special
characters. How about "PATH_EXCLUDE_REGEX", or similar? E.g. the regex above
will also match "po/catpoo".
> +N=$(git grep -n -I -P '^.*[ ]+$' -- $(git rev-parse --show-toplevel) | grep -v $EXCLUDE_FILES | wc -l)

The "^.*" combination in the regex is a noop and can be removed for
readability. There is no need to use '[]' around the space. Matching just
space would skip trailing tabs, the general whitespace pattern '\s' might be
better here. So the pattern can become '\s+$'.

We don't support running under a path with whitespace, but it's still better
to quote the "git rev-parse" command substitution. I.e. write this instead:

"$(git rev-parse --show-toplevel)"

The grep pattern is not anchored to anything, which means that we're likely to
have unintended matches. E.g. a Makefile.am line with trailing whitespace,
mentioning matching file will get ignored. You can anchor it like this:

grep -v "^\($EXCLUDE_FILES\):"
> +if [ "$N" -ne 0 ]; then
> echo "Trailing whitespace found:"
> git grep -n -I -P '^.*[ ]+$' -- $(git rev-parse --show-toplevel) | grep -v $EXCLUDE_FILES
> exit 1
>
> +fi
>
> +exit 0

Nobody likes duplicated code, and in this case there are several ways to avoid
that. I like the one using awk, which combined with the above comments can
look like this:

#!/bin/bash

# An AWK regex matching tracked file paths to be excluded from the search.
     # Example: '.*.po|README'
     PATH_EXCLUDE_REGEX='.*.po'

git grep -n -I -P '\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
             }
         "

Note that you don't need the "exit" command or checking the exit status after
this command, if it is the last one in the script. Its exit status will be
used as the script's exit status.

BTW, shouldn't we take care to not use git, or to not run this test in case
we're running outside git tree, e.g. from unpacked tarball?

Nick

[Lukas Slebodnik]

On (11/08/15 15:04), Nikolai Kondrashov wrote:
> Hi Pavel,
>
> It's a good idea to have such a test, thank you! Please see my comments
> regarding the script (as you requested) below.
>
> On 08/11/2015 01:46 PM, Pavel Reichl wrote:
> > diff --git a/src/tests/whitespace_test.sh b/src/tests/whitespace_test.sh
> > new file mode 100755
>
> We don't really need an extension in an executable file name. It will make it
> that bit difficult to change the implementation language if we ever need that.
>
> We don't put ".elf" on binary files, after all.
>
> It's not a big deal, of course, just ranting a little.
> > index 0000000000000000000000000000000000000000..4f34bdf981996440b89a3e7e5f99ad3b6b3eae34
> > --- /dev/null
> > +++ b/src/tests/whitespace_test.sh
> > @@ -0,0 +1,13 @@
> > +#!/bin/sh
>
> Did you intend /bin/sh here? Most people expect Bash to run their scripts so
> it might be safer to just specify it explicitly if you don't really need to
> support the vanilla shell. It might prevent some unexpected failures on
> systems where /bin/sh isn't Bash (e.g. Debian with Dash installed) in the
> future, even though it works fine now.
> > +# example: EXCLUDE_FILES="path1|path2"
> > +EXCLUDE_FILES="po/ca.po"
>
> The name of the variable is somewhat confusing: it's not just files, it's a
> regex, so there's more syntax, and care must be taken to escape special
> characters. How about "PATH_EXCLUDE_REGEX", or similar? E.g. the regex above
> will also match "po/catpoo".
> > +N=$(git grep -n -I -P '^.*[ ]+$' -- $(git rev-parse --show-toplevel) | grep -v $EXCLUDE_FILES | wc -l)
>
> The "^.*" combination in the regex is a noop and can be removed for
> readability. There is no need to use '[]' around the space. Matching just
> space would skip trailing tabs, the general whitespace pattern '\s' might be
> better here. So the pattern can become '\s+$'.
>
> We don't support running under a path with whitespace, but it's still better
> to quote the "git rev-parse" command substitution. I.e. write this instead:
>
> "$(git rev-parse --show-toplevel)"
>
> The grep pattern is not anchored to anything, which means that we're likely to
> have unintended matches. E.g. a Makefile.am line with trailing whitespace,
> mentioning matching file will get ignored. You can anchor it like this:
>
> grep -v "^\($EXCLUDE_FILES\):"
> > +if [ "$N" -ne 0 ]; then
> > echo "Trailing whitespace found:"
> > git grep -n -I -P '^.*[ ]+$' -- $(git rev-parse --show-toplevel) | grep -v $EXCLUDE_FILES
> > exit 1
> >
> > +fi
> >
> > +exit 0
>
> Nobody likes duplicated code, and in this case there are several ways to avoid
> that. I like the one using awk, which combined with the above comments can
> look like this:
>
> #!/bin/bash
>
> # An AWK regex matching tracked file paths to be excluded from the search.
>    # Example: '.*.po|README'
>    PATH_EXCLUDE_REGEX='.*.po'
>
> git grep -n -I -P '\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
>            }
>        "
>
> Note that you don't need the "exit" command or checking the exit status after
> this command, if it is the last one in the script. Its exit status will be
> used as the script's exit status.
>
> BTW, shouldn't we take care to not use git, or to not run this test in case
> we're running outside git tree, e.g. from unpacked tarball?

+1

LS

[Pavel Reichl]

On 08/11/2015 02:11 PM, Lukas Slebodnik wrote:
> On (11/08/15 15:04), Nikolai Kondrashov wrote:
> > Hi Pavel,
> >
> > It's a good idea to have such a test, thank you! Please see my comments
> > regarding the script (as you requested) below.
> >
> > On 08/11/2015 01:46 PM, Pavel Reichl wrote:
> > > diff --git a/src/tests/whitespace_test.sh b/src/tests/whitespace_test.sh
> > > new file mode 100755
> >
> > We don't really need an extension in an executable file name. It will make it
> > that bit difficult to change the implementation language if we ever need that.
> >
> > We don't put ".elf" on binary files, after all.
> >
> > It's not a big deal, of course, just ranting a little.
> > > index 0000000000000000000000000000000000000000..4f34bdf981996440b89a3e7e5f99ad3b6b3eae34
> > > --- /dev/null
> > > +++ b/src/tests/whitespace_test.sh
> > > @@ -0,0 +1,13 @@
> > > +#!/bin/sh
> >
> > Did you intend /bin/sh here? Most people expect Bash to run their scripts so
> > it might be safer to just specify it explicitly if you don't really need to
> > support the vanilla shell. It might prevent some unexpected failures on
> > systems where /bin/sh isn't Bash (e.g. Debian with Dash installed) in the
> > future, even though it works fine now.
> > > +# example: EXCLUDE_FILES="path1|path2"
> > > +EXCLUDE_FILES="po/ca.po"
> >
> > The name of the variable is somewhat confusing: it's not just files, it's a
> > regex, so there's more syntax, and care must be taken to escape special
> > characters. How about "PATH_EXCLUDE_REGEX", or similar? E.g. the regex above
> > will also match "po/catpoo".
> > > +N=$(git grep -n -I -P '^.*[ ]+$' -- $(git rev-parse --show-toplevel) | grep -v $EXCLUDE_FILES | wc -l)
> >
> > The "^.*" combination in the regex is a noop and can be removed for
> > readability. There is no need to use '[]' around the space. Matching just
> > space would skip trailing tabs, the general whitespace pattern '\s' might be
> > better here. So the pattern can become '\s+$'.
> >
> > We don't support running under a path with whitespace, but it's still better
> > to quote the "git rev-parse" command substitution. I.e. write this instead:
> > "$(git rev-parse --show-toplevel)"
> >
> > The grep pattern is not anchored to anything, which means that we're likely to
> > have unintended matches. E.g. a Makefile.am line with trailing whitespace,
> > mentioning matching file will get ignored. You can anchor it like this:
> > grep -v "^\\($EXCLUDE_FILES\\):"
> >
> > > +if [ "$N" -ne 0 ]; then
> > > echo "Trailing whitespace found:"
> > > git grep -n -I -P '^.*[ ]+$' -- $(git rev-parse --show-toplevel) | grep -v $EXCLUDE_FILES
> > > exit 1
> > >
> > > +fi
> > >
> > > +exit 0
> >
> > Nobody likes duplicated code, and in this case there are several ways to avoid
> > that. I like the one using awk, which combined with the above comments can
> > look like this:
> > #!/bin/bash
> >
> > # An AWK regex matching tracked file paths to be excluded from the search.
> > # Example: '.*\.po|README'
> > PATH_EXCLUDE_REGEX='.*\.po'
> >
> > git grep -n -I -P '\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
> >         }
> >     "
> >
> > Note that you don't need the "exit" command or checking the exit status after
> > this command, if it is the last one in the script. Its exit status will be
> > used as the script's exit status.
> >
> > BTW, shouldn't we take care to not use git, or to not run this test in case
> > we're running outside git tree, e.g. from unpacked tarball?
>
> +1
>
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Nick, thank you for the comments.

Would you take over this patch? I understand that you already have a 
better solution and I think there's no need to update mine in that case. 
Would you send a patch with proposed solution? There's no time pressure 
on this effort so you would not have to hurry.

Thanks!

Thanks!

[Nikolai Kondrashov]

Hi Pavel,

On 08/11/2015 03:44 PM, Pavel Reichl wrote:
> On 08/11/2015 02:11 PM, Lukas Slebodnik wrote:
> > On (11/08/15 15:04), Nikolai Kondrashov wrote:
> > > Hi Pavel,
> > >
> > > It's a good idea to have such a test, thank you! Please see my comments
> > > regarding the script (as you requested) below.
> > >
> > > BTW, shouldn't we take care to not use git, or to not run this test in case
> > > we're running outside git tree, e.g. from unpacked tarball?
> >
> > +1
> >
> > LS
>
> Nick, thank you for the comments.
>
> Would you take over this patch? I understand that you already have a better
> solution and I think there's no need to update mine in that case. Would you
> send a patch with proposed solution? There's no time pressure on this effort
> so you would not have to hurry.

Sure, no problem. I'll send an update this week.

Nick

[PATCH v2] Remove trailing whitespace

[Nikolai Kondrashov]

On 08/11/2015 07:13 PM, Nikolai Kondrashov wrote:
> Hi Pavel,
>
> On 08/11/2015 03:44 PM, Pavel Reichl wrote:
> > On 08/11/2015 02:11 PM, Lukas Slebodnik wrote:
> > > On (11/08/15 15:04), Nikolai Kondrashov wrote:
> > > > Hi Pavel,
> > > >
> > > > It's a good idea to have such a test, thank you! Please see my comments
> > > > regarding the script (as you requested) below.
> > > >
> > > > BTW, shouldn't we take care to not use git, or to not run this test in case
> > > > we're running outside git tree, e.g. from unpacked tarball?
> > >
> > > +1
> > >
> > > LS
> >
> > Nick, thank you for the comments.
> >
> > Would you take over this patch? I understand that you already have a better
> > solution and I think there's no need to update mine in that case. Would you
> > send a patch with proposed solution? There's no time pressure on this effort
> > so you would not have to hurry.
>
> Sure, no problem. I'll send an update this week.

Alright, here are the updated patches. Changes from the last version by Pavel
include:

* Reworked and renamed the whitespace test, fixing a number of issues.
* Made the whitespace test run only if the source is in a Git worktree
   (checkout).
* Added another patch making Git worktree (checkout) detection work in a
   complete out-of-tree dir and at the same time don't trigger erroneously from
   a subdir of (possibly unrelated) worktree. This is also needed to have the
   whitespace test not run during "make distcheck", but run in CI.

CI results: http://sssd-ci.duckdns.org/logs/job/21/40/summary.html

Nick

[PATCH v2] Remove trailing whitespace

[Pavel Reichl]

On 08/13/2015 01:47 PM, Nikolai Kondrashov wrote:
> On 08/11/2015 07:13 PM, Nikolai Kondrashov wrote:
> > Hi Pavel,
> >
> > On 08/11/2015 03:44 PM, Pavel Reichl wrote:
> > > On 08/11/2015 02:11 PM, Lukas Slebodnik wrote:
> > > > On (11/08/15 15:04), Nikolai Kondrashov wrote:
> > > > > Hi Pavel,
> > > > >
> > > > > It's a good idea to have such a test, thank you! Please see my 
> > > > > comments
> > > > > regarding the script (as you requested) below.
> > > > >
> > > > > BTW, shouldn't we take care to not use git, or to not run this 
> > > > > test in case
> > > > > we're running outside git tree, e.g. from unpacked tarball?
> > > >
> > > > +1
> > > >
> > > > LS
> > >
> > > Nick, thank you for the comments.
> > >
> > > Would you take over this patch? I understand that you already have a 
> > > better
> > > solution and I think there's no need to update mine in that case. 
> > > Would you
> > > send a patch with proposed solution? There's no time pressure on 
> > > this effort
> > > so you would not have to hurry.
> >
> > Sure, no problem. I'll send an update this week.
>
> Alright, here are the updated patches. Changes from the last version 
> by Pavel
> include:
> Reworked and renamed the whitespace test, fixing a number of issues.
> Made the whitespace test run only if the source is in a Git worktree
> (checkout).
> Added another patch making Git worktree (checkout) detection work in a
> complete out-of-tree dir and at the same time don't trigger
>
> erroneously from
>   a subdir of (possibly unrelated) worktree. This is also needed to 
> have the
>   whitespace test not run during "make distcheck", but run in CI.
>
> CI results: http://sssd-ci.duckdns.org/logs/job/21/40/summary.html
>
> Nick

Thanks for the patches Nick! I think the authorship of the 3rd patch 
should be changed to you, as you changed (no doubt to better) the core 
of the patch.

So if you will amend patch during the review process please consider 
changing it, thanks!

[PATCH v2] Remove trailing whitespace

[Nikolai Kondrashov]

On 08/13/2015 02:54 PM, Pavel Reichl wrote:
> On 08/13/2015 01:47 PM, Nikolai Kondrashov wrote:
> > On 08/11/2015 07:13 PM, Nikolai Kondrashov wrote:
> > > Hi Pavel,
> > >
> > > On 08/11/2015 03:44 PM, Pavel Reichl wrote:
> > > > On 08/11/2015 02:11 PM, Lukas Slebodnik wrote:
> > > > > On (11/08/15 15:04), Nikolai Kondrashov wrote:
> > > > > > Hi Pavel,
> > > > > >
> > > > > > It's a good idea to have such a test, thank you! Please see my comments
> > > > > > regarding the script (as you requested) below.
> > > > > >
> > > > > > BTW, shouldn't we take care to not use git, or to not run this test in case
> > > > > > we're running outside git tree, e.g. from unpacked tarball?
> > > > >
> > > > > +1
> > > > >
> > > > > LS
> > > >
> > > > Nick, thank you for the comments.
> > > >
> > > > Would you take over this patch? I understand that you already have a better
> > > > solution and I think there's no need to update mine in that case. Would you
> > > > send a patch with proposed solution? There's no time pressure on this effort
> > > > so you would not have to hurry.
> > >
> > > Sure, no problem. I'll send an update this week.
> >
> > Alright, here are the updated patches. Changes from the last version by Pavel
> > include:
> > Reworked and renamed the whitespace test, fixing a number of issues.
> > Made the whitespace test run only if the source is in a Git worktree
> > (checkout).
> > Added another patch making Git worktree (checkout) detection work in a
> > complete out-of-tree dir and at the same time don't trigger erroneously from
> > a subdir of (possibly unrelated) worktree. This is also needed to have the
> > whitespace test not run during "make distcheck", but run in CI.
> >
> > CI results: http://sssd-ci.duckdns.org/logs/job/21/40/summary.html
> >
> > Nick
>
> Thanks for the patches Nick! I think the authorship of the 3rd patch should
> be changed to you, as you changed (no doubt to better) the core of the
> patch.
>
> So if you will amend patch during the review process please consider
> changing it, thanks!

Yeah, I considered changing it, but the base idea was yours, so I decided to
leave it. I'll change it since you think it should be me instead.

Thank you.

Nick

[PATCH v2] Remove trailing whitespace

[Lukas Slebodnik]

On (13/08/15 14:47), Nikolai Kondrashov wrote:
> On 08/11/2015 07:13 PM, Nikolai Kondrashov wrote:
> > Hi Pavel,
> >
> > On 08/11/2015 03:44 PM, Pavel Reichl wrote:
> > > On 08/11/2015 02:11 PM, Lukas Slebodnik wrote:
> > > > On (11/08/15 15:04), Nikolai Kondrashov wrote:
> > > > > Hi Pavel,
> > > > >
> > > > > It's a good idea to have such a test, thank you! Please see my comments
> > > > > regarding the script (as you requested) below.
> > > > >
> > > > > BTW, shouldn't we take care to not use git, or to not run this test in case
> > > > > we're running outside git tree, e.g. from unpacked tarball?
> > > >
> > > > +1
> > > >
> > > > LS
> > >
> > > Nick, thank you for the comments.
> > >
> > > Would you take over this patch? I understand that you already have a better
> > > solution and I think there's no need to update mine in that case. Would you
> > > send a patch with proposed solution? There's no time pressure on this effort
> > > so you would not have to hurry.
> >
> > Sure, no problem. I'll send an update this week.
>
> Alright, here are the updated patches. Changes from the last version by Pavel
> include:
> Reworked and renamed the whitespace test, fixing a number of issues.
> Made the whitespace test run only if the source is in a Git worktree
>
> (checkout).
> Added another patch making Git worktree (checkout) detection work in a
>
> complete out-of-tree dir and at the same time don't trigger erroneously from
>  a subdir of (possibly unrelated) worktree. This is also needed to have the
>  whitespace test not run during "make distcheck", but run in CI.
>
> CI results: http://sssd-ci.duckdns.org/logs/job/21/40/summary.html
>
> Nick
> From c22d9d9103f443c8fd134cd57f0eb7b6d65fd7b9 Mon Sep 17 00:00:00 2001
> From: Nikolai Kondrashov 
> Date: Wed, 12 Aug 2015 13:27:07 +0300
> Subject: [PATCH 2/3] BUILD: Check for Git worktree at srcdir exactly
>
> Check for Git worktree (checkout) presence at $srcdir exactly, instead
> of any directory at or above $builddir. Use that directory and the
> associated repo for all Git invocations during build and tests.
>
> This allows Git invocations to work completely out-of-tree (not just in
> a sub-directory), when configured from a Git worktree. This also
> prevents erroneous detection of a Git worktree with an $srcdir located
> deeper, such as a distcheck build, or a tarball build under an unrelated
> worktree.
>
> Makefile.am  | 5 +++++
> configure.ac | 3 ++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index 8b64317..8016591 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -84,6 +84,11 @@ INSTALL = @INSTALL@
>
> SSSD_USER = @SSSD_USER@
>
> +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.

LS

[PATCH v2] Remove trailing whitespace

[Nikolai Kondrashov]

On 08/13/2015 02:57 PM, Lukas Slebodnik wrote:
> On (13/08/15 14:47), Nikolai Kondrashov wrote:
> > On 08/11/2015 07:13 PM, Nikolai Kondrashov wrote:
> > > Hi Pavel,
> > >
> > > On 08/11/2015 03:44 PM, Pavel Reichl wrote:
> > > > On 08/11/2015 02:11 PM, Lukas Slebodnik wrote:
> > > > > On (11/08/15 15:04), Nikolai Kondrashov wrote:
> > > > > > Hi Pavel,
> > > > > >
> > > > > > It's a good idea to have such a test, thank you! Please see my comments
> > > > > > regarding the script (as you requested) below.
> > > > > >
> > > > > > BTW, shouldn't we take care to not use git, or to not run this test in case
> > > > > > we're running outside git tree, e.g. from unpacked tarball?
> > > > >
> > > > > +1
> > > > >
> > > > > LS
> > > >
> > > > Nick, thank you for the comments.
> > > >
> > > > Would you take over this patch? I understand that you already have a better
> > > > solution and I think there's no need to update mine in that case. Would you
> > > > send a patch with proposed solution? There's no time pressure on this effort
> > > > so you would not have to hurry.
> > >
> > > Sure, no problem. I'll send an update this week.
> >
> > Alright, here are the updated patches. Changes from the last version by Pavel
> > include:
> > Reworked and renamed the whitespace test, fixing a number of issues.
> > Made the whitespace test run only if the source is in a Git worktree
> > (checkout).
> > Added another patch making Git worktree (checkout) detection work in a
> > complete out-of-tree dir and at the same time don't trigger erroneously from
> > a subdir of (possibly unrelated) worktree. This is also needed to have the
> > whitespace test not run during "make distcheck", but run in CI.
> >
> > CI results: http://sssd-ci.duckdns.org/logs/job/21/40/summary.html
> >
> > Nick
> > From c22d9d9103f443c8fd134cd57f0eb7b6d65fd7b9 Mon Sep 17 00:00:00 2001
> > From: Nikolai Kondrashov 
> > Date: Wed, 12 Aug 2015 13:27:07 +0300
> > Subject: [PATCH 2/3] BUILD: Check for Git worktree at srcdir exactly
> >
> > Check for Git worktree (checkout) presence at $srcdir exactly, instead
> > of any directory at or above $builddir. Use that directory and the
> > associated repo for all Git invocations during build and tests.
> >
> > This allows Git invocations to work completely out-of-tree (not just in
> > a sub-directory), when configured from a Git worktree. This also
> > prevents erroneous detection of a Git worktree with an $srcdir located
> > deeper, such as a distcheck build, or a tarball build under an unrelated
> > worktree.
> >
> > Makefile.am  | 5 +++++
> > configure.ac | 3 ++-
> > 2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 8b64317..8016591 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -84,6 +84,11 @@ INSTALL = @INSTALL@
> >
> > SSSD_USER = @SSSD_USER@
> >
> > +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.

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.

Nick

[PATCH v2] Remove trailing whitespace

[Lukas Slebodnik]

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:
> > > On 08/11/2015 07:13 PM, Nikolai Kondrashov wrote:
> > > > Hi Pavel,
> > > >
> > > > On 08/11/2015 03:44 PM, Pavel Reichl wrote:
> > > > > On 08/11/2015 02:11 PM, Lukas Slebodnik wrote:
> > > > > > On (11/08/15 15:04), Nikolai Kondrashov wrote:
> > > > > > >Hi Pavel,
> > > > > > >
> > > > > > >It's a good idea to have such a test, thank you! Please see my comments
> > > > > > >regarding the script (as you requested) below.
> > > > > > >
> > > > > > >BTW, shouldn't we take care to not use git, or to not run this test in case
> > > > > > >we're running outside git tree, e.g. from unpacked tarball?
> > > > > > >
> > > > > > +1
> > > > > >
> > > > > > LS
> > > > >
> > > > > Nick, thank you for the comments.
> > > > >
> > > > > Would you take over this patch? I understand that you already have a better
> > > > > solution and I think there's no need to update mine in that case. Would you
> > > > > send a patch with proposed solution? There's no time pressure on this effort
> > > > > so you would not have to hurry.
> > > >
> > > > Sure, no problem. I'll send an update this week.
> > >
> > > Alright, here are the updated patches. Changes from the last version by Pavel
> > > include:
> > > Reworked and renamed the whitespace test, fixing a number of issues.
> > > Made the whitespace test run only if the source is in a Git worktree
> > >
> > > (checkout).
> > > Added another patch making Git worktree (checkout) detection work in a
> > >
> > > complete out-of-tree dir and at the same time don't trigger erroneously from
> > >  a subdir of (possibly unrelated) worktree. This is also needed to have the
> > >  whitespace test not run during "make distcheck", but run in CI.
> > >
> > > CI results: http://sssd-ci.duckdns.org/logs/job/21/40/summary.html
> > >
> > > Nick
> > > From c22d9d9103f443c8fd134cd57f0eb7b6d65fd7b9 Mon Sep 17 00:00:00 2001
> > > From: Nikolai Kondrashov 
> > > Date: Wed, 12 Aug 2015 13:27:07 +0300
> > > Subject: [PATCH 2/3] BUILD: Check for Git worktree at srcdir exactly
> > >
> > > Check for Git worktree (checkout) presence at $srcdir exactly, instead
> > > of any directory at or above $builddir. Use that directory and the
> > > associated repo for all Git invocations during build and tests.
> > >
> > > This allows Git invocations to work completely out-of-tree (not just in
> > > a sub-directory), when configured from a Git worktree. This also
> > > prevents erroneous detection of a Git worktree with an $srcdir located
> > > deeper, such as a distcheck build, or a tarball build under an unrelated
> > > worktree.
> > >
> > > Makefile.am  | 5 +++++
> > > configure.ac | 3 ++-
> > > 2 files changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Makefile.am b/Makefile.am
> > > index 8b64317..8016591 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -84,6 +84,11 @@ INSTALL = @INSTALL@
> > >
> > > SSSD_USER = @SSSD_USER@
> > >
> > > +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-Tes...

LS

[PATCH v3] Remove trailing whitespace

[Nikolai Kondrashov]

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-Tes...

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?

Nick

[PATCH v3] Remove trailing whitespace

[Lukas Slebodnik]

----- Original Message -----
> From: "Nikolai Kondrashov" 
> To: "Development of the System Security Services Daemon" 
> 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-Tes...
>
> 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.

LS

[PATCH v3] Remove trailing whitespace

[Nikolai Kondrashov]

On 08/17/2015 08:42 PM, Lukas Slebodnik wrote:
> ----- Original Message -----
> > From: "Nikolai Kondrashov" 
> > To: "Development of the System Security Services Daemon" 
> > 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-Tes...
> >
> > 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.

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.

Nick

[1] git://anonscm.debian.org/pkg-sssd/sssd.git

[PATCH v3] Remove trailing whitespace

[Michal Židek]

On 08/18/2015 02:31 PM, Nikolai Kondrashov wrote:
> On 08/17/2015 08:42 PM, Lukas Slebodnik wrote:
> > ----- Original Message -----
> > > From: "Nikolai Kondrashov" 
> > > To: "Development of the System Security Services Daemon"
> > >
> > > 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-Tes...
> > >
> > > 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.
>
> 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.
>
> Nick
>
> [1] git://anonscm.debian.org/pkg-sssd/sssd.git

Honestly I think it is OK if this test is properly
executed on Fedora only and disabled elsewhere.
The code is the same everywhere so if there are no
trailing whitespaces on Fedora then there are no
trailing whitespaces on other systems as well.

This is different from testing functionality where we
want to check if something is broken on different
platforms.

Ofcourse if making it multiplatform is easy, then go
for it, but I would not invest too much effort in making
this test multiplatform because I do not see additional
value here.

Michal

[PATCH v3] Remove trailing whitespace

[Lukas Slebodnik]

On (18/08/15 14:47), Michal Židek wrote:
> On 08/18/2015 02:31 PM, Nikolai Kondrashov wrote:
> > On 08/17/2015 08:42 PM, Lukas Slebodnik wrote:
> > > ----- Original Message -----
> > > > From: "Nikolai Kondrashov" 
> > > > To: "Development of the System Security Services Daemon"
> > > >
> > > > 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-Tes...
> > > >
> > > > 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.
> >
> > 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.
> >
> > Nick
> >
> > [1] git://anonscm.debian.org/pkg-sssd/sssd.git
>
> Honestly I think it is OK if this test is properly
> executed on Fedora only and disabled elsewhere.
> The code is the same everywhere so if there are no
> trailing whitespaces on Fedora then there are no
> trailing whitespaces on other systems as well.
>
> This is different from testing functionality where we
> want to check if something is broken on different
> platforms.
>
> Ofcourse if making it multiplatform is easy, then go
> for it, but I would not invest too much effort in making
> this test multiplatform because I do not see additional
> value here.

Package maintainers in other distribution usually do not spend
time with troubleshooting tests. The result is disabled tests at all.
It's not our goal.

LS

[PATCH v3] Remove trailing whitespace

[Nikolai Kondrashov]

On 08/18/2015 03:31 PM, Nikolai Kondrashov wrote:
> On 08/17/2015 08:42 PM, Lukas Slebodnik wrote:
> > ----- Original Message -----
> > > From: "Nikolai Kondrashov" 
> > > To: "Development of the System Security Services Daemon" 
> > > 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-Tes...
> > >
> > > 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.
>
> 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.
>
> Nick
>
> [1] git://anonscm.debian.org/pkg-sssd/sssd.git

Lukas, does my answer prove we'll be fine with the patch set as is?

Nick

[PATCH v3] Remove trailing whitespace

[Lukas Slebodnik]

On (18/08/15 15:31), Nikolai Kondrashov wrote:
> On 08/17/2015 08:42 PM, Lukas Slebodnik wrote:
> > ----- Original Message -----
> > > From: "Nikolai Kondrashov" 
> > > To: "Development of the System Security Services Daemon" 
> > > 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-Tes...
> > >
> > > 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-Tes...

"""
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

LS

[PATCH v3] Remove trailing whitespace

[Lukas Slebodnik]

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" 
> > > > To: "Development of the System Security Services Daemon" 
> > > > 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-Tes...
> > > >
> > > > 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-Tes...
>
> """
> 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

[PATCH v3] Remove trailing whitespace

[Nikolai Kondrashov]

On 08/31/2015 09:59 AM, Lukas Slebodnik wrote:
> 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'

I suggest we also add '|/debian/.*' here to keep the downstream happy.
Although it theoretically should be done in their own repo, we can make life
simpler to them.
> +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

Strictly speaking, this message should be output to stderr, but that doesn't
make much difference here.

Nick

[PATCH v3] Remove trailing whitespace

[Lukas Slebodnik]

On (01/09/15 15:36), Nikolai Kondrashov wrote:
> On 08/31/2015 09:59 AM, Lukas Slebodnik wrote:
> > 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'
>
> I suggest we also add '|/debian/.*' here to keep the downstream happy.
> Although it theoretically should be done in their own repo, we can make life
> simpler to them.

Even though there isn't big problem with other files in debian
Timo (debian maintainer) prefer the same.
> > +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
>
> Strictly speaking, this message should be output to stderr, but that doesn't
> make much difference here.

No problem.
Updated.

BTW test passed(was not skipped) even if builddir was outside of GIT_WORK_TREE.

Nikolai,
Do you have any other ideas dow to improve the test?
Or is there a use case which is covered only in your version?

LS

[PATCH v3] Remove trailing whitespace

[Nikolai Kondrashov]

On 09/02/2015 10:05 AM, Lukas Slebodnik wrote:
> On (01/09/15 15:36), Nikolai Kondrashov wrote:
> > On 08/31/2015 09:59 AM, Lukas Slebodnik wrote:
> > > 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'
> >
> > I suggest we also add '|/debian/.*' here to keep the downstream happy.
> > Although it theoretically should be done in their own repo, we can make life
> > simpler to them.
>
> Even though there isn't big problem with other files in debian
> Timo (debian maintainer) prefer the same.
> > > +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
> >
> > Strictly speaking, this message should be output to stderr, but that doesn't
> > make much difference here.
>
> No problem.
> Updated.
>
> BTW test passed(was not skipped) even if builddir was outside of GIT_WORK_TREE.

Sure. I was concerned with the other git uses, i.e. the rpm* targets.
> Nikolai,
> Do you have any other ideas dow to improve the test?
> Or is there a use case which is covered only in your version?

Hmm, no, I don't think there is anything else to do here.

I haven't tested it, otherwise ACK.

Thank you, Lukas.

Nick

[PATCH v3] Remove trailing whitespace

[Lukas Slebodnik]

On (02/09/15 14:39), Nikolai Kondrashov wrote:
> On 09/02/2015 10:05 AM, Lukas Slebodnik wrote:
> > On (01/09/15 15:36), Nikolai Kondrashov wrote:
> > > On 08/31/2015 09:59 AM, Lukas Slebodnik wrote:
> > > > 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'
> > >
> > > I suggest we also add '|/debian/.*' here to keep the downstream happy.
> > > Although it theoretically should be done in their own repo, we can make life
> > > simpler to them.
> >
> > Even though there isn't big problem with other files in debian
> > Timo (debian maintainer) prefer the same.
> > > > +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
> > >
> > > Strictly speaking, this message should be output to stderr, but that doesn't
> > > make much difference here.
> >
> > No problem.
> > Updated.
> >
> > BTW test passed(was not skipped) even if builddir was outside of GIT_WORK_TREE.
>
> Sure. I was concerned with the other git uses, i.e. the rpm* targets.

In case of "make rpms" or "make prerelease-rpms", we run rpmbuild
for building packages. And rpmbuild uses tarball as a source so
ABS_TOP_SRCDIR would not be the same as GIT_WORK_TREE.
> > Nikolai,
> > Do you have any other ideas dow to improve the test?
> > Or is there a use case which is covered only in your version?
>
> Hmm, no, I don't think there is anything else to do here.
>
> I haven't tested it, otherwise ACK.

Thank you for review. But It would be good if 3rd person
formally ACK the patch because we(both) are authors of 2nd patch :-)

LS

[PATCH v3] Remove trailing whitespace

[Jakub Hrozek]

On Wed, Sep 02, 2015 at 01:52:42PM +0200, Lukas Slebodnik wrote:
> On (02/09/15 14:39), Nikolai Kondrashov wrote:
> > I haven't tested it, otherwise ACK.
>
> Thank you for review. But It would be good if 3rd person
> formally ACK the patch because we(both) are authors of 2nd patch :-)

Pavel, you were involved in the original thread, please help out Lukas
and Nikolai here.

[PATCH v3] Remove trailing whitespace

[Pavel Reichl]

On 09/02/2015 02:25 PM, Jakub Hrozek wrote:
> On Wed, Sep 02, 2015 at 01:52:42PM +0200, Lukas Slebodnik wrote:
> > On (02/09/15 14:39), Nikolai Kondrashov wrote:
> > > I haven't tested it, otherwise ACK.
> >
> > Thank you for review. But It would be good if 3rd person
> > formally ACK the patch because we(both) are authors of 2nd patch :-)
>
> Pavel, you were involved in the original thread, please help out Lukas
> and Nikolai here.

LGTM, ci passed: http://sssd-ci.duckdns.org/logs/job/24/27/summary.html

I added whitespace to source files - it was detected.
I added whitespace to po files  - it was ignored.

ACK

[PATCH v3] Remove trailing whitespace

[Jakub Hrozek]

On Wed, Sep 02, 2015 at 03:48:27PM +0200, Pavel Reichl wrote:
> On 09/02/2015 02:25 PM, Jakub Hrozek wrote:
> > On Wed, Sep 02, 2015 at 01:52:42PM +0200, Lukas Slebodnik wrote:
> > > On (02/09/15 14:39), Nikolai Kondrashov wrote:
> > > > I haven't tested it, otherwise ACK.
> > >
> > > Thank you for review. But It would be good if 3rd person
> > > formally ACK the patch because we(both) are authors of 2nd patch :-)
> >
> > Pavel, you were involved in the original thread, please help out Lukas
> > and Nikolai here.
>
> LGTM, ci passed: http://sssd-ci.duckdns.org/logs/job/24/27/summary.html
>
> I added whitespace to source files - it was detected.
> I added whitespace to po files  - it was ignored.
>
> ACK

* master:
    * 2b490bc947dbe0094417304840bd721417a162d9
    * cbff3fcdce5b0377a62fbe74f32e476efbf7ca9c

[PATCH v3] Remove trailing whitespace

[Nikolai Kondrashov]

On 09/03/2015 10:53 AM, Jakub Hrozek wrote:
> On Wed, Sep 02, 2015 at 03:48:27PM +0200, Pavel Reichl wrote:
> > On 09/02/2015 02:25 PM, Jakub Hrozek wrote:
> > > On Wed, Sep 02, 2015 at 01:52:42PM +0200, Lukas Slebodnik wrote:
> > > > On (02/09/15 14:39), Nikolai Kondrashov wrote:
> > > > > I haven't tested it, otherwise ACK.
> > > >
> > > > Thank you for review. But It would be good if 3rd person
> > > > formally ACK the patch because we(both) are authors of 2nd patch :-)
> > >
> > > Pavel, you were involved in the original thread, please help out Lukas
> > > and Nikolai here.
> >
> > LGTM, ci passed: http://sssd-ci.duckdns.org/logs/job/24/27/summary.html
> >
> > I added whitespace to source files - it was detected.
> > I added whitespace to po files  - it was ignored.
> >
> > ACK
> master:
> 2b490bc947dbe0094417304840bd721417a162d9
> cbff3fcdce5b0377a62fbe74f32e476efbf7ca9c

Thanks, everyone!

Nick

[PATCH v3] Remove trailing whitespace

[Nikolai Kondrashov]

On 08/27/2015 11:56 AM, 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" 
> > > > To: "Development of the System Security Services Daemon" 
> > > > 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-Tes...
> > > >
> > > > 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.

I wouldn't say these are exclusive. I think we still need to fix the Git tree
detection, so it doesn't consider to be under a git repo, which doesn't start
at the source root, and so it works with completely out-of-tree builds.

We can have ABS_TOP_SRCDIR as well, no problem. Although I think this is
complicating things a bit too early, since we don't have any tests yet that
need it (AFAIK).
> 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-Tes...
>
> """
> 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.
> """

Hmm, yes, SKIP'ping the test explicitly might be better here.
> > 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

I think distro package maintainers don't really care about trailing whitespace
test - it is needed for people reviewing patches, after all. Debian directory
has other files, so it might be better to exclude it as a whole. I agree on
the *.patch files.

Nick

[PATCH v3] Remove trailing whitespace

[Lukas Slebodnik]

On (31/08/15 16:24), Nikolai Kondrashov wrote:
> On 08/27/2015 11:56 AM, 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" 
> > > > > To: "Development of the System Security Services Daemon" 
> > > > > 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-Tes...
> > > > >
> > > > > 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.
>
> I wouldn't say these are exclusive. I think we still need to fix the Git tree
> detection, so it doesn't consider to be under a git repo, which doesn't start
> at the source root, and so it works with completely out-of-tree builds.

The detection is implemented in the last available version
https://lists.fedorahosted.org/pipermail/sssd-devel/2015-August/024590.html
> We can have ABS_TOP_SRCDIR as well, no problem. Although I think this is
> complicating things a bit too early, since we don't have any tests yet that
> need it (AFAIK).

We also do not have a tests which require GIT_DIR or GIT_WORKTREE.
Could you comment the last version of patch and explain what is there
complicated (comparing to previous version of patch?)
> > 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-Tes...
> >
> > """
> > 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.
> > """
>
> Hmm, yes, SKIP'ping the test explicitly might be better here.

Implemented in the last available version
> > > 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
>
> I think distro package maintainers don't really care about trailing whitespace
> test - it is needed for people reviewing patches, after all. Debian directory
> has other files, so it might be better to exclude it as a whole. I agree on
> the *.patch files.

diff filles and patches are excluded in the last available version.

LS

[PATCH v3] Remove trailing whitespace

[Nikolai Kondrashov]

On 09/01/2015 07:21 AM, Lukas Slebodnik wrote:
> On (31/08/15 16:24), Nikolai Kondrashov wrote:
> > I wouldn't say these are exclusive. I think we still need to fix the Git tree
> > detection, so it doesn't consider to be under a git repo, which doesn't start
> > at the source root, and so it works with completely out-of-tree builds.
>
> The detection is implemented in the last available version
> https://lists.fedorahosted.org/pipermail/sssd-devel/2015-August/024590.html

You mean the detection inside the whitespace test? I meant the detection
within configure, which is used with rpm* targets.
> > We can have ABS_TOP_SRCDIR as well, no problem. Although I think this is
> > complicating things a bit too early, since we don't have any tests yet that
> > need it (AFAIK).
>
> We also do not have a tests which require GIT_DIR or GIT_WORKTREE.
> Could you comment the last version of patch and explain what is there
> complicated (comparing to previous version of patch?)

Yes, this is simple enough. However, the latest version's configure script
will still detect GIT repo presence even if the tarball source root is under
another repo, and will not detect the GIT repo for a completely out-of-tree
build. Sure, this is only used for developer-oriented rpm* targets and it was
behaving this way for a long while now, so we're probably fine.

If we don't want to fix that, then yes, this is fine.

If we want to fix that, then exporting GIT_DIR and GIT_WORKTREE in the
Makefile might be simpler.  If we want to do the latter, then we might as well
rely on these variables being present in the whitespace test and avoid passing
(so far unneeded) ABS_TOP_SRCDIR and making GIT_DIR and GIT_WORKTREE from it
in the test.

I'm fine with either approach: don't fix the configure detection and only add
ABS_TOP_SRCDIR, or fix the configure detection and export GIT_DIR and
GIT_WORKTREE, not adding ABS_TOP_SRCDIR yet.
> > Hmm, yes, SKIP'ping the test explicitly might be better here.
>
> Implemented in the last available version
> > I think distro package maintainers don't really care about trailing whitespace
> > test - it is needed for people reviewing patches, after all. Debian directory
> > has other files, so it might be better to exclude it as a whole. I agree on
> > the *.patch files.
>
> diff filles and patches are excluded in the last available version.

Ah, didn't look at it yet. Will leave my comments there, thank you.

Nick

[Nikolai Kondrashov]

On 08/11/2015 01:46 PM, Pavel Reichl wrote:
> +# example: EXCLUDE_FILES="path1|path2"
> +EXCLUDE_FILES="po/ca.po"

Ah, forgot to mention: wouldn't it be better to just match all .po files?
I assumed it would and used that in the code I suggested. Please change it,
if you'll be using my code, and there are reasons against that.

Nick