Skip to content
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

Handle kotlinc symbolic link for determining KOTLIN_DIR #2175

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Handle kotlinc symbolic link for determining KOTLIN_DIR #2175

merged 1 commit into from
Oct 10, 2023

Conversation

agcom
Copy link
Contributor

@agcom agcom commented Oct 7, 2023

The KOTLIN_DIR variable needs to point to the compiler binary directory containing the kotlinc binary; because, latter in the script, KOTLIN_DIR is used to point to the kotlin-stdlib.jar; sometimes kotlinc would be just a symbolic link residing in a directory included in the PATH environment variable.

It is possible to manually set the KOTLIN_DIR variable; but, with this change, unzipping the Kotlin installation and creating a symbolic link would suffice; see DOMjudge/domjudge-packaging#156.

@agcom agcom marked this pull request as draft October 7, 2023 08:11
@nickygerritsen
Copy link
Member

I'd like input from others as well here.

@agcom agcom marked this pull request as ready for review October 7, 2023 09:04
@eldering
Copy link
Member

eldering commented Oct 8, 2023

LGTM, but please squash the commits into a single one before merging.

@vmcj
Copy link
Member

vmcj commented Oct 10, 2023

@agcom can you squash the commits? Afterwards we'll merge it.

Find the real path of `kotlinc` in case it is a symbolic link, ensuring that the correct compiler directory containing `kotlinc` is determined. Previously, it directly used `dirname` on the result of `command -v kotlinc`, which could have led to incorrect directory when `kotlinc` was a symbolic link, and as a result, not finding the `kotlin-stdlib.jar` latter in the script.
@agcom
Copy link
Contributor Author

agcom commented Oct 10, 2023

@vmcj There you go. I thought there is an auto squash and merge option.

@vmcj
Copy link
Member

vmcj commented Oct 10, 2023

@vmcj There you go. I thought there is an auto squash and merge option.

There is, but some people like to have control on how the resulting message looks like.

@vmcj vmcj merged commit c9add07 into DOMjudge:main Oct 10, 2023
1 check passed
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.

4 participants