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