[SSSD] [PATCH v3] Remove trailing whitespace

Lukas Slebodnik lslebodn at redhat.com
Tue Sep 1 04:21:58 UTC 2015


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


More information about the sssd-devel mailing list