microsoft / build-server-for-gradle

An implementation of the Build Server Protocol for Gradle
MIT License
55 stars 8 forks source link

Handle java source/target defined at extension level #188

Closed Arthurm1 closed 1 month ago

Arthurm1 commented 2 months ago

This will take into account the java plugin source/target compatibility settings. e.g. using...

java {
  targetCompatibility = JavaVersion.VERSION_1_8
  sourceCompatibility = JavaVersion.VERSION_1_9
}

instead of

tasks.withType(JavaCompile) {
  sourceCompatibility = JavaVersion.VERSION_1_8
  targetCompatibility = JavaVersion.VERSION_1_9
}

I'm not sure if this is enough to fix https://github.com/redhat-developer/vscode-java/issues/3721. The source/target compatibility in JVMBuildTarget will have changed but the javac options will still contain --release 8. Hopefully that will be ok for the eclipse settings.

I note that here it says to configure the compileJava task with source/target and not the java plugin when using Jabel

I've also extended the POJO constructor copying used in DefaultSourceSets to the language extensions. It's not needed for normal running but it's a pain when debugging as all the variables/watches just show the proxy classes.

jdneo commented 2 months ago

I've also extended the POJO constructor copying used in DefaultSourceSets to the language extensions. It's not needed for normal running but it's a pain when debugging as all the variables/watches just show the proxy classes.

There is an issue about this: https://github.com/microsoft/build-server-for-gradle/issues/175. I also did some investigation and turns out the copy constructor is not needed for running.

I'm thinking that, for debugging purpose, maybe override toString() is a better approach than using the copy constructor?


Update:

The copy constructor is required. If we remove all the copy constructor, here:

https://github.com/microsoft/build-server-for-gradle/blob/395b97757da8d0d82bc2c68c4aca4331bea4e584/model/src/main/java/com/microsoft/java/bs/gradle/model/actions/GetSourceSetsAction.java#L174

The sourceset is a proxy wrapperred type, and it will encounter some problem

image

Arthurm1 commented 2 months ago

The copy constructor is required. If we remove all the copy constructor, here:

Shall I leave my changes to DefaultJavaExtension/DefaultScalaExtension then or do you want them gone?

jdneo commented 2 months ago

Shall I leave my changes to DefaultJavaExtension/DefaultScalaExtension

I think you can leave as it is. Until we find a way to get rid of the copy constructor entirely

jdneo commented 2 months ago

@Arthurm1

I think we should use javaCompile.getSourceCompatibility() and javaCompile.getTargetCompatibility().

AFAIK, there are three different kind of ways to set the source/target level.

1.

java {
  targetCompatibility = JavaVersion.VERSION_1_8
  sourceCompatibility = JavaVersion.VERSION_1_9
}

2.

tasks.withType(JavaCompile) {
  sourceCompatibility = JavaVersion.VERSION_1_8
  targetCompatibility = JavaVersion.VERSION_1_9
}

3.

sourceCompatibility = JavaVersion.VERSION_1_8
targetCompatibility = JavaVersion.VERSION_1_9

The current approach cannot get the version in case 2. While using the JavaCompile instance can cover all three cases.

Considering JavaCompile is official API which no need to use reflection, I prefer to use it directly.

jdneo commented 2 months ago

Hi @Arthurm1, would you like to update the PR?

Or do you have any concern about javaCompile.getSourceCompatibility() and javaCompile.getTargetCompatibility()

Arthurm1 commented 2 months ago

@jdneo Will look at it this weekend and probably change the PR

jdneo commented 2 months ago

@Arthurm1 Thank you.

My thought is that we can just simply get the source/target level from JavaCompile, no need to inspecting from compile args. (Correct me if this is working for any cases)

Arthurm1 commented 1 month ago

@jdneo I've changed it so that the source/target compatibility is populated purely from the JavaCompile task. JavacOptions are still passed back for BSP but they are ignored for compatibility settings.

This seems to reflect more how people use the Gradle settings, i.e. set source/target compatibility in JavaCompile for IDE usage and the JavacOptions for jar and release.

jdneo commented 1 month ago

Thank you @Arthurm1