Skip to content

Support different kotlin/kotlinc install directory setups in backup classpath resolver #383

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

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

themkat
Copy link
Collaborator

@themkat themkat commented Aug 10, 2022

Found out about this issue after investigating the issue in:
fwcd/vscode-kotlin#101

Usually I just use Maven or Gradle, so I almost never need to have a kotlinc/kotlin install available for the language server. The current language server expects a structure like this: libexec/bin/kotlinc, but that is not always the case.

Example on my machine. kotlinc in path resolves to /opt/homebrew/Cellar/kotlin/xxx/bin/kotlinc. stdlib lies in /opt/homebrew/Cellar/kotlin/xxx/libexec/lib/. Because kotlinc is resolved to that place, resolving lib like the code does now (before the PR) won't work as there is no lib-directory in the parent directory. We will have to navigate to libexec, and then lib.

Another example is sdkman installs. In that install, the current implementation would work as it is still has a lib-directory in the parent. kotlinc-location .sdkman/candidates/kotlin/1.7.10/bin/kotlinc. The lib-directory is there with the bin-directory in the parent.

Unsure if it will solve the snap issue (discussed in the issue linked to above). snap requires insane amounts of permissions, so wasn't able to get it working inside a docker container. Reminds me of why I avoided it when I still used Debian-based systems 😆 If any of you use snap, could you post the location of kotlinc and stdlib jar file? If you want to send in your own PR with a fix for it later, that is probably also awesome for the people using snap 🙂

I kept the takeIf-expression, even though we have already checked it for one of the possible options. The reason for not putting it into the let, is because I think it was way more readable this way.

Copy link
Owner

@fwcd fwcd left a comment

Choose a reason for hiding this comment

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

Looks good! The entire resolver is unfortunately a bit hacky, resolving the library correctly in every environment is pretty hard, given how much packaging of the Kotlin compiler differs across platforms, but adding a fallback for this seems reasonable.

@fwcd fwcd merged commit 4969c58 into fwcd:main Aug 12, 2022
@themkat themkat deleted the kotlinc_stdlib_location branch August 12, 2022 19:06
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.

2 participants