-
Notifications
You must be signed in to change notification settings - Fork 270
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
Fix tools.jar path for jbre. #309
Fix tools.jar path for jbre. #309
Conversation
Also removed the lines adding the tools.jar of the JVM gradle is run on.
if (executable) classpath += project.files("${new File(executable).parent}/../../lib/tools.jar") | ||
def toolsJar = Jvm.current().toolsJar | ||
if (toolsJar != null) classpath += project.files(toolsJar) | ||
if (executable) classpath += project.files("${new File(executable).parent}/../lib/tools.jar") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if executable is not set?
Are you sure about the path to tools jar in different jre (JetBrains and oracle) for different oses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we don't need to check for null
as we can assume executable is always set https://docs.gradle.org/current/dsl/org.gradle.api.tasks.JavaExec.html#org.gradle.api.tasks.JavaExec:executable. Also, the convention mapping declared at IntelliJPlugin#322
never returns null. Let me verify if it's safe to assume the path to tools.jar
is always the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on this https://docs.oracle.com/javase/8/docs/technotes/tools/windows/jdkfiles.html , it's safe to assume about the path to tools.jar
. Looks like jbre is using the same layout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much. Could you also bump the version in build.gradle (optionally modify changelog file) and run ‘documetr’ task please? So I could upload the plugin with one click.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zolotov done. I've opted out on updating the changelog 😂 .
Also removed the lines adding the tools.jar of the JVM gradle is run on. Haven't tested it locally yet, having trouble running the tests. @zolotov can you guide me on this? Need to somehow publish to plugin locally and test it out against the spek repo.
Resolves #307.