Add support for source JARs with Maven

[daplf]

This adds supports for source JARs with Maven.

It seems to work fine. I created a data class to hold compiled and source JARs. Source JARs are downloaded asynchronously. The JARs are downloaded using mvn dependency:sources.

When a source jar is unavailable we still decompile the bytecode. If the source jar is available, we just open it. This required a new query parameter to be added to the KLS URI, in order for the jarClassContents request to be able to know when to decompile and when to fetch the source JAR.

I tested all the features showcased in the README and nothing broke, but I'm sure there's stuff I might have missed. I'll try to test more things if I remember anything I overlooked. If you can think of any other scenarios to test, let me know so I can try them out.

I did not include Gradle support for this yet, since I'm not really sure what the best approach would be there at the moment and I'd prefer to have some feedback regarding the current mechanism before adding Gradle support.

Maybe we should also include a configuration property to enable/disable downloading sources? Not sure if it makes sense.

This would partially complete #217 (missing Gradle support)

[fwcd]

Nice, thank you for looking into this! I think adding a query parameter is a good solution and a config option sounds like a good idea too, though we can always add that later.

For Gradle support I would ideally like to move forward with #85 first, since the tooling API provides first-class support for things like source/javadoc JAR resolution, though I'm not sure how well that would mix with Android projects, which currently have quite a bit of additional class path resolution logic in projectClassPathFinder.gradle.

[daplf]

a config option sounds like a good idea too, though we can always add that later.

I'll add it later then to avoid introducing too many things here 🙂

Regarding Gradle, I totally agree.

[daplf]

@fwcd I updated the PR as per your suggestions.

Just an FYI, I found an issue with the way we are executing the maven dependency plugin (both on the list and sources goals). For some reason, when the outputFile is specified, the second part of each line contains some garbage characters, like so:

org.springframework.boot:spring-boot-starter:jar:sources:2.4.5�[36m -- module spring.boot.starter�[0;1m [auto]�[m

This led to a bug on the sources, because it was preventing the resolver from capturing the correct version for some artifacts. This issue also happens on the list goal, but for that one it's fine, because the garbage characters appear after the compile section, which isn't needed.

The solution for now was to drop the outputFile and redirect the output instead. This works fine. I'll try to find out find out why this is happening when I can

[fwcd]

Thanks for the updates! The Maven issue looks like some codes/escape codes got included in the output, you could try adding something like -Dstyle.color=never and see whether that fixes the issue. Piping using > might be shell-dependent and something I'd ideally like to avoid if possible.

[daplf]

Thanks for the updates! The Maven issue looks like some codes/escape codes got included in the output, you could try adding something like -Dstyle.color=never and see whether that fixes the issue. Piping using > might be shell-dependent and something I'd ideally like to avoid if possible.

Worked, thanks! Updated the PR

[fwcd]

I think, we have to be careful here, not to introduce any race conditions. The Compiler instance expects a full class path AFAICT, so perhaps we should keep it as a synchronous operation?

[daplf]

You're right. Although making it synchronous might significantly delay the language server startup, especially for bigger projects. This should only happen on the first time (on subsequent startups, no downloading will be required), but I don't know how I feel about this

[daplf]

Ideally, we should make the update atomic. Not sure how that will affect other parts of the code though

[daplf]

@fwcd Any thoughts on this?

[fwcd]

Not sure, how would the language server be able to compile the project (and provide useful diagnostics) without the full class path?

Sorry, I misread your snippet. The base class path is computed synchronously and just the source jars are fetched asynchronously, right? I hope to find some time to take a closer look at this soon.

[daplf]

The base class path is computed synchronously and just the source jars are fetched asynchronously, right?

That is correct. My motivation to make it async is that source JARs are not a "vital" feature IMO. And for large projects it might take some time to download all the JARs the first time.

[daplf]

@fwcd Hey. I just tried using the language server on master and I noticed the go to definition is broken. I'm getting an exception every time I try to go to a definition:

java.nio.file.InvalidPathException: Illegal char <:> at index 4:

This happens when calling KlsURI.getJarPath, which is internally calling Paths.get with a URI starting with file://.

Are you aware of this? I noticed you made some tweaks after merging. Does it work on your machine?