Skip to content
This repository was archived by the owner on Mar 21, 2023. It is now read-only.

feature: Try and resolve java home if it's a symlink - #479

Open
tgodzik wants to merge 1 commit into
scalameta:mainfrom
tgodzik:try-resolve
Open

feature: Try and resolve java home if it's a symlink#479
tgodzik wants to merge 1 commit into
scalameta:mainfrom
tgodzik:try-resolve

Conversation

@tgodzik

@tgodzik tgodzik commented Aug 8, 2022

Copy link
Copy Markdown
Contributor

@tgodzik
tgodzik requested review from kpodsiad and tanishiking August 8, 2022 16:16
@tgodzik
tgodzik marked this pull request as ready for review August 8, 2022 16:16
Comment thread src/getJavaHome.ts
Comment on lines +42 to +49
if (fs.existsSync(configuredJavaHome)) {
const stat = fs.lstatSync(configuredJavaHome);
const realpath = fs.realpathSync(configuredJavaHome);
if (stat.isSymbolicLink() && realpath.endsWith(`bin${path.sep}java`)) {
const javaHome = path.dirname(path.dirname(realpath));
return TE.right(javaHome);
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it more readable? I find flat ifs and early return more readable than nesting it

  const cleanJavaHome = configuredJavaHome?.trim();

  if (cleanJavaHome == null) return TE.left({});

  if (cleanJavaHome.length === 0) return TE.left({});

  if (fs.existsSync(cleanJavaHome)) {
    const stat = fs.lstatSync(cleanJavaHome);
    const realpath = fs.realpathSync(cleanJavaHome);
    if (stat.isSymbolicLink() && realpath.endsWith(`bin${path.sep}java`)) {
      const javaHome = path.dirname(path.dirname(realpath));
      return TE.right(javaHome);
    }
  }
  
  return TE.right(cleanJavaHome);

@dos65 dos65 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I took a look over conversations about that and I can't that I was convinced that it should work in this way 😸

What about adding an additional fallback function that tries to infer java Home from java in $PATH. Then it should work in the same way but without a setting.
I mean here -

pipe(fromConfig(configuredJavaHome), TE.orElse(fromEnv), TE.orElse(locate))

@kpodsiad

kpodsiad commented Aug 8, 2022

Copy link
Copy Markdown
Member

I took a look over conversations about that and I can't that I was convinced that it should work in this way 😸

Maybe it's finally time for metals to download own Java? Problems with java home seem to be never ending 😢

@tanishiking tanishiking left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about adding an additional fallback function that tries to infer java Home from java in $PATH. Then it should work in the same way but without a setting.

I agree with @dos65
Adding another inference rule based on java on $PATH (the rule should be the same with this PR, resolve symlink and grandparent dir should be its java home).
This inference rule should have the lowest priority, next to the locate-java-home.


By the way, maybe we should replace locate-java-home with find-java-home looks like it has the inference rule for the grandparent dir of javac).

@tgodzik

tgodzik commented Aug 9, 2022

Copy link
Copy Markdown
Contributor Author

What about adding an additional fallback function that tries to infer java Home from java in $PATH. Then it should work in the same way but without a setting.

I agree with @dos65 Adding another inference rule based on java on $PATH (the rule should be the same with this PR, resolve symlink and grandparent dir should be its java home). This inference rule should have the lowest priority, next to the locate-java-home.

Och, I didn't think this would be controvertial 😅

By the way, maybe we should replace locate-java-home with find-java-home looks like it has the inference rule for the grandparent dir of javac).

Looks like it does the same thing as this PR, so might be good to use that instead. Let's check it after the next release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Infer Metals Java Home When Path Symlinks to Java Binary

4 participants