URL: https://github.com/SSSD/sssd/pull/5435 Author: deastoe Title: #5435: whitespace_test: fix exclusion of debian directory Action: opened
PR body: """ These tests are supposed to ignore trailing whitespace in files in the debian directory. However this is not the case with the current exclude pattern and usage of `git grep`.
Use --full-name with `git grep` to ensure its output is always relative to the repository root, rather than the current directory. Additionally remove the leading slash from the exclude pattern as this will never match the output. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5435/head:pr5435 git checkout pr5435
URL: https://github.com/SSSD/sssd/pull/5435 Title: #5435: whitespace_test: fix exclusion of debian directory
alexey-tikhonov commented: """ Hi @deastoe,
what is this `debian` dir? I don't find it in current source tree... """
See the full comment at https://github.com/SSSD/sssd/pull/5435#issuecomment-755441460
URL: https://github.com/SSSD/sssd/pull/5435 Title: #5435: whitespace_test: fix exclusion of debian directory
deastoe commented: """ Hi @alexey-tikhonov,
I maintain SSSD packages in a Debian derivative, hence have a debian directory in the repository root. This test was failing in some custom build environments, due to whitespace in files below that directory - so it is a downstream issue.
I am guessing that it is the debian packaging directory that the test is intending to ignore, but I'm not sure why it was explicitly ignored. It looks like it has been in place since the test was added: https://github.com/SSSD/sssd/commit/cbff3fcdce5b0377a62fbe74f32e476efbf7ca9c. Even back then I don't see a debian directory in the source tree. """
See the full comment at https://github.com/SSSD/sssd/pull/5435#issuecomment-756060778
URL: https://github.com/SSSD/sssd/pull/5435 Title: #5435: whitespace_test: fix exclusion of debian directory
alexey-tikhonov commented: """ Hi,
I maintain SSSD packages in a Debian derivative, hence have a debian directory in the repository root. This test was failing in some custom build environments, due to whitespace in files below that directory - so it is a downstream issue.
I'm not sure... Should this really be handled upstream? Could this be a downstream Debian patch for this test? In this case upstream we could remove "debian" from exclude pattern entirely...
Btw, do you know how Debian maintainer handles this? They should have the same issue as you, right? """
See the full comment at https://github.com/SSSD/sssd/pull/5435#issuecomment-756071511
URL: https://github.com/SSSD/sssd/pull/5435 Title: #5435: whitespace_test: fix exclusion of debian directory
deastoe commented: """ Absolutely - I do already apply this as a patch. I also sent it upstream since the exclude was already present, just seemingly non-functional. Removing it from the upstream exclude, is not un-reasonable IMO.
re. Debian it looks like they also now have an identical patch: https://salsa.debian.org/sssd-team/sssd/-/blob/master/debian/patches/fix-whi.... (I didn't realise this as we use older packaging based on Debian stable). """
See the full comment at https://github.com/SSSD/sssd/pull/5435#issuecomment-756081686
URL: https://github.com/SSSD/sssd/pull/5435 Title: #5435: whitespace_test: fix exclusion of debian directory
deastoe commented: """ Absolutely - I do already apply this as a patch. I also sent it upstream since the exclude was already present, just seemingly non-functional. Removing it from the upstream exclude is not un-reasonable IMO.
re. Debian it looks like they also now have an identical patch: https://salsa.debian.org/sssd-team/sssd/-/blob/master/debian/patches/fix-whi.... (I didn't realise this as we use older packaging based on Debian stable). """
See the full comment at https://github.com/SSSD/sssd/pull/5435#issuecomment-756081686
URL: https://github.com/SSSD/sssd/pull/5435 Title: #5435: whitespace_test: fix exclusion of debian directory
alexey-tikhonov commented: """
Removing it from the upstream exclude is not un-reasonable IMO.
From my point of view, this is preferable.
Downstream will have to update their patches, but that shouldn't be too troublesome. """
See the full comment at https://github.com/SSSD/sssd/pull/5435#issuecomment-756213133
URL: https://github.com/SSSD/sssd/pull/5435 Title: #5435: whitespace_test: fix exclusion of debian directory
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/5435 Title: #5435: whitespace_test: fix exclusion of debian directory
pbrezina commented: """ I talked to tjaalton and he's fine with removing it completely. @deastoe would you like to update this PR or shall we? """
See the full comment at https://github.com/SSSD/sssd/pull/5435#issuecomment-811833427
URL: https://github.com/SSSD/sssd/pull/5435 Title: #5435: whitespace_test: fix exclusion of debian directory
Label: +superseded
URL: https://github.com/SSSD/sssd/pull/5435 Title: #5435: whitespace_test: fix exclusion of debian directory
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/5435 Title: #5435: whitespace_test: fix exclusion of debian directory
alexey-tikhonov commented: """ Superseded by #5592 """
See the full comment at https://github.com/SSSD/sssd/pull/5435#issuecomment-822734366
URL: https://github.com/SSSD/sssd/pull/5435 Author: deastoe Title: #5435: whitespace_test: fix exclusion of debian directory Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5435/head:pr5435 git checkout pr5435
sssd-devel@lists.fedorahosted.org