Drop the dependency on `com.google.errorprone:javac` #197

Stephan202 · 2022-08-14T12:44:47Z

Suggested commit message:

Drop the dependency on `com.google.errorprone:javac` (#197)

This new setup matches the upstream Error Prone build configuration and
simplifies development against JDK 11+ internal APIs.

This new setup matches the upstream Error Prone build configuration and simplifies development against JDK 11+ internal APIs.

Stephan202

Some context.

Stephan202 · 2022-08-14T12:45:21Z

pom.xml

-                        <release>${version.jdk}</release>
+                        <source>${version.jdk}</source>
+                        <target>${version.jdk}</target>


Javac does not allow --release i.c.w. the above --add-exports flags.

https://openjdk.org/jeps/247 Do we want to link to a related explanation or not really?

I don't think that JEP link would clarify too much unless one reads very carefully. Any person wondering why we're not using --release will likely give it a try and find out 🙃

I'm not necessarily opposed to adding a comment, but it'd be a rather complicated sentence. Not sure it's worth the effort.

Let's leave it at this then.

Stephan202 · 2022-08-14T12:46:46Z

error-prone-contrib/README.md

-When loading the project in IntelliJ IDEA (and perhaps other IDEs) errors about
-the inaccessibility of `com.sun.tools.javac.*` classes may be reported. If this
-happens, configure your IDE to enable the `add-exports` profile.
+When running the project's tests in IntelliJ IDEA, you might see the following
+error:
+```
+java: exporting a package from system module jdk.compiler is not allowed with --release
+```
+
+If this happens, go to _Settings -> Build, Execution, Deployment -> Compiler ->
+Java Compiler_ and deselect the option _Use '--release' option for
+cross-compilation (Java 9 and later)_. See [IDEA-288052][idea-288052] for
+details.


We're trading one manual bit of configuration for another; acceptable, IMHO.

IMO this is easier to detect and fix then the previous manual config.

I think this is also nicer, as the previous profile existed solely for the purpose of IDEA: now we've reduced it to a comment (due to a bug) so IMO this is an improvement. :)

Stephan202 · 2022-08-14T12:47:25Z

error-prone-contrib/README.md

-[sonarcloud]: https://sonarcloud.io
 [pitest]: https://pitest.org
 [pitest-maven]: https://pitest.org/quickstart/maven
+[sonarcloud]: https://sonarcloud.io


To be sure: there's a lot more stuff to fix/update/remove in this README, but that's out of scope.

rickie

Changes LGTM.

One proposal but already approving.

rickie · 2022-09-05T10:45:42Z

pom.xml

-                        <release>${version.jdk}</release>
+                        <source>${version.jdk}</source>
+                        <target>${version.jdk}</target>


https://openjdk.org/jeps/247 Do we want to link to a related explanation or not really?

rickie · 2022-09-05T10:47:52Z

error-prone-contrib/README.md

-When loading the project in IntelliJ IDEA (and perhaps other IDEs) errors about
-the inaccessibility of `com.sun.tools.javac.*` classes may be reported. If this
-happens, configure your IDE to enable the `add-exports` profile.
+When running the project's tests in IntelliJ IDEA, you might see the following
+error:
+```
+java: exporting a package from system module jdk.compiler is not allowed with --release
+```
+
+If this happens, go to _Settings -> Build, Execution, Deployment -> Compiler ->
+Java Compiler_ and deselect the option _Use '--release' option for
+cross-compilation (Java 9 and later)_. See [IDEA-288052][idea-288052] for
+details.


IMO this is easier to detect and fix then the previous manual config.

nathankooij

Just one question and not a blocker, but feel a bit underqualified to review this. :)

nathankooij · 2022-09-05T12:42:04Z

error-prone-contrib/README.md

-When loading the project in IntelliJ IDEA (and perhaps other IDEs) errors about
-the inaccessibility of `com.sun.tools.javac.*` classes may be reported. If this
-happens, configure your IDE to enable the `add-exports` profile.
+When running the project's tests in IntelliJ IDEA, you might see the following
+error:
+```
+java: exporting a package from system module jdk.compiler is not allowed with --release
+```
+
+If this happens, go to _Settings -> Build, Execution, Deployment -> Compiler ->
+Java Compiler_ and deselect the option _Use '--release' option for
+cross-compilation (Java 9 and later)_. See [IDEA-288052][idea-288052] for
+details.


I think this is also nicer, as the previous profile existed solely for the purpose of IDEA: now we've reduced it to a comment (due to a bug) so IMO this is an improvement. :)

nathankooij · 2022-09-05T12:45:15Z

pom.xml

            --add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED
            --add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED
            --add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED
            --add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED
            --add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED
            --add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED
            --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED
-            --add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED


I didn't check in detail what we do(n't) need from this module, so trust that this reduced visibility is OK. :)

The entry added above is for the same module, but now with --add-exports rather than --add-opens. So IIUC this means that we no longer rely on reflective access non-public members of this module. A nice improvement :)

Indeed! I meant that I didn't go through what we need but if it compiles, then idd a very nice improvement. 😄

nathankooij · 2022-09-05T12:46:16Z

pom.xml

+                            <arg>--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
+                            <arg>--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
+                            <arg>--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
+                            <arg>--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>


How come we don't need to match what we have in jvm.config here?

TBH: we could! The flags in <argLine> and ./mvn/jvm.config specify which modules can be accessed at (test) runtime. The flags in <compilerArgs> specify which modules can be referenced explicitly by the code being compiled. The list here is the minimal configuration required to compile the current code; over time this list might shrink or expand. (And likewise for the other two sets of flags, though ideally we don't rely on any compiler internals not already depended on by Error Prone itself.)

Ack. Was mostly concerned with having to do triple bookkeeping, but keeping it minimal sounds like a good improvement. 👍

Drop the dependency on com.google.errorprone:javac

773226a

This new setup matches the upstream Error Prone build configuration and simplifies development against JDK 11+ internal APIs.

Stephan202 added this to the 0.1.1 milestone Aug 14, 2022

Stephan202 requested a review from rickie August 14, 2022 12:44

Stephan202 commented Aug 14, 2022

View reviewed changes

Stephan202 mentioned this pull request Aug 14, 2022

Update all tests to use text blocks #198

Draft

rickie modified the milestones: 0.1.1, 0.1.2 Aug 22, 2022

rickie added the improvement label Aug 25, 2022

Stephan202 mentioned this pull request Aug 28, 2022

Introduce {CONTRIBUTING,LICENSE,README}.md and Error Prone Support's logo #212

Merged

rickie approved these changes Sep 5, 2022

View reviewed changes

nathankooij approved these changes Sep 5, 2022

View reviewed changes

Stephan202 merged commit 4c8e125 into master Sep 5, 2022

Stephan202 deleted the sschroevers/drop-javac-dependency branch September 5, 2022 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop the dependency on `com.google.errorprone:javac` #197

Drop the dependency on `com.google.errorprone:javac` #197

Stephan202 commented Aug 14, 2022

Stephan202 left a comment

Stephan202 Aug 14, 2022

rickie Sep 5, 2022

Stephan202 Sep 5, 2022

rickie Sep 5, 2022

Stephan202 Aug 14, 2022

rickie Sep 5, 2022

nathankooij Sep 5, 2022

Stephan202 Aug 14, 2022

rickie left a comment

rickie Sep 5, 2022

rickie Sep 5, 2022

nathankooij left a comment

nathankooij Sep 5, 2022

nathankooij Sep 5, 2022

Stephan202 Sep 5, 2022

nathankooij Sep 5, 2022

nathankooij Sep 5, 2022

Stephan202 Sep 5, 2022

nathankooij Sep 5, 2022

Drop the dependency on com.google.errorprone:javac #197

Drop the dependency on com.google.errorprone:javac #197

Conversation

Stephan202 commented Aug 14, 2022

Stephan202 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

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

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathankooij 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drop the dependency on `com.google.errorprone:javac` #197

Drop the dependency on `com.google.errorprone:javac` #197