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
bugfix: Use classpath jar for defining run classpath #4868
Conversation
c8974e4
to
b0d536e
Compare
Opinions? Should we always add the classpath jar or add a specific case when the classpath is too long? Having it always makes the commandline much shorter. |
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.
Hey! This make sense to me. Nice solution. Just a couple questions/nit.
metals/src/main/scala/scala/meta/internal/metals/debug/ExtendedScalaMainClass.scala
Outdated
Show resolved
Hide resolved
metals/src/main/scala/scala/meta/internal/metals/debug/ExtendedScalaMainClass.scala
Outdated
Show resolved
Hide resolved
|
||
val out = Files.newOutputStream(manifestJar.toNIO) | ||
|
||
Using.resource(new jar.JarOutputStream(out, manifest)) { jol => |
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.
Just ignore me if this doesn't make sense because in my head I was trying to think about what this looks like but was struggling with it. Since we're managing mutliple resources here should we maybe be using Using.Manager
, or does that not fit here because of the jol == null
check deciding what we call close()
on. That's not 100% clear to me.
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.
+1 for answers! :D
- Shouldn't
JarOutputStream
take care of closingout
? - can constructor of
JarOutputStream
return null?
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.
- It does close
out
so it's safe to only run close one one. - It can't, but it can throw.
I reworked it a bit and also wrapped it all in Try to be on the safe side.
metals/src/main/scala/scala/meta/internal/metals/debug/ExtendedScalaMainClass.scala
Outdated
Show resolved
Hide resolved
b0d536e
to
b0b40ad
Compare
metals/src/main/scala/scala/meta/internal/metals/debug/ExtendedScalaMainClass.scala
Outdated
Show resolved
Hide resolved
b0b40ad
to
c4443f1
Compare
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.
LGTM!
Previously, we would use the classpath as string which might be too long on some systems for example on Windows. Now, we define it using jar with the classpath defined inside its manifest. This also benefits other system since it makes the path shorter. The jar will only be created once per classpath change and deleted on exit. Fixes scalameta#4827
c4443f1
to
78d1e30
Compare
Previously, we would use the classpath as string which might be too long on some systems for example on Windows. Now, we define it using jar with the classpath defined inside its manifest. This also benefits other system since it makes the path shorter.
The jar will only be created once per classpath change and deleted on exit.
Fixes #4827