Skip to content
New issue

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

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

Already on GitHub? # to your account

Traversing the directory hierarchy is broken on Windows as of version 0.47 #1600

Closed
Tracked by #1397
0x6675636b796f75676974687562 opened this issue Aug 22, 2022 · 29 comments · Fixed by #1615
Closed
Tracked by #1397
Labels
cli ktlint command line interface
Milestone

Comments

@0x6675636b796f75676974687562
Copy link
Contributor

0x6675636b796f75676974687562 commented Aug 22, 2022

Expected Behavior

Running ktlint w/o specifying any input files previously made it search for *.kt and *.kts files, starting from the current directory, e. g.:

$ ./ktlint
14:54:01.402 [main] INFO com.pinterest.ktlint.internal.KtlintCommandLine - Enable default patterns [**/*.kt, **/*.kts]
10:56:53.905 [main] DEBUG com.pinterest.ktlint.internal.KtlintCommandLine - 6821ms / 14 file(s) / 167 error(s)

This behaviour still holds when ktlint is run from the command line on Linux or Mac OS X.

Observed Behavior

Yet, using just the very same command line switches on Windows (i. e. when running ktlint from Git Bash) results in no input files being found:

15:01:09.889 [main] INFO com.pinterest.ktlint.internal.KtlintCommandLine - Enable default patterns [**\\*.kt, **\\*.kts]
15:01:10.457 [main] ERROR com.pinterest.ktlint.internal.KtlintCommandLine - No files matched [**\\*.kt, **\\*.kts]

This is a clear regression against version 0.46.1 and, apparently, has been caused by the recent changes which re-enable (sort of — see #1601) the parsing of Ant-like path patterns (e.g.: **/*.kt) — which was broken in version 0.46.1.

Steps to Reproduce

  1. Download the fat JAR from https://github.com/pinterest/ktlint/releases/tag/0.47.0.
  2. Run the far JAR against any non-empty codebase on Windows:
    java -Xmx512m --add-opens java.base/java.lang=ALL-UNNAMED -jar ktlint

Your Environment

  • Version of ktlint used: 0.47
  • Relevant parts of the .editorconfig settings: none
  • Name and version (or code for custom task) of integration used (Gradle plugin, Maven plugin, command line, custom Gradle task): CLI
  • Version of Gradle used (if applicable): N/A
  • Operating System and version:
    • MINGW64_NT-10.0-19043 unit-725 3.3.4-341.x86_64 2022-05-09 11:56 UTC x86_64 Msys
    • Linux unit-725 5.10.102.1-microsoft-standard-WSL2 #1 SMP Wed Mar 2 00:30:59 UTC 2022 x86_64 GNU/Linux
@paul-dingemans paul-dingemans added this to the 0.47.1 milestone Aug 22, 2022
@paul-dingemans
Copy link
Collaborator

I am not sure whether I can fix this soon. Biggest problem for me is that I do not have access to a Windows machine on which I can test. Also the unit testing on the windows-look-a-like-filesystem is limited. Is anybody willing to help me out by either fixing the problem or run tests whenever I have a possible fix ready?

@paul-dingemans paul-dingemans added the cli ktlint command line interface label Aug 22, 2022
@0x6675636b796f75676974687562
Copy link
Contributor Author

Biggest problem for me is that I do not have access to a Windows machine on which I can test. Also the unit testing on the windows-look-a-like-filesystem is limited.

It doesn't actually matter whether any of the unit tests is successful on your or my PC, be it Windows or Linux. The only source of truth should be the CI server, and, since you're using GitHub, you can easily have one. Only when each commit to the project gets tested on the CI infrastructure can you be sure that nothing gets broken.

Please read on about Building and testing Java with Gradle, and also about Using a matrix for your jobs.

There's also plenty of examples on how to use the gradle/gradle-build-action.

@paul-dingemans
Copy link
Collaborator

Yes, that is already in place. But you don't have an actual filesystem in CI. So we use jimfs to simulate that. The windows version of it, lacks some functionality. So certain unit tests can not run on CI.

@0x6675636b796f75676974687562
Copy link
Contributor Author

@paul-dingemans, thank you for the clarification.

Correct me if I'm wrong, but file system operations are allowed when using GitHub Actions — you can create/delete/append files, traverse directories etc.

Probably, FileUtilsFileSequenceTest could be extended to use not only jimfs but also the default file system. If you think the resulting test would be too heavy, then it could be extracted into a separate GitHub workflow. Or am I missing something here?

@paul-dingemans
Copy link
Collaborator

I would not expect it to be too heavy. I just have no experience with it. Most code in this project was written before I got involved.

If you have a reference to a project where something like this is set up, I would be happy to use that. Otherwise, I have to investigate it myself. Of course, it might take longer as a result of that. So any help is appreciated.

@0x6675636b796f75676974687562
Copy link
Contributor Author

@paul-dingemans, I'll try to refactor a unit test and give you a clue how a default file system can be tested.

In a short while.

@paul-dingemans
Copy link
Collaborator

@paul-dingemans, I'll try to refactor a unit test and give you a clue how a default file system can be tested.

In a short while.

That would really be appreciated.

paul-dingemans added a commit to paul-dingemans/ktlint that referenced this issue Sep 1, 2022
…matching

Globs always use a "/" as directory separator on all OS's. Input patterns containing
a "\" on Windows OS are transformed to "/" as users on Windows more likely would
assume that the "\" may be used.

On WindowsOS, transform "\" in the filepath to "/" before comparing the filename with
the regular expression (of the glob) which always uses "/" as separator.

Refactor all logic which create globs based on an input path.
- If a path (absolute or relative) point to a directory, that path is expanded
  to the default globs (*.kt, *.kts) in that specific directory or any of its
  subdirectories.
- If a path (absolute or relative) does not point to a directory, e.g. it
  points to a file, or it is a pattern. See "**" replacement below.
- On Windows OS patters containing a "*" (or "**") can not be resolved with
  default Paths utilities. In such case the given input pattern is handled as
  is. See "**" replacement below.

Patterns that contain one or more occurrence of a "**" are split into multiple
patterns so that files on that specific path and subdirectories will be matched.
 - For example, for path "some/path/**/*.kt" an additional pattern
   "some/path/*.kt" is generated to make sure that not only the "*.kt" files in
   a subdirectory of "some/path/" are found but also the "*.kt" in directory
   "some/path" as well. This is in sync with the "**" notation in a glob which
   should be interpreted as having zero or more intermediate subdirectories.
 - For example, for path "some/**/path/**/*.kt", multiple additional patterns
   are generated. As it contains two "**" patterns, 2 x 2 patterns are needed
   to match all possible combinations:
   - "some/**/path/**/*.kt"
   - "some/**/path/*.kt"
   - "some/path/**/*.kt"
   - "some/path/*.kt"

Finally, on Windows OS more fixes are needed as the resulting globs may not
contain any drive destinations as the start of the path. Such a drive
destination is replaced with a "**". So "D:/some/path/*.kt" becomes
"/some/path/*.kt". Note that the last glob representation is less strict than
the original pattern as it could match on other drives that "D:/" as well.

Extend trace logging.

Closes pinterest#1600
Closes pinterest#1601
@paul-dingemans
Copy link
Collaborator

I think I have found the problems with the Unit tests for windows. It was caused by incorrect logic in the FileUtils.kt which has been present there for a long time. See #1615 for fixes related to #1600 and #1601.

@paul-dingemans
Copy link
Collaborator

Hi @0x6675636b796f75676974687562 ,

As far as I can see, I have fixed the problem with path traversing and the ant-path parsing (#1601). I would be grateful if you could test latest snapshot (see https://pinterest.github.io/ktlint/install/snapshot-build/) to see whether the problems are resolved on Windows.

@paul-dingemans
Copy link
Collaborator

Hi @0x6675636b796f75676974687562 ,

As far as I can see, I have fixed the problem with path traversing and the ant-path parsing (#1601). I would be grateful if you could test latest snapshot (see https://pinterest.github.io/ktlint/install/snapshot-build/) to see whether the problems are resolved on Windows.

@0x6675636b796f75676974687562 Andrey,
Can you please check this on Windows? It is the last issue for which I am waiting for a confirmation so that version 0.47.1 can be released.

@unix-junkie
Copy link

@paul-dingemans, I'll test and get back to you tomorrow morning.

@0x6675636b796f75676974687562
Copy link
Contributor Author

Tested on Windows, works like a charm.

Thank you @paul-dingemans.

@CokieForever
Copy link

Not sure if you fixed that as well, but when using **, only files under the current directory are searched. That leads to some confusion with patterns like ../src/**/*.kt finding no file at all, while for example ../src/main/kotlin/com/package/*.kt would find (some) files.

Little demonstration: ktlint_test.zip

ktlint_test> ktlint *.kt
ktlint_test\Test.kt:2:16: Unnecessary trailing comma before ")" (trailing-comma-on-declaration-site)

ktlint_test> ktlint **/*.kt
ktlint_test\Test.kt:2:16: Unnecessary trailing comma before ")" (trailing-comma-on-declaration-site)

ktlint_test> cd subdir

ktlint_test\subdir> ktlint ../*.kt
ktlint_test\subdir\..\Test.kt:2:16: Unnecessary trailing comma before ")" (trailing-comma-on-declaration-site)

ktlint_test\subdir> ktlint ../**/*.kt
12:40:22.807 [main] ERROR com.pinterest.ktlint.internal.KtlintCommandLine - No files matched [../**/*.kt]

@unix-junkie
Copy link

@CokieForever, please enclose the last argument in double quotes to make sure it doesn't get expanded by the shell.

@CokieForever
Copy link

CokieForever commented Sep 12, 2022

@unix-junkie I'm not sure what you mean, the argument is logged by ktlint and seems perfectly fine. Besides I'm on Windows like the OP, so shell expansion is not really a thing 😄 And even if by some strange phenomenon it was indeed expanded, it should still work, right? 🤔

For the sake of completeness I tried with double quotes anyway, and it made no difference. I also tried with absolute paths, also no difference. From what I can gather from the debug logs the issue is caused by the rootDir parameter of FileSystem.fileSequence(): https://github.com/pinterest/ktlint/blob/master/ktlint/src/main/kotlin/com/pinterest/ktlint/internal/FileUtils.kt#L46

ktlint_test\subdir> ktlint "../**/*.kt" --debug
16:05:01.441 [main] DEBUG com.pinterest.ktlint.internal.KtlintCommandLine - Discovered reporter with "baseline" id.
16:05:01.443 [main] DEBUG com.pinterest.ktlint.internal.KtlintCommandLine - Discovered reporter with "checkstyle" id.
16:05:01.444 [main] DEBUG com.pinterest.ktlint.internal.KtlintCommandLine - Discovered reporter with "json" id.
16:05:01.445 [main] DEBUG com.pinterest.ktlint.internal.KtlintCommandLine - Discovered reporter with "format" id.
16:05:01.445 [main] DEBUG com.pinterest.ktlint.internal.KtlintCommandLine - Discovered reporter with "html" id.
16:05:01.445 [main] DEBUG com.pinterest.ktlint.internal.KtlintCommandLine - Discovered reporter with "plain" id.
16:05:01.445 [main] DEBUG com.pinterest.ktlint.internal.KtlintCommandLine - Discovered reporter with "sarif" id.
16:05:01.446 [main] DEBUG com.pinterest.ktlint.internal.KtlintCommandLine - Initializing "plain" reporter with {verbose=false, color=false, color_name=DARK_GRAY, format=false}
16:05:01.472 [main] DEBUG com.pinterest.ktlint.internal.FileUtils - Start walkFileTree for rootDir: 'C:\ktlint_test\subdir'
   include:
[      - sun.nio.fs.WindowsFileSystem$2@44a7bfbc]
   exlcude:
[]
16:05:01.477 [main] DEBUG com.pinterest.ktlint.internal.FileUtils - Discovered 0 files to be processed in 4 ms
16:05:01.484 [main] ERROR com.pinterest.ktlint.internal.KtlintCommandLine - No files matched [../**/*.kt]

@paul-dingemans
Copy link
Collaborator

Are you sure that you are using version 0.47.1? In the logging above, I see a typo exlcude which did exist in 0.47.0 but was fixed in 0.47.1. In 0.47.1, you should have seen a warning that ".." may not be used when it results in a reference outside the current directory (https://github.com/pinterest/ktlint/blob/master/ktlint/src/main/kotlin/com/pinterest/ktlint/internal/FileUtils.kt#L283).

@CokieForever
Copy link

No sorry I was using 0.47.0, I hadn't realized 0.47.1 was out. I can see the error message with 0.47.1, and apparently we went back to WARN instead of ERROR as well.

ktlint_test\subdir> ktlint ../**/*.kt
16:46:19.763 [main] WARN com.pinterest.ktlint.internal.FileUtils - On WindowsOS the pattern '../**/*.kt' can not be used as it refers to a path outside of the current directory
16:46:19.790 [main] WARN com.pinterest.ktlint.internal.KtlintCommandLine - No files matched [../**/*.kt]

Does this mean the issue will not get fixed then? Using parent directories would be an effective workaround for #1288 😞

@CokieForever
Copy link

Nevermind, nevermind, apparently #1288 was fixed in 0.47.1 as well, or at least it works for me again 😄

@tommus
Copy link

tommus commented May 4, 2023

@paul-dingemans I think the problem mentioned by @CokieForever still remains. The only difference in my case is - I am using Mac OS X machine.

I have a build.gradle file in a module defined in a subdirectory of the $rootDir of my project. I am interested in running the static analysis against all the modules and due to the error explained by @CokieForever - it is impossible.

I've tested ktlint 0.47.1 which @CokieForever mentioned as working in his case. With no luck. I've also tested the most recent one - 0.49.0 with exact same result. I've changed either gradle dependency and a ktlint CLI binary installed on my machine.

The FileUtils class point to a current directory as a starting point of file tree walk and finds no files to scan.

Here is the output of a debug logger:

DEBUG com.pinterest.ktlint.internal.FileUtils - Start walkFileTree for rootDir: '/Users/tommus/Code/android/my-project-android/infrastructure/static-analysis'
   include:
[      - sun.nio.fs.UnixFileSystem$3@11a9e7c8]
   exclude:
[]
DEBUG com.pinterest.ktlint.internal.FileUtils - Discovered 0 files to be processed in 6 ms
DEBUG com.pinterest.ktlint.internal.KtlintCommandLine - 66ms / 0 file(s) / 0 error(s)

With respective warnings (based on the arguments provided):

WARN com.pinterest.ktlint.internal.KtlintCommandLine - No files matched [**/*.kt]
WARN com.pinterest.ktlint.internal.KtlintCommandLine - No files matched [../../**/*.kt]
WARN com.pinterest.ktlint.internal.KtlintCommandLine - No files matched [/Users/tommus/Code/android/my-project-android/**/*.kt]

I have tested multiple variants of the arguments:

  • **/*.kt
  • ../../**/*.kt
  • $rootDir/**/*.kt (as it is a custom gradle task)

In each case - the starting point of the file walk was defined incorrectly and scanned only the files from the current directory (pwd in case of ktlint CLI and a build.gradle parent directory in case of Gradle task).

Side note: The last working version in the setup described above - was 0.39.0. I've checked all the versions from 0.49.0 down to 0.40.0 and it seems the changes to globs introduced the problem.

@paul-dingemans
Copy link
Collaborator

I have a build.gradle file in a module defined in a subdirectory of the $rootDir of my project. I am interested in running the static analysis against all the modules and due to the error explained by @CokieForever - it is impossible.

I would assume that build.gradle is actually build.gradle.kts. If you do not supply any glob at the command then this file is picked up, even in case it is located in the directory from which you execute the command. Your sample globs all end with *.kt which will never match a file ending with *.kts. Can you post the exact (and entire) command that you execute if the problem persists?

@jokinol
Copy link

jokinol commented May 8, 2023

Same here, I just updated from 0.39.0 to 0.49.0 and noticed the same issue. Running it on MacOS machine and seems that this relates to bash shell somehow because didn't notice any issues with zsh or fish.

It is quite easy to reproduce with bash: just run ktlint ../project-directory/src/**/*.kt in some subdirectory other than project-directory, in project-directory it works as expected. This gives you 09:00:51.660 [main] WARN com.pinterest.ktlint.cli.internal.KtlintCommandLine - No files matched [../project-dir/src/**/*.kt] error (should that log level be ERROR btw?). As said, fish and zsh works fine so it was a bit hard to catch, because everything worked in command line, but failed when I tried to run ktlint in bash script.

Hope this helps.

@tommus
Copy link

tommus commented May 8, 2023

@paul-dingemans - in this exact project, all the gradle files are implemented in groovy. So the assumption about build.gradle.kts is wrong. Also, I am interested in scanning for .kt files, so the problem is not with the input (I simply do not bother about .kts files).

I've also tried to use the kltint cli with no explicitely passed input globs. With exactly same result of no files found due to the fact, the traversal started in the module's directory not in the project's directory.

@jokinol described how one can reproduce the problem. There is no difference whether you will run the ktlint cli from the command line or try to use gradle to actually call it.

In addition to what @jokinol wrote, I experience this problem with Z Sheel (zsh) - so the guess about the problem being shell-dependent, might be unlucky guess.

@paul-dingemans
Copy link
Collaborator

in this exact project, all the gradle files are implemented in groovy. So the assumption about build.gradle.kts is wrong.

Tnx for clarifying this false assumption. I can now reproduce the problem and will create a new issue for fixing it.

@paul-dingemans
Copy link
Collaborator

The problem is now fixed via #2002 / #2004. Can you please verify using the snapshot that the problem is really gone for your use cases?

@jokinol
Copy link

jokinol commented May 10, 2023

For some reason I was not able to execute shadowJarExecutable task with 0.49.0 (Unable to generate signature for '/Users//src/ktlint/ktlint-cli/build/run/ktlint' as no signatory is available to sign) but I used commit bc39250a0ed84f72b091b52e2b4d85737f93ce97 (one before your PR) and generated executable jar and it was broken as expected:

bash-3.2$ java -jar /Users/<user>/src/ktlint/ktlint-cli/build/libs/ktlint-cli-0.49.1-SNAPSHOT-all.jar ../<project>/src/**/*.kt
08:48:07.294 [main] WARN com.pinterest.ktlint.cli.internal.KtlintCommandLine - No files matched [../<project>/src/**/*.kt]
bash-3.2$

But in master it worked:

bash-3.2$ java -jar /Users/<user>/src/ktlint/ktlint-cli/build/libs/ktlint-cli-0.49.1-SNAPSHOT-all.jar ../<project>/src/**/*.kt
bash-3.2$

So I can confirm that at least it works on my machine.

@tommus
Copy link

tommus commented May 10, 2023

@paul-dingemans - I've tested 0.49.1-SNAPSHOT in my multi-module project with this nested ktlint module configuration and it seems to work fine. It resolves the files starting from the project root directory.

@jokinol - regarding the shadowJarExecutable error, more recent version of ktlint started using updated version of shadow library that creates a fatjar package. This library added a new (shadow) runtime configuration. It's a backward-incompatible change as ktlint become incapable of selecting proper runtime configuration on it's own, so you have to provide some help.

It's enough to define the attributes within the module dependencies:

dependencies {

    // (...)

    // The attribute workaround below solves the problems introduced by the updated shadow library.
    // @see https://github.com/pinterest/ktlint/issues/1114
    // @see https://github.com/JLLeitschuh/ktlint-gradle/issues/458

    add("ktlint", "com.pinterest:ktlint:<ktlint-version>") {
        attributes {
            attribute(Bundling.BUNDLING_ATTRIBUTE, objects.named(Bundling, Bundling.EXTERNAL))
        }
    }
}

@paul-dingemans
Copy link
Collaborator

@jokinol @tommus Tnx for confirming.

@jokinol
Copy link

jokinol commented May 11, 2023

Could you release 0.49.1 kindly please? I did some ugly hacks to my CI scripts and would like to remove those 😬

@paul-dingemans
Copy link
Collaborator

I cannot release on request as everybody want a release as soon as their problem is solved ;-)
But you're lucky as I already has plans to release this week. But, I need to fix a couple of things before I do so. So it might take a little longer.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
cli ktlint command line interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants