Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

whitespace_test: fix exclusion of debian directory #5435

Closed

Conversation

deastoe
Copy link
Contributor

@deastoe deastoe commented Dec 15, 2020

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.

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.
@alexey-tikhonov
Copy link
Member

Hi @deastoe,

what is this debian\ dir? I don't find it in current source tree...

@deastoe
Copy link
Contributor Author

deastoe commented Jan 7, 2021

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: cbff3fc. Even back then I don't see a debian directory in the source tree.

@alexey-tikhonov
Copy link
Member

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?

@deastoe
Copy link
Contributor Author

deastoe commented Jan 7, 2021

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-whitespace-test.diff. (I didn't realise this as we use older packaging based on Debian stable).

@alexey-tikhonov
Copy link
Member

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.

@pbrezina
Copy link
Member

pbrezina commented Apr 1, 2021

I talked to tjaalton and he's fine with removing it completely. @deastoe would you like to update this PR or shall we?

@alexey-tikhonov alexey-tikhonov self-assigned this Apr 1, 2021
alexey-tikhonov added a commit to alexey-tikhonov/sssd that referenced this pull request Apr 19, 2021
as this is downstream specific.

See discussion in SSSD#5435 for details
@alexey-tikhonov alexey-tikhonov added superseded This PR is superseded in favor if a different one and removed Changes requested labels Apr 19, 2021
@alexey-tikhonov
Copy link
Member

Superseded by #5592

@pbrezina pbrezina closed this Apr 21, 2021
pbrezina pushed a commit that referenced this pull request Apr 21, 2021
…stream specific.

See discussion in #5435 for details

Reviewed-by: Pavel Březina <pbrezina@redhat.com>
akuster pushed a commit to akuster/sssd that referenced this pull request May 18, 2021
…stream specific.

See discussion in SSSD#5435 for details

Reviewed-by: Pavel Březina <pbrezina@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
superseded This PR is superseded in favor if a different one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants