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