Fix pre-commit hook command not found error #564

rbleuse · 2022-01-08T03:43:35Z

Fixes #562

G00fY2 · 2022-01-24T19:21:11Z

@rbleuse I noticed your PR too late and fixed the same issue. Please check out my changes from #568. You don't need Regex to improve the tests. I also suggest to switch to underscore_case for consistency (e.g. CHANGED_FILES). Also I don't see a need for a const val only for gradleCommandExitCode. ${'$'}$gradleCommandExitCode makes it even harder to read. For NF it makes sense because it's used for concatenation.

Maybe you can apply my changes to your PR.

rbleuse · 2022-01-25T04:48:35Z

Hi @G00fY2 thanks for the suggestion !

G00fY2 · 2022-01-25T19:50:39Z

Not sure why the tests on windows are not passing. Looks like they are somewhat flaky. I tried to reproduce this on my windows machine but actually for me all tests are failing (maybe because I use JDK 17).

JLLeitschuh · 2022-01-25T21:40:50Z

Here's the exception, I don't think this was caused by you. I've re-kicked the jobs:

https://scans.gradle.com/s/zjxfgwkm3i2fg/tests/:test/org.jlleitschuh.gradle.ktlint.KtLintSupportedVersionsTest/Format should successfully finish on sources with style violations$ktlint_gradle(GradleVersion, String)[12]?anchor=e30&expanded-stacktrace=WyIwLTEtMiIsIjAtMSIsIjAiXQ&top-execution=1

rbleuse · 2022-01-25T22:46:07Z

It seems it failed again :

2: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':samples:android-app:loadKtlintReporters'.
> Could not resolve all files for configuration ':samples:android-app:ktlint'.
   > Could not resolve com.pinterest:ktlint:0.42.1.
     Required by:
         project :samples:android-app
      > Could not resolve com.pinterest:ktlint:0.42.1.
         > Could not get resource 'https://repo.maven.apache.org/maven2/com/pinterest/ktlint/0.42.1/ktlint-0.42.1.pom'.
            > Could not GET 'https://repo.maven.apache.org/maven2/com/pinterest/ktlint/0.42.1/ktlint-0.42.1.pom'.
               > repo.maven.apache.org

crisguitar · 2022-02-23T14:20:59Z

Any reasons this is not merged yet?

JLLeitschuh · 2022-02-23T15:44:56Z

plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/GitHook.kt

@@ -54,13  54,12 @@ private fun postCheck(
            git add ${'$'}file
        fi
    done
-    """.trimIndent()


Doesn't some kind of trimming need to happen here?

@JLLeitschuh No because if we keep it, formatUsesGradleExitCode fails to find gradle_command_exit_code=$? as there is indentation before :

With trimming (original)

Expecting: <["#!/bin/sh", "######## KTLINT-GRADLE HOOK START ########", "", " CHANGED_FILES="$(git --no-pager diff --name-status --no-color --cached | awk '$1 != "D" && $NF ~ /\.kts?$/ { print $NF }')"", "", " if [ -z "$CHANGED_FILES" ]; then", " echo "No Kotlin staged files."", " exit 0", " fi;", "", " echo "Running ktlint over these files:"", " echo "$CHANGED_FILES"", "", " diff=.git/unstaged-ktlint-git-hook.diff", " git diff --color=never > $diff", " if [ -s $diff ]; then", " git apply -R $diff", " fi", "", " ./gradlew --quiet ktlintFormat -PinternalKtlintGitFilter="$CHANGED_FILES"", " gradle_command_exit_code=$?", "", " echo "Completed ktlint run."", " echo "$CHANGED_FILES" | while read -r file; do", " if [ -f $file ]; then", " git add $file", " fi", "done", "", " if [ -s $diff ]; then", " git apply --ignore-whitespace $diff", " fi", " rm $diff", " unset diff", "", " echo "Completed ktlint hook."", " exit $gradle_command_exit_code", "######## KTLINT-GRADLE HOOK END ########"]> to contain: <["gradle_command_exit_code=$?"]> but could not find: <["gradle_command_exit_code=$?"]>

Without trimming

"######## KTLINT-GRADLE HOOK START ########", "", "CHANGED_FILES="$(git --no-pager diff --name-status --no-color --cached | awk '$1 != "D" && $NF ~ /\.kts?$/ { print $NF }')"", "", "if [ -z "$CHANGED_FILES" ]; then", " echo "No Kotlin staged files."", " exit 0", "fi;", "", "echo "Running ktlint over these files:"", "echo "$CHANGED_FILES"", "", "diff=.git/unstaged-ktlint-git-hook.diff", "git diff --color=never > $diff", "if [ -s $diff ]; then", " git apply -R $diff", "fi", "", "./gradlew --quiet ktlintFormat -PinternalKtlintGitFilter="$CHANGED_FILES"", "gradle_command_exit_code=$?", "", "echo "Completed ktlint run."", "", "echo "$CHANGED_FILES" | while read -r file; do", " if [ -f $file ]; then", " git add $file", " fi", "done", "", "", "if [ -s $diff ]; then", " git apply --ignore-whitespace $diff", "fi", "rm $diff", "unset diff", "", "echo "Completed ktlint hook."", "exit $gradle_command_exit_code", "######## KTLINT-GRADLE HOOK END ########"]>

But if you tell me that the first indentation with trimIndent (while it's supposed to remove indentation) is expected, then I'll add it back and look for exit $gradle_command_exit_code with indentation in the test instead.

JLLeitschuh · 2022-02-23T15:46:32Z

Any reasons this is not merged yet?

The tests were failing. I was waiting for a green run

craigberry1 · 2022-03-10T15:07:17Z

Hey @JLLeitschuh will this get merged soon? We just updated to 10.2.1 to fix #539 but now I'm running into this... thanks!

JLLeitschuh · 2022-03-10T18:28:52Z

Happy to merge this. @rbleuse can you rebase your changes and I'll merge! 😄

rbleuse · 2022-03-11T13:12:12Z

@JLLeitschuh done :)

rbleuse · 2022-03-12T00:59:25Z

@JLLeitschuh It seems there's an OOM when running tests on windows :

# There is insufficient memory for the Java Runtime Environment to continue.
# Native memory allocation (malloc) failed to allocate 32744 bytes for ChunkPool::allocate
# An error report file with more information is saved as:
# D:\a\ktlint-gradle\ktlint-gradle\plugin\hs_err_pid7152.log

JLLeitschuh · 2022-03-12T02:19:00Z

#I've been returning these tests multiple times and every time they fail with an OOM error. I have no idea why, but master doesn't fail. This failure is consistently reproducible with your changes and I have no idea why.

rbleuse · 2022-03-13T13:30:07Z

@JLLeitschuh When you twice merged into my fix, the two builds were successful :

I can confirm that this OOM is not because of this fix.
I reverted my fix except the changelog, and it still fails with an OOM (you can check last commit build result). There's definitely an issue with the github windows build as the ubuntu one builds correctly.

My guess is, parallel tests are enabled, and somehow there's a memory issue with parallel tests which use too much memory

Also one note, tests always fail into OOM on the KtLintSupportedVersionsTest class. Probably something wrong in this test class or in the code that this class is testing

Reference failed builds :
https://github.com/JLLeitschuh/ktlint-gradle/runs/5527597501?check_suite_focus=true
https://github.com/JLLeitschuh/ktlint-gradle/runs/5527663939?check_suite_focus=true
https://github.com/JLLeitschuh/ktlint-gradle/runs/5514012872?check_suite_focus=true
https://github.com/JLLeitschuh/ktlint-gradle/runs/5527730803?check_suite_focus=true

G00fY2 · 2022-03-13T14:32:53Z

I can aggree. I also run these tests on my local Windows machine. No such error occurred.

* master: testAnnotations use version_current and upload memory dump

JLLeitschuh · 2022-03-14T17:29:44Z

Updated the GH Action to upload memory dumps. Let's see what it says:

Link to zip:
https://github.com/JLLeitschuh/ktlint-gradle/suites/5653913145/artifacts/185071671

* master: Also grab the replay_pid log file on crash Log test events during test execution

* master: Fix formatting of build file

* master: Disable the Gradle Daemon on Windows tests

JLLeitschuh · 2022-03-14T19:45:00Z

I can confirm that this OOM is not because of this fix.

I 100% agree with you, but unfortunately, I can't merge this PR knowing that it will break master, it would prevent me from releasing. Unfortunately, I've already spent more time trying to figure out what the problem is here, and haven't had any luck.

I'm trying one last thing with this change: bbc0c94

If we can't figure out the root cause of this OOM issue I can't merge this PR. Unfortunately, as I spelled out in this issue here #569 I'm not really available anymore to maintain this project beyond merging PRs from the community. I don't actively use ktlint-gradle anymore, and I'm now a full-time security researcher not working on any Kotlin or Gradle based projects actively at this time. I'm more than happy to merge any PRs that adequately fix this overall OOM error, or even PRs that help debug what's going wrong, but I don't think I have the time to continue to figure out what's wrong here. I'm really sorry as that's probably pretty disappointing to hear.

* master: Fix failing test from Daemon disable

* master: Fix formatting

rbleuse · 2022-03-15T22:25:01Z

Yes I don't have much time to investigate where this OOM comes from either, but as I mentioned it doesn't come from this PR :(

As far as we can investigate from builds history is that the PR #543 has also an OOM when the PR has been merged to master
Here is build log : https://github.com/JLLeitschuh/ktlint-gradle/runs/4739934010?check_suite_focus=true

Anyway, your last commit didn't pass either, so what should we do ?

JLLeitschuh · 2022-03-18T14:50:38Z

Anyway, your last commit didn't pass either, so what should we do ?

I'm not certain. Maybe this is caused by a specific version of the Ktlint library on windows? If so, and someone can figure that out, a PR to disable that in our tests would be accepted.

I just can't merge a PR I know breaks the main branch as I won't be able to publish releases. I'm really sorry.

* master: Fix "Unknown command-line option '--no-daemon'" error

rbleuse · 2022-03-19T01:31:44Z

I just can't merge a PR I know breaks the main branch as I won't be able to publish releases. I'm really sorry.

With all due respect, did you notice that almost all your Create prepare_release.yaml commits directly on master also failed with this same insufficient memory error ?
As I mentioned before, this error doesn't come from this PR, as main is already broken.
Besides, my PR now doesn't have any extra code, only the changelog (as it's required to start testing with GA) so it's in pair identical to the main and it also fails.

What about increasing tests memory ? Same as mentioned in the readme

tasks.withType<org.jlleitschuh.gradle.ktlint.tasks.BaseKtLintCheckTask> {
    workerMaxHeapSize.set("512m")
}

Or disable parallel tests ?

JLLeitschuh · 2022-03-21T21:43:23Z

With all due respect, did you notice that almost all your Create prepare_release.yaml commits directly on master also failed with this same insufficient memory error ?

I actually didn't. So that 100% confirms this isn't your change breaking things. Okay, I'll merge it. Thanks for pointing out my mistake here. Sorry.

* master: Remove windows tests from critical path

JLLeitschuh · 2022-03-21T21:51:57Z

I've removed the windows tests from the critical path, so the other checks should theoretically now run

rbleuse · 2022-03-21T21:59:58Z

With all due respect, did you notice that almost all your Create prepare_release.yaml commits directly on master also failed with this same insufficient memory error ?

I actually didn't. So that 100% confirms this isn't your change breaking things. Okay, I'll merge it. Thanks for pointing out my mistake here. Sorry.

No worries :)
I'll then commit back the fix later on today, for now this PR contains only the changelog

This reverts commit 58b9a86.

This reverts commit 92ccd8a.

This reverts commit a1c79a9.

rbleuse · 2022-03-21T22:05:55Z

It should be ok now

* master: Fix `fail-fast` declaration

JLLeitschuh · 2022-03-22T15:07:46Z

Merged, thanks for bearing with me on this one. Much appreciated

rbleuse added 3 commits January 8, 2022 12:43

Fix pre-commit hook command not found error

468a430

add changelog

56310e8

Merge branch 'master' into hotfix/JLLeitschuhgh-562

c02c5b0

JonathanDeLeon mentioned this pull request Jan 21, 2022

Unstaged modification lost when running git hook #552

Open

G00fY2 mentioned this pull request Jan 24, 2022

Pre Commit Hook Error #562

Closed

G00fY2 mentioned this pull request Jan 24, 2022

Fix error in pre-commit hook bash syntax and improve tests #568

Closed

refactor hook

f23ac9a

JLLeitschuh added 2 commits February 7, 2022 14:06

Merge branch 'master' into hotfix/JLLeitschuhgh-562

a4d2d55

Merge branch 'master' into hotfix/JLLeitschuhgh-562

20b0d10

JLLeitschuh reviewed Feb 23, 2022

View reviewed changes

Merge branch 'master' into hotfix/JLLeitschuhgh-562

50fe85b

rbleuse added 3 commits March 13, 2022 21:31

experiment : revert to readText instead of readLines

a1c79a9

experiment: revert to original code

92ccd8a

revert even tests

58b9a86

Merge branch 'master' into hotfix/JLLeitschuhgh-562

331895e

* master: testAnnotations use version_current and upload memory dump

JLLeitschuh added 3 commits March 14, 2022 13:57

Merge branch 'master' into hotfix/JLLeitschuhgh-562

2ecbce0

* master: Also grab the replay_pid log file on crash Log test events during test execution

Merge branch 'master' into hotfix/JLLeitschuhgh-562

6264179

* master: Fix formatting of build file

Merge branch 'master' into hotfix/JLLeitschuhgh-562

e4bee72

* master: Disable the Gradle Daemon on Windows tests

JLLeitschuh added 2 commits March 14, 2022 16:07

Merge branch 'master' into hotfix/JLLeitschuhgh-562

a9a2aca

* master: Fix failing test from Daemon disable

Merge branch 'master' into hotfix/JLLeitschuhgh-562

792bc14

* master: Fix formatting

Merge branch 'master' into hotfix/JLLeitschuhgh-562

9ec201d

* master: Fix "Unknown command-line option '--no-daemon'" error

Merge branch 'master' into hotfix/JLLeitschuhgh-562

6f7ee2f

* master: Remove windows tests from critical path

rbleuse added 3 commits March 22, 2022 07:01

Revert "revert even tests"

8ca666d

This reverts commit 58b9a86.

Revert "experiment: revert to original code"

39c8eff

This reverts commit 92ccd8a.

Revert "experiment : revert to readText instead of readLines"

3f327ac

This reverts commit a1c79a9.

Merge branch 'master' into hotfix/JLLeitschuhgh-562

53d2390

* master: Fix `fail-fast` declaration

JLLeitschuh merged commit b3094f5 into JLLeitschuh:master Mar 22, 2022

rbleuse deleted the hotfix/gh-562 branch March 23, 2022 23:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pre-commit hook command not found error #564

Fix pre-commit hook command not found error #564

rbleuse commented Jan 8, 2022

G00fY2 commented Jan 24, 2022 •

edited

Loading

rbleuse commented Jan 25, 2022

G00fY2 commented Jan 25, 2022

JLLeitschuh commented Jan 25, 2022

rbleuse commented Jan 25, 2022

crisguitar commented Feb 23, 2022

JLLeitschuh Feb 23, 2022

rbleuse Feb 25, 2022

JLLeitschuh commented Feb 23, 2022

craigberry1 commented Mar 10, 2022

JLLeitschuh commented Mar 10, 2022

rbleuse commented Mar 11, 2022

rbleuse commented Mar 12, 2022

JLLeitschuh commented Mar 12, 2022

rbleuse commented Mar 13, 2022 •

edited

Loading

G00fY2 commented Mar 13, 2022

JLLeitschuh commented Mar 14, 2022

JLLeitschuh commented Mar 14, 2022

rbleuse commented Mar 15, 2022

JLLeitschuh commented Mar 18, 2022

rbleuse commented Mar 19, 2022

JLLeitschuh commented Mar 21, 2022

JLLeitschuh commented Mar 21, 2022

rbleuse commented Mar 21, 2022

rbleuse commented Mar 21, 2022

JLLeitschuh commented Mar 22, 2022

Fix pre-commit hook command not found error #564

Fix pre-commit hook command not found error #564

Conversation

rbleuse commented Jan 8, 2022

G00fY2 commented Jan 24, 2022 • edited Loading

rbleuse commented Jan 25, 2022

G00fY2 commented Jan 25, 2022

JLLeitschuh commented Jan 25, 2022

rbleuse commented Jan 25, 2022

crisguitar commented Feb 23, 2022

JLLeitschuh Feb 23, 2022

Choose a reason for hiding this comment

rbleuse Feb 25, 2022

Choose a reason for hiding this comment

JLLeitschuh commented Feb 23, 2022

craigberry1 commented Mar 10, 2022

JLLeitschuh commented Mar 10, 2022

rbleuse commented Mar 11, 2022

rbleuse commented Mar 12, 2022

JLLeitschuh commented Mar 12, 2022

rbleuse commented Mar 13, 2022 • edited Loading

G00fY2 commented Mar 13, 2022

JLLeitschuh commented Mar 14, 2022

JLLeitschuh commented Mar 14, 2022

rbleuse commented Mar 15, 2022

JLLeitschuh commented Mar 18, 2022

rbleuse commented Mar 19, 2022

JLLeitschuh commented Mar 21, 2022

JLLeitschuh commented Mar 21, 2022

rbleuse commented Mar 21, 2022

rbleuse commented Mar 21, 2022

JLLeitschuh commented Mar 22, 2022

G00fY2 commented Jan 24, 2022 •

edited

Loading

rbleuse commented Mar 13, 2022 •

edited

Loading