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