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

Change "setSource", so it also applies the configured filter #715

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aytchell
Copy link

@aytchell aytchell commented Sep 29, 2023

In our project we're using gradle-ktlint plus we have a GraphQL API and generate stubs from a schema file.
We have team members working on Linux, MacOS and Windows.
To exclude the generated code from linting and formatting we've added this filter statement to our build.gradle.kts:

val sep = File.separatorChar
ktlint {
    version.set("0.49.1")
    filter {
        exclude { it.file.path.contains("${sep}generated$sep") }
    }
}

This works fine for our team members using Linux and MacOS.
But the exclude statement is ignored on the Windows machines (which looks a bit like #579 ).

The changes in this PR fix the exclude filter for our team members developing on Windows.

(at the moment we're using version 11.5.0 of the plugin; tests have shown, that with 11.6.0 the filter is still ignored and this patch works, too)

fun setSource(source: FileTree): BaseKtLintCheckTask {
sourceFiles = objectFactory
.fileCollection()
.from({ source.matching(patternFilterable) })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean the filter is applied twice? since the provider on line 120 also applies the same filter?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is my first contact with gradle internals so I don't fully understand when these methods are called.
From diffing the debug output of Windows and Linux runs I got the impression, that filesystem changes (inotify et al triggered while generating the stubs) are somehow handled differently.

@wakingrufus
Copy link
Collaborator

Thanks for the PR! I have 1 question which I put on the change set itself.

@wakingrufus wakingrufus self-assigned this Sep 29, 2023
@wakingrufus
Copy link
Collaborator

FYI, out github actions run the full test suite on osx, linux, and windows, so you should be able to add a failing test and see it fail on the windows action (since this seems to be a windows-specific issue).

@aytchell
Copy link
Author

aytchell commented Oct 4, 2023

FYI, out github actions run the full test suite on osx, linux, and windows, so you should be able to add a failing test and see it fail on the windows action (since this seems to be a windows-specific issue).

I feared that request a bit (and at the same time I would've suggested this, too :-D).
I'll try to come up with such a test ...

@wakingrufus
Copy link
Collaborator

which gradle version are you using? this might be an issue that was fixed in gradle itself

@aytchell
Copy link
Author

I've managed to write a unit test which passes on Linux but fails on Windows.
The unit test has its own commit (514f7e1).
Should I modify this PR (or open another one) so that it only includes the new test? This way we could verify this behaviour with the PR test suite (I've only called ./gradlew build on both OSes)

@aytchell
Copy link
Author

In the project we're using gradle 8.3

@wakingrufus
Copy link
Collaborator

thanks for the failing test. the thing about your potential fix, is that it shouldnt make a difference. the way apply the patternFilterable to the FileCollection, it is "live". as files are added to the collection, the filter will be applied to those new files. I wonder if maybe the windows failure is happening b/c the file is created too close to when the ktlint task runs, and maybe windows file system is not actually showing the file yet.

@wakingrufus
Copy link
Collaborator

can you add that test to this branch so I can run the CI checks on it?

aytchell pushed a commit to aytchell/ktlint-gradle that referenced this pull request Oct 11, 2023
@aytchell
Copy link
Author

aytchell commented Oct 11, 2023

I've dropped the last commit (with the fix in it) from my remote branch, so the only change is the new unit test.
Once the test suite ran (and "hopefully" failed) I can push the fix onto the branch and you can re-run the test suite

@aytchell
Copy link
Author

Hi there, is there anything missing from my side, so the test suite can start? If not: could anyone with the necessary privileges please start the tests?

@aytchell
Copy link
Author

aytchell commented Oct 18, 2023

I've rebased my branch to the latest main and fixed the finding of ktlint
@wakingrufus Could you please start the test suite?

@JLLeitschuh
Copy link
Owner

Approving run now. Sorry about the delay

@aytchell
Copy link
Author

Ok, this is interesting. The new test fails on my Windows machine.
I'm gonna check if this is somehow timing related

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants