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.

Sign up for GitHub

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

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

Merged
merged 1 commit into from
Sep 5, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .mvn/jvm.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
-XX:SoftRefLRUPolicyMSPerMB=10
-XX:+UseParallelGC
--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED
--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED
--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
--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED
16 changes: 12 additions & 4 deletions error-prone-contrib/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,16 @@ Two other goals that one may find relevant:
`target/pit-reports/index.html` files. For more information check the [PIT
Maven plugin][pitest-maven].

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.
Comment on lines -36 to +45
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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. :)


### Contribution guidelines

Expand Down Expand Up @@ -335,8 +342,9 @@ Refaster's expressiveness:
[forbidden-apis]: https://github.com/policeman-tools/forbidden-apis
[fossa]: https://fossa.io
[google-java-format]: https://github.com/google/google-java-format
[idea-288052]: https://youtrack.jetbrains.com/issue/IDEA-288052
[maven]: https://maven.apache.org
[modernizer-maven-plugin]: https://github.com/gaul/modernizer-maven-plugin
[sonarcloud]: https://sonarcloud.io
[pitest]: https://pitest.org
[pitest-maven]: https://pitest.org/quickstart/maven
[sonarcloud]: https://sonarcloud.io
Comment on lines -340 to +350
Copy link
Member Author

Choose a reason for hiding this comment

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

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

5 changes: 0 additions & 5 deletions error-prone-contrib/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,6 @@
<artifactId>jsr305</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>javac</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>com.google.googlejavaformat</groupId>
<artifactId>google-java-format</artifactId>
Expand Down
50 changes: 8 additions & 42 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,14 @@
-XX:SoftRefLRUPolicyMSPerMB=10
-XX:+UseParallelGC
--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED
--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED
--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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED
<!-- The test JVMs are short-running. By disabling certain
expensive JIT optimizations we actually speed up most tests. -->
Expand Down Expand Up @@ -252,11 +252,6 @@
<artifactId>jsr305</artifactId>
<version>3.0.2</version>
</dependency>
<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>javac</artifactId>
<version>9+181-r4173-1</version>
</dependency>
<dependency>
<groupId>com.google.googlejavaformat</groupId>
<artifactId>google-java-format</artifactId>
Expand Down Expand Up @@ -846,13 +841,19 @@
</path>
</annotationProcessorPaths>
<compilerArgs>
<arg>--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
<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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

<arg>-Xmaxerrs</arg>
<arg>10000</arg>
<arg>-Xmaxwarns</arg>
<arg>10000</arg>
</compilerArgs>
<parameters>true</parameters>
<release>${version.jdk}</release>
<source>${version.jdk}</source>
<target>${version.jdk}</target>
Comment on lines -855 to +856
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Let's leave it at this then.

<!-- Erroneously inverted logic... for details, see
https://issues.apache.org/jira/browse/MCOMPILER-209. -->
<useIncrementalCompilation>false</useIncrementalCompilation>
Expand Down Expand Up @@ -1717,41 +1718,6 @@
</pluginManagement>
</build>
</profile>
<profile>
<!-- Some code in this project interfaces directly with the Java
compiler. The following `add-exports` arguments are not necessary
for the code to compile because `com.google.errorprone:javac` is on
the classpath. In fact, enabling this profile when building with
Maven on the command line will cause a build failure, because these
flags are incompatible with the `release` flag. This profile exists
solely to be enabled within an IDE: without them IntelliJ IDEA
reports compilation errors. -->
<id>add-exports</id>
<build>
<pluginManagement>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<compilerArgs combine.children="append">
<arg>--add-exports</arg>
<arg>jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
<arg>--add-exports</arg>
<arg>jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
<arg>--add-exports</arg>
<arg>jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
<arg>--add-exports</arg>
<arg>jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
<arg>--add-exports</arg>
<arg>jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
</compilerArgs>
</configuration>
</plugin>
</plugins>
</pluginManagement>
</build>
</profile>
<profile>
<!-- This profile is auto-activated when performing a release. -->
<id>release</id>
Expand Down
5 changes: 0 additions & 5 deletions refaster-compiler/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,6 @@
<artifactId>jsr305</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>javac</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
Expand Down
5 changes: 0 additions & 5 deletions refaster-runner/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,6 @@
<artifactId>jsr305</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>javac</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
Expand Down
5 changes: 0 additions & 5 deletions refaster-support/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,6 @@
<artifactId>jsr305</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>javac</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
Expand Down
5 changes: 0 additions & 5 deletions refaster-test-support/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,6 @@
<artifactId>jsr305</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>javac</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
Expand Down