Get jdk info

[SocksDevil]

Closes #7 . Properly populates the jvm build target with information regarding the javaHome version, currently obtained from the jdk runtime used by the client, and the java version used, obtained from the toolchain through an aspect.

I'm leaving this PR drafted since there is a matter at hand that me and @jastice are not exactly too sure about, which is if returning the javaHome of the jdk runtime used to run the code is a proper answer.

Moreover, I want to try and obtain the javaHome through an aspect as well. This also needs tests, which I plan on adding with the structure from #42 .

[agluszak]

general recommendation: BazelBspServerBuildManager deserves a refactor. Unless I'm missing something here, I think that all these "private getters" could be run simultaneously during the construction and therefore they would no longer be "private getters" but rather simple steps of the initialization. This would help reduce the spaghettiness of this class. We need to think about which field can be null and which cannot (i.e. it is an error when we fail to obtain them from bazel).

[agluszak]

does it make sense to ever return null as java version? Isn't it an error when it cannot be obtained?

[SocksDevil]

The inability to provide the java version is not necessarily an error, though it does mean that the user must set it manually afterwards. This is the case for the protocol itself.

In our case, if the javaVersion is not obtained it means there's some bug on our side, but I don't think we should return as an error to the client. Perhaps we should invest in error logging, since this is definitely not the only case.

[agluszak]

what does this method do? Its name doesn't explain much

[SocksDevil]

For this method a list of rules' attributes is passed and the rule graph is traversed. I'm also trying to remove the need for this method as a whole.

[agluszak]

this method does two things: it sets the value of the nullable field javaVersion and returns an optional. Why not make the field itself optional? Another question: what should happen if we fail to find the java version? With current logic we will try to do it each time this method is called, regardless of whether it failed before.

[agluszak]

I see that this is a common practice with the rest of the getters in this class as well. I don't find it very readable tbh

[SocksDevil]

My first implementation was as you mentioned as well, but due to the other getters, I ended up doing the same, just for the sake of consistency for now. Tho I agree on a refactor, we definitely need a method of lazy loading these values, I'm not sure this the place to do it tho. Wdyt?

[agluszak]

why is this rule passed around so many times? It's pretty much spaghetti code

[SocksDevil]

This rule is passed around so much since the rule holds most of the information regarding the current target. I think we need a specific class that holds all these getters.

[abrams27]

it can be done in the single call (something like that):

Suggested change

	List<String> traversingPath = Lists.newArrayList("$jvm", "$java_runtime", ":alias", "actual");
	Optional<Build.Rule> jdkRule = traverseDependency(rule, traversingPath);
	if (!jdkRule.isPresent()) {
	return Optional.empty();
	}
	String jdkLocation = jdkRule.get().getLocation();
	String localJdkPath = jdkLocation.substring(0, jdkLocation.indexOf("/BUILD"));
	return Optional.of(localJdkPath);
	}
	List<String> traversingPath = Lists.newArrayList("$jvm", "$java_runtime", ":alias", "actual");
	return traverseDependency(rule, traversingPath)
	.map(Build.Rule::getLocation)
	.map(location -> location.substring(0, jdkLocation.indexOf("/BUILD"));
	}

[abrams27]

i think that this huuge code block could be splitted + for could be replaced with stream

[abrams27]

prob this file is unnecessary, as well as other logs

[SocksDevil]

Thanks! Didn't notice they were there, will make sure the gitignores are updated as well