feat: Add Java Module #314

jakubclark · 2019-09-08T00:09:56Z

Implemented a basic Java module, that shows the Java version, when inside a Java/Maven Project.

Description

New module java that calls javac --version, to get the version of Java
Simple tests for the formatting and call to javac
Updated the Dockerfile to install Java

Motivation and Context

Just another language that can be supported.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Screenshots (if appropriate):

MacOS:

Linux (WSL):

How Has This Been Tested?

I have tested using MacOS
I have tested using Linux
I have tested using Windows

Checklist:

I have updated the documentation accordingly.
I have updated the tests accordingly.

chipbuster

Hey @jakubclark, thanks for the PR!

This is looking pretty good already. You'll need to make a few minor changes due to a new PR that just got merged (sorry about the crummy timing) and there are a few questions I have, but the code is almost ready to merge (aside from those annoying version number issues you seem to be running into).

src/print.rs

tests/testsuite/java.rs

src/modules/java.rs

chipbuster

Just a few minor styling + comment nitpicks and then we'll be good to go, I think!

chipbuster · 2019-09-08T16:15:17Z

docs/config/README.md

+| Variable   | Default        | Description                                              |
+| ---------- | -------------- | -------------------------------------------------------- |
+| `symbol`   | `"☕ "`         | The symbol used before displaying the version of Java.  |
+| `style`    | `"bold RGB(166, 42, 42)"` | The style for the module.                     |


The syntax for colors is #RRGGBB, so the style would be bold #a62a2a

I think built-in modules should probably all use the 16 ANSI colors to remain compatible with as many emulators and configurations as possible.

Sure, I can change it to be red instead.

Didn't think about that. This is probably right. The color appears to be a darker version of the red color, perhaps dimmed red would be a good color?

docs/config/README.md

jakubclark · 2019-09-08T16:31:35Z

@chipbuster I've addressed your comments. Let me know if there is anything else to change!

chipbuster · 2019-09-08T16:47:28Z

@jakubclark I'm really sorry about this, I should have done this a little sooner. I just pulled and tested the prompt from your fork, and the addition of the java module tanks performance.

To give you an idea, we can get directory, git branch, git status, package, rust, and python info in 28ms, but adding a single java file raises that to 137.3ms.

Do you have any ideas on how we could potentially mitigate this timing problem? I can tell you that it's definitely noticeable--the reason I reached for hyperfine was because I was surprised at how slow the prompt felt.

Maybe it would be possible to parse the information out of java -version instead?

With the current speeds I'm seeing, I don't think we can justify putting this module into the prompt :\

jakubclark · 2019-09-08T16:53:03Z

@chipbuster I'll try getting the version from java -version.

I just chose javac because the output was simpler.

I'll do some performance testing and post the results here.

chipbuster · 2019-09-08T16:56:22Z

@jakubclark Thanks. I think the number we'd love to hit for java module render (via starship module java) would be 35-40ms, but 50 might be tolerable. Anything higher than that will most likely be questionable.

jakubclark · 2019-09-08T17:52:05Z

@chipbuster Unfortunately, using java -version doesn't help much:

Looks like java -version and javac -version are simply not fast enough.

chipbuster · 2019-09-08T18:58:59Z

@jakubclark Damn 😞

Unfortunately, while we're building towards support for several things that could help to mitigate this, like disabled-by-default modules, timeouts, caching data, and (perhaps someday) asynchronous rendering, none of those are quite ready at the moment. I'm afraid we might have to put this PR on hold until one of those comes in.

If you figure out some way to get the module time down without those, let us know and we'll be happy to merge this in!

nickwb · 2019-09-08T23:47:08Z

I had a similar issue when I tried to implement #182 - I'm considering implementing the version detection myself 😅

hdevalke · 2019-09-18T17:06:58Z

@chipbuster Unfortunately, using java -version doesn't help much:
Looks like java -version and javac -version are simply not fast enough.

At least on OSX the openjdk jvm installed a release file, I was able to get the version number under 5ms.

❯ hyperfine "cat $JAVA_HOME/release | grep JAVA_VERSION | grep -Eo '[0-9]+\\.[0-9]+\\.[0-9]+'"
Benchmark #1: cat /Library/Java/JavaVirtualMachines/jdk1.8.0_211.jdk/Contents/Home/release | grep JAVA_VERSION | grep -Eo '[0-9]+\.[0-9]+\.[0-9]+'
  Time (mean ± σ):       3.2 ms ±   0.1 ms    [User: 2.6 ms, System: 4.2 ms]
  Range (min … max):     3.0 ms …   4.3 ms    566 runs

If JAVA_HOME is not set this could be used

❯ hyperfine "cat $(/usr/libexec/java_home)/release | grep JAVA_VERSION | grep -Eo '[0-9]+\\.[0-9]+\\.[0-9]+'"
Benchmark #1: cat /Library/Java/JavaVirtualMachines/jdk-12.0.1.jdk/Contents/Home/release | grep JAVA_VERSION | grep -Eo '[0-9]+\.[0-9]+\.[0-9]+'
  Time (mean ± σ):       3.3 ms ±   0.7 ms    [User: 2.7 ms, System: 4.3 ms]
  Range (min … max):     3.0 ms …  16.1 ms    491 runs

Not sure for other OSes but caching java -version to some file could help to create a generic solution

hdevalke · 2019-09-18T21:31:46Z

Just found out there is an option -Xinternalversion this is the output on my mac:

❯ hyperfine  'java -Xinternalversion'
Benchmark #1: java -Xinternalversion
  Time (mean ± σ):      24.2 ms ±   7.3 ms    [User: 12.5 ms, System: 8.5 ms]
  Range (min … max):    22.4 ms …  69.8 ms    41 runs

On my Linux it is even below 5ms.

I tried to add this functionallity, but more testing is needed. hdevalke@51428ed

chipbuster · 2019-09-19T03:34:43Z

@hdevalke I think the -Xinternalversion option will be workable, even though parsing the output will be one hell of a task (our main restriction is process launch time, not Rust runtime, so the cost will be code complexity, not runtime).

Assuming the existence of any paths is a slightly dicey proposition, because we're making it a goal to (eventually) support all OSses, including things like BSDs, Windows, and Android (in Termux), so I'd prefer not to go for the JAVA_HOME or libexec options, but we can try to fall back to those if the internal version turns out to be unparseable.

@jakubclark Would you still be interested in working on this?

jakubclark · 2019-09-19T09:11:46Z

@chipbuster @hdevalke I'll try using java -Xinternalversion to extract the Java version using regular expressions from inside Rust and what's already been done in hdevalke@51428ed.

Get java version from internalversion system property

jakubclark · 2019-09-19T17:45:55Z

After making the necessary changes, here are the results from my mac:

chipbuster · 2019-09-19T18:29:31Z

@jakubclark

I get no noticeable difference on my Linux machine either. Looks like we got that timing issue nailed! Great work, and thanks to @hdevalke for the hint!

Do you know if this works cross-platform with both OpenJDK/JRE and Oracle Java? I can try to test this out in a docker container later if you don't know.

hdevalke · 2019-09-19T18:30:08Z

src/modules/java.rs

+}
+
+fn get_java_version() -> Option<String> {
+    match Command::new("java").arg("-Xinternalversion").output() {


Should we take into account the JAVA_HOME variable?
If exists the version could be $JAVA_HOME/bin/java -Xinternalversion.
When using maven it will take priority over the java binary in the PATH.

I like this idea. I'm not a fan of relying on JAVA_HOME being set (simply because half the envars that really ought to be set usually aren't), but I think checking for the existence of a JAVA_HOME variable should be more than fast enough.

Do you know if that variable gets set on Windows as well, or is that a UNIX-like-only thing?

Answer to my own question: it seems like (as with many envars) it's supposed to be set on windows, but a lot of installers fail to do so.

I don't think it's necessary to check $JAVA_HOME. Running $JAVA_HOME/bin/java -Xinternalversion should be identical to running java -Xinternalversion. If java isn't on the PATH environment variable, then Java simply isn't set up properly.

That's to say that if you set up a (maven?) environment, the PATH should automatically modify to put the newly-versioned Java binary first?

https://www.baeldung.com/maven-java-home-jdk-jre

First checks $JAVA_HOME, otherwise defaults to java on the $PATH.

But I've never had to set up separate Java installations and setting up $JAVA_HOME for different projects...

So maybe we should follow the same strategy as Maven:

Check for $JAVA_HOME.
Otherwise just use the java that is on the $PATH.

jakubclark · 2019-09-19T18:35:15Z

@chipbuster I have Oracle Java installed and it works for me.

Also, the result of running java -Xinternalversion on my raspberry pi (which has Oracle Java version 1.8) is here.

So I think it's safe to say that this works for both OpenJDK and Oracle versions.

chipbuster · 2019-09-19T18:37:39Z

Alright, excellent.

Looks like our CI is failing the standard test suite, though not the containerized tests...do we potentially have a test somewhere that should be #[ignore]d that isn't?

jakubclark · 2019-09-19T18:47:32Z

For the test suites, I don't know what the issue is. Could be that the result of running java -Xinternalversion might be different than expected, but hard to debug the cause. This is the specific failed test.

jakubclark · 2019-09-19T19:50:36Z

@chipbuster any suggestions on how to debug the Test Suites? I tried printing some data using eprintln!() but it was written to the logs.

chipbuster · 2019-09-19T19:55:32Z

@jakubclark Unfortunately, the CI is not my specialty.

Looking at the logs, it looks like the Java version simply isn't being detected at all (the actual string for the Java version is just a v followed by a color reset code). That suggests to me that the Java version isn't being correctly found.

Perhaps you could try something like

// whatever you need to get the full version string here
let ver = std::command::process("java")...
assert_eq!(ver,"nope");

This will obviously fail, but it'll let you see what's going on when you try call the java binary to get the version.

hdevalke · 2019-09-19T20:33:27Z

As this fails on Zulu maybe we could check the length of the version to be not zero:

if end == start {
  None
} else {
  Some(format!("v{}", &java_stdout[start..end]))
}

Support for openjdk could be the MVP, other JVM variants could be added later on.

hdevalke · 2019-09-19T20:52:05Z

I tested some other jvm builds:

for image in {azul/zulu-openjdk,adoptopenjdk,amazoncorretto}; do docker run --rm $image java -Xinternalversion; done
OpenJDK 64-Bit Server VM (25.222-b10) for linux-amd64 JRE (Zulu 8.40.0.25-CA-linux64) (1.8.0_222-b10), built on Jul 11 2019 11:36:39 by "zulu_re" with gcc 4.4.7 20120313 (Red Hat 4.4.7-3)
OpenJDK 64-Bit Server VM (12.0.2+10) for linux-amd64 JRE (12.0.2+10), built on Jul 18 2019 14:41:47 by "jenkins" with gcc 7.3.1 20180303 (Red Hat 7.3.1-5)
OpenJDK 64-Bit Server VM (25.222-b10) for linux-amd64 JRE (1.8.0_222-b10), built on Jul 11 2019 20:48:53 by "root" with gcc 7.3.1 20180303 (Red Hat 7.3.1-5)

jakubclark · 2019-09-19T20:53:30Z

I think you're right @hdevalke. It's at least a start.

jakubclark · 2019-09-19T21:53:13Z

Okay, I think this PR is (finally) in a state that it can be merged.

It's able to extract the java version from the java -Xinternalversion command, assuming that the output of the command contains something similar to "JRE (12.0.2+10)". It seems that the most common distributions of Java follow this pattern.

The Zulu distribution that comes with actions/setup-java does not follow this pattern, so an integration test wouldn't work in the CI.

We could include a local copy of the jdkFile from a specific vendor, but I don't think it makes sense to keep an entire JDK in this repo.

Thanks for all your comments and suggestions @chipbuster and @hdevalke !

chipbuster

Minor doc nitpick: do we need to update this?

src/modules/java.rs

docs/config/README.md

chipbuster

LGTM! Thanks for sticking through the (somewhat prolonged) process!

Once this passes CI, we'll go ahead and merge it in.

jakubclark · 2019-09-19T22:45:21Z

At least now we know a number of ways to get the Java version 😄

chipbuster · 2019-09-19T22:46:07Z

That we do! It's why I love working on these projects--you're never quite sure what you're going to learn next.

chipbuster · 2019-09-19T23:03:19Z

Woo! What a marathon of an issue. Thanks for all your work and for sticking it out!

@all-contributors, please add @jakubclark for code, doc, and tests.

allcontributors · 2019-09-19T23:03:30Z

@chipbuster

I've put up a pull request to add @jakubclark! 🎉

chipbuster · 2019-09-19T23:03:38Z

@all-contributors please also add @hdevalke for their invaluable ideas on this PR.

allcontributors · 2019-09-19T23:03:46Z

@chipbuster

I've put up a pull request to add @hdevalke! 🎉

hdevalke · 2019-09-20T05:03:42Z

@all-contributors please also add @hdevalke for their invaluable ideas on this PR.

Thank you!

allcontributors · 2019-09-20T05:04:08Z

@hdevalke

I've updated the pull request to add @hdevalke! 🎉

chipbuster reviewed Sep 8, 2019

View reviewed changes

src/print.rs Show resolved Hide resolved

tests/testsuite/java.rs Outdated Show resolved Hide resolved

src/modules/java.rs Show resolved Hide resolved

src/modules/java.rs Outdated Show resolved Hide resolved

src/modules/java.rs Outdated Show resolved Hide resolved

chipbuster reviewed Sep 8, 2019

View reviewed changes

jakubclark force-pushed the Implement-Java-Module branch from 36c2ad4 to f8f72eb Compare September 8, 2019 16:30

matchai added the on hold Progress on this issue or PR is currently suspended. label Sep 9, 2019

Kuba Clark and others added 6 commits September 19, 2019 19:08

Implemented a basic Java module

ffdf3c7

fix formatting

66fa1e0

Use update new config_value_style method

0a404d1

Update continous-integration.yml

a782285

Update documentation

ade0dcf

Fix style-value/example in documentation

d29de7a

jakubclark force-pushed the Implement-Java-Module branch from eae284c to bada063 Compare September 19, 2019 17:09

ear This is a combination of 4 commits.

b095738

Get java version from internalversion system property

jakubclark force-pushed the Implement-Java-Module branch from 74ac6cc to b095738 Compare September 19, 2019 17:39

hdevalke reviewed Sep 19, 2019

View reviewed changes

chipbuster added in progress ✨ enhancement A new feature implementation. and removed on hold Progress on this issue or PR is currently suspended. labels Sep 19, 2019

Check JAVA_HOME variable first

3487b8f

jakubclark force-pushed the Implement-Java-Module branch from 66944cc to 3487b8f Compare September 19, 2019 19:40

jakubclark added 2 commits September 19, 2019 22:02

Verbose tests to see ci config

8ebe2e5

fix formatting

040c24f

Support most common java distributions

ebb29ee

chipbuster reviewed Sep 19, 2019

View reviewed changes

src/modules/java.rs Outdated Show resolved Hide resolved

docs/config/README.md Outdated Show resolved Hide resolved

Update docs to reflect support extensions/files

17a31a6

chipbuster approved these changes Sep 19, 2019

View reviewed changes

chipbuster merged commit 71f03ec into starship:master Sep 19, 2019

allcontributors bot mentioned this pull request Sep 19, 2019

docs: add jakubclark as a contributor #395

Merged

allcontributors bot mentioned this pull request Sep 19, 2019

docs: add hdevalke as a contributor #396

Merged

jakubclark deleted the Implement-Java-Module branch September 19, 2019 23:08

feat: Add Java Module #314

feat: Add Java Module #314

Conversation

jakubclark commented Sep 8, 2019 • edited

Description

Motivation and Context

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

chipbuster left a comment

Choose a reason for hiding this comment

chipbuster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakubclark commented Sep 8, 2019

chipbuster commented Sep 8, 2019

jakubclark commented Sep 8, 2019

chipbuster commented Sep 8, 2019

jakubclark commented Sep 8, 2019

chipbuster commented Sep 8, 2019

nickwb commented Sep 8, 2019

hdevalke commented Sep 18, 2019

hdevalke commented Sep 18, 2019 • edited

chipbuster commented Sep 19, 2019 • edited

jakubclark commented Sep 19, 2019 • edited

jakubclark commented Sep 19, 2019

chipbuster commented Sep 19, 2019

Choose a reason for hiding this comment

chipbuster Sep 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakubclark commented Sep 19, 2019 • edited

chipbuster commented Sep 19, 2019

jakubclark commented Sep 19, 2019

jakubclark commented Sep 19, 2019

chipbuster commented Sep 19, 2019 • edited

hdevalke commented Sep 19, 2019

hdevalke commented Sep 19, 2019

jakubclark commented Sep 19, 2019

jakubclark commented Sep 19, 2019

chipbuster left a comment

Choose a reason for hiding this comment

chipbuster left a comment

Choose a reason for hiding this comment

jakubclark commented Sep 19, 2019

chipbuster commented Sep 19, 2019

chipbuster commented Sep 19, 2019 • edited

allcontributors bot commented Sep 19, 2019

chipbuster commented Sep 19, 2019

allcontributors bot commented Sep 19, 2019

hdevalke commented Sep 20, 2019

allcontributors bot commented Sep 20, 2019

jakubclark commented Sep 8, 2019 •

edited

hdevalke commented Sep 18, 2019 •

edited

chipbuster commented Sep 19, 2019 •

edited

jakubclark commented Sep 19, 2019 •

edited

chipbuster Sep 19, 2019 •

edited

jakubclark commented Sep 19, 2019 •

edited

chipbuster commented Sep 19, 2019 •

edited

chipbuster commented Sep 19, 2019 •

edited