Skip to content
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

Populate java compiler args #109

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Conversation

Arthurm1
Copy link
Contributor

Populate the missing javac options - before it was the only the additional used specified options that appeared.

@nickzhums
Copy link
Member

@Arthurm1 Thank you so much for the contribution! Since there is a lot of PRs pending, it's gonna take some time for review, please allow us some time :) again, thanks so much!

@nickzhums
Copy link
Member

@Arthurm1 meanwhile, feel free us to let us know if you have any feedback using the product, anything is useful to us, thanks

Copy link
Member

@jdneo jdneo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the Checkstyle violations, this change looks good to me.

With this PR merged, looks like we can get rid of

https://github.com/microsoft/build-server-for-gradle/blob/develop/model/src/main/java/com/microsoft/java/bs/gradle/model/GradleSourceSet.java#L92-L100

and recover the source and target compatibility from the compiler args. I'll do that later.

@Arthurm1
Copy link
Contributor Author

Arthurm1 commented Dec 5, 2023

Fixed checkstyle issues

@jdneo
Copy link
Member

jdneo commented Dec 6, 2023

Since it's using internal apis, there might be chances to meet NoSuchClass/NoSuchMethod error.

Do we have public utils that can be used? If not, considering adding a try-catch block and use the original implementation as a fallback.

@Arthurm1
Copy link
Contributor Author

Arthurm1 commented Dec 6, 2023

I've wrapped the code in try-catch as there are no public classes that create the full args list. The other option is to duplicate the JavaCompilerArgumentsBuilder class but there's a lot of code in there and it's subject to change so it would have to be maintained.

I've added more tests as I wasn't handling the --release flag properly.

I've added code to cope with Groovy's use of GStringImpl when Strings are built using String interpolation e.g. "${variable}". This causes IllegalArgumentException when returning data through the tooling API so these have to be manually converted to Strings. Yuck

@Arthurm1
Copy link
Contributor Author

Arthurm1 commented Dec 7, 2023

Hopefully I've addressed all your points.

The source and target compatibility are now both set from the compiler args. Before, if a build had set the source/target directly in the args, then the compatibility settings would be wrong.
If nothing is set in the project's build file then Gradle automatically adds the current JVM version to the compiler args. So these should always be set in DefaultGradleSourceSet and hence in JvmBuildTargetEx.

I've added to the tests to check for both these issues.

Copy link
Member

@jdneo jdneo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

I should probably add a test for toolchains as well, in case they set the source/target/release in a different manner

Would you like to add a toolchain test in this PR? I'm both ok if merging the PR as it is.

@jdneo jdneo added the enhancement New feature or request label Dec 8, 2023
@jdneo jdneo added this to the 0.2.0 milestone Dec 8, 2023
@Arthurm1
Copy link
Contributor Author

Arthurm1 commented Dec 8, 2023

@jdneo I've added a toolchain test. No other changes. Hopefully good to merge now

@jdneo
Copy link
Member

jdneo commented Dec 11, 2023

Thank you @Arthurm1!

@jdneo jdneo merged commit b77ecfd into microsoft:develop Dec 11, 2023
4 checks passed
@Arthurm1 Arthurm1 deleted the compiler_args branch December 11, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants