runningcode / gradle-doctor

The right prescription for your Gradle build.
http://runningcode.github.io/gradle-doctor
Apache License 2.0
723 stars 46 forks source link

Revert "Compare bin subdirs of java home on root mismatch [fix 187]" #241

Closed runningcode closed 1 year ago

runningcode commented 1 year ago

Reverts runningcode/gradle-doctor#240

This doesn't work. I have this issue on my local machine:

  | Gradle is not using JAVA_HOME.                                                                       |
  | JAVA_HOME is /Users/no/.asdf/installs/java/adoptopenjdk-11.0.11+9                                    |
  | Gradle is using /Users/no/.asdf/installs/java/adoptopenjdk-11.0.11+9                                 |
  | This can slow down your build significantly when switching from Android Studio to the terminal.      |
  | To fix: Project Structure -> JDK Location.                                                           |
  | Set this to your JAVA_HOME.

See build scan here: https://scans.gradle.com/s/utvdqanc3oila

Rattenkrieg commented 1 year ago

Really weird, seems both of us are using sdkman on mac. If you happen to have time to look at this, I suggest logging both gradleJavaHomePath.toRealPath().resolve("bin") and environmentJavaHomePath.resolve("bin").toRealPath() perhaps this could provide a clue.

slowpacer commented 3 months ago

Getting back to this thread, I believe the "zulu-sdkman" issue is still present and troubles people from time to time. The fix that was reverted within this MR is actually partially fixing the java home check. The problem is - it's focusing only on the issue but won't work in normal circumstances, so only works for zulu distributions.

The if (gradleJavaHomePath.toRealPath() != environmentJavaHomePath.toRealPath()) logic handles the fail scenario, so the java home is appended with "bin" into its path and gets a real path comparison(links are unwrapped). But in case the initial condition returns true - the outer logic returns false. And that's why on local machine the java home check failed for oracle distribution(even with proper setup), because there were no issues with linking.

In order to handle it properly I believe we can keep the reverted check, but as a fallback, so that we will execute it only in case the primary check fails. So it can end up being something like this snippet(to be polished ofc):

private fun isGradleUsingJavaHome(): Boolean {
        // Follow symlinks when checking that java home matches.
        return environmentJavaHome?.let {
            val gradleJavaHomePath = gradleJavaHome.toPath()
            val environmentJavaHomePath = File(it).toPath()
            return gradleJavaHomePath.toRealPath() != environmentJavaHomePath.toRealPath() || javaHomeFallbackCheck(
                gradleJavaHomePath,
                environmentJavaHomePath
            )
        } ?: false
    }

    private fun javaHomeFallbackCheck(gradleJavaHomePath: Path, environmentJavaHomePath: Path) =
        gradleJavaHomePath.toRealPath().resolve("bin") == environmentJavaHomePath.resolve("bin").toRealPath()

P.S. @runningcode let me know if that makes sense and isn't introducing more complexity. I can throw an MR. P.P.S. the gradle fix with zulu support(mentioned here) is doing something similar, in a nutshell it just gets the right java exec from the bin folder(I believe so). But the initial java home resolution logic relies on System.getProperty("java.home") and the check gradle doctor relies on system env(System.getenv("JAVA_HOME")) hence the difference. But conceptually, adding bin and unwrapping the real path makes both approaches close(according to me).

runningcode commented 3 months ago

Hi @slowpacer thanks for trying to find a solution here. I think I understand what you are trying to get at but the logic you wrote seems to do something else.

return gradleJavaHomePath.toRealPath() != environmentJavaHomePath.toRealPath() ||

Wouldn't this part above return true if the paths are not equal? Did you mean for that part to have == instead of !=?

Other than that, your solutions seems like a good idea.

slowpacer commented 3 months ago

Hey @runningcode, yeah, apologies, my bad, blindly copy pasted. Indeed should be == instead of !=. EDIT: Well, on that note, maybe it might be easier just to have the fallback check(the one with "bin" in the pass). I guess it should handle all scenarios.🤔🤔🤔

runningcode commented 3 months ago

Isn't only checking "the bin in the path" the issue with what was reverted?

slowpacer commented 3 months ago

From what I see the issue was that your local machine oracle distribution was evaluated against this condition: gradleJavaHomePath.toRealPath() != environmentJavaHomePath.toRealPath() and it returned false as both were the same (/Users/no/.asdf/installs/java/adoptopenjdk-11.0.11+9), and then the outer return just went on with the false negative result.