Skip to content

Commit

Permalink
Migrate the ant examples to use the -Xplugin integration
Browse files Browse the repository at this point in the history
and delete the legacy ant compiler adapter.

MOE_MIGRATED_REVID=209509256
  • Loading branch information
cushon committed Aug 21, 2018
1 parent 82b319c commit 3f2ca43
Show file tree
Hide file tree
Showing 11 changed files with 37 additions and 412 deletions.
75 changes: 0 additions & 75 deletions ant/pom.xml

This file was deleted.

This file was deleted.

This file was deleted.

27 changes: 0 additions & 27 deletions examples/ant/ant_fork/build.xml

This file was deleted.

25 changes: 0 additions & 25 deletions examples/ant/ant_lib/build.xml

This file was deleted.

23 changes: 0 additions & 23 deletions examples/ant/ant_lib/src/Main.java

This file was deleted.

37 changes: 37 additions & 0 deletions examples/ant/build.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<!--
Copyright 2018 The Error Prone Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
<project name="error prone example" default="compile">
<target name="compile">
<delete dir="build"/>
<mkdir dir="build"/>

<property name="error_prone_core.jar" location="${user.home}/.m2/repository/com/google/errorprone/error_prone_core/2.3.2-SNAPSHOT/error_prone_core-2.3.2-SNAPSHOT-with-dependencies.jar"/>
<property name="jformatstring.jar" location="${user.home}/.m2/repository/com/google/code/findbugs/jFormatString/3.0.0/jFormatString-3.0.0.jar"/>
<property name="javac.jar" location="${user.home}/.m2/repository/com/google/errorprone/javac/9+181-r4173-1/javac-9+181-r4173-1.jar"/>

<!-- using github.com/google/error-prone-javac is required when running on JDK 8 -->
<condition property="patch.javac" value="-XDempty" else="-J-Xbootclasspath/p:${javac.jar}">

This comment has been minimized.

Copy link
@tbroyer

tbroyer Aug 21, 2018

Contributor

I don't know what's more idiomatic but I would have used an if:set= or if:true= on the compilerarg to conditionally add it.

This comment has been minimized.

Copy link
@cushon

cushon Aug 21, 2018

Author Collaborator

Thanks, I didn't know that was a thing. Is this what you had in mind?

diff --git a/examples/ant/build.xml b/examples/ant/build.xml
index cfae09b60..92f6b0653 100644
--- a/examples/ant/build.xml
+++ b/examples/ant/build.xml
@@ -13,7 +13,7 @@
   See the License for the specific language governing permissions and
   limitations under the License.
   -->
-<project name="error prone example" default="compile">
+<project name="error prone example" default="compile" xmlns:unless="ant:unless">
   <target name="compile">
     <delete dir="build"/>
     <mkdir dir="build"/>
@@ -23,12 +23,12 @@
     <property name="javac.jar" location="${user.home}/.m2/repository/com/google/errorprone/javac/9+181-r4173-1/javac-9+181-r4173-1.jar"/>
 
     <!-- using github.com/google/error-prone-javac is required when running on JDK 8 -->
-    <condition property="patch.javac" value="-XDempty" else="-J-Xbootclasspath/p:${javac.jar}">
-        <javaversion atleast="9"/>
+    <condition property="patch.javac">
+      <javaversion atleast="9"/>
     </condition>
 
     <javac srcdir="src" destdir="build" fork="yes" includeantruntime="no">
-      <compilerarg value="${patch.javac}"/>
+      <compilerarg value="-J-Xbootclasspath/p:${javac.jar}" unless:set="patch.javac"/>
       <compilerarg line="-XDcompilePolicy=simple"/>
       <compilerarg line="-processorpath ${error_prone_core.jar}:${jformatstring.jar}"/>
       <compilerarg value="-Xplugin:ErrorProne -Xep:DeadException:ERROR" />

This comment has been minimized.

Copy link
@tbroyer

tbroyer Aug 21, 2018

Contributor

👍 (except the property name is misleading then!)

This comment has been minimized.

Copy link
@cushon

cushon Aug 21, 2018

Author Collaborator

I updated the examples in e8c1889 (including fixing the profile name). Thanks!

<javaversion atleast="9"/>
</condition>

<javac srcdir="src" destdir="build" fork="yes" includeantruntime="no">
<compilerarg value="${patch.javac}"/>
<compilerarg line="-XDcompilePolicy=simple"/>
<compilerarg line="-processorpath ${error_prone_core.jar}:${jformatstring.jar}"/>
<compilerarg value="-Xplugin:ErrorProne -Xep:DeadException:ERROR" />
</javac>
</target>
</project>
Loading

11 comments on commit 3f2ca43

@don-vip
Copy link
Contributor

@don-vip don-vip commented on 3f2ca43 Sep 3, 2018

Choose a reason for hiding this comment

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

-Xplugin is only available in JDK9, so error_prone 2.3.2 will no longer work with JDK8 with this change. Can you please keep the Ant compiler adapter? It can be marked as deprecated, many projects are still using Java 8.

@jbduncan
Copy link
Contributor

Choose a reason for hiding this comment

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

@don-vip I could be wrong about this, but I believe that's not true anymore.

IIUC, @tbroyer recently discovered a trick where the Java 8 boot classpath can be populated with the necessary error-prone artifacts. I don't know much more, admittedly, other than I understand that his new Gradle plugin gradle-errorprone-javacplugin-plugin uses this trick and thus supports Java 8 out-of-the-box now.

So if you use Gradle, that may be a way forward for you? :)

@don-vip
Copy link
Contributor

@don-vip don-vip commented on 3f2ca43 Sep 3, 2018

Choose a reason for hiding this comment

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

Oh, great. I wasn't aware of this trick. I'm trying it right now :)

@don-vip
Copy link
Contributor

@don-vip don-vip commented on 3f2ca43 Sep 3, 2018

Choose a reason for hiding this comment

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

I can't make it work. My task is now:

        <property name="error_prone_core.jar" location="${tools.dir}/error_prone_core.jar"/>
        <property name="error_prone_javac.jar" location="${tools.dir}/error_prone_javac.jar"/>
        <property name="jformatstring.jar" location="${spotbugs.dir}/jFormatString-3.0.0.jar"/>

        <javac sourcepath="" srcdir="${src.dir}" fork="yes"
            excludes="com/**,javax/**,oauth/**,org/apache/commons/**,org/glassfish/**,org/openstreetmap/gui/jmapviewer/Demo.java,org/openstreetmap/gui/jmapviewer/JMapViewerTree.java,org/openstreetmap/gui/jmapviewer/checkBoxTree/**,org/openstreetmap/josm/**,org/tukaani/**,gnu/**"
            destdir="${build.dir}" target="${java.lang.version}" source="${java.lang.version}" debug="on" includeantruntime="false" createMissingPackageInfoClass="false" encoding="UTF-8">
            <compilerarg value="-J-Xbootclasspath/p:${error_prone_javac.jar}" unless:set="isJava9"/>
            <compilerarg line="-XDcompilePolicy=simple"/>
            <compilerarg line="-processorpath ${error_prone_core.jar}:${jformatstring.jar}"/>
            <compilerarg value="-Xplugin:ErrorProne -Xep:CatchAndPrintStackTrace:OFF -Xep:ReferenceEquality:OFF -Xep:StringSplitter:OFF"/>
        </javac>

But I always get this error, I don't understand:

    [javac] Compiling 50 source files to C:\SVN\josm\core\build
    [javac] Using external javac compiler
    [javac] Compilation arguments:
    [javac] '-d'
    [javac] 'C:\SVN\josm\core\build'
    [javac] '-classpath'
    [javac] 'C:\SVN\josm\core\build'
    [javac] '-target'
    [javac] '1.8'
    [javac] '-encoding'
    [javac] 'UTF-8'
    [javac] '-g'
    [javac] '-J-Xbootclasspath/p:C:\SVN\josm\core\tools\error_prone_javac.jar'
    [javac] '-XDcompilePolicy=simple'
    [javac] '-processorpath'
    [javac] 'C:\SVN\josm\core\tools\error_prone_core.jar:C:\SVN\josm\core\tools\spotbugs\jFormatString-3.0.0.jar'
    [javac] '-Xplugin:ErrorProne -Xep:CatchAndPrintStackTrace:OFF -Xep:ReferenceEquality:OFF -Xep:StringSplitter:OFF'
    [javac] '-source'
    [javac] '1.8'
    [javac] 
    [javac] The ' characters around the executable and arguments are
    [javac] not part of the command.
    [javac] Files to be compiled:
    [javac]     ...
    [javac] error: plug-in not found: ErrorProne
    [javac] 1 error

I'm using Oracle JDK 8u181. Maybe that's an Oracle/OpenJDK difference?

@cushon
Copy link
Collaborator Author

@cushon cushon commented on 3f2ca43 Sep 4, 2018

Choose a reason for hiding this comment

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

plug-in not found: ErrorProne
Maybe that's an Oracle/OpenJDK difference?

@don-vip I doubt it's a difference with the Oracle JDK build. Are you sure the path to the Error Prone jar is correct? Are you using the shaded -with-dependencies.jar?

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented on 3f2ca43 Sep 4, 2018

Choose a reason for hiding this comment

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

IIUC, @tbroyer recently discovered a trick where the Java 8 boot classpath can be populated with the necessary error-prone artifacts.

Credit where credit is due: that was @cushon, in #535 (comment)

            <compilerarg line="-processorpath ${error_prone_core.jar}:${jformatstring.jar}"/>

On Windows, the path separator is ;, not : (I unfortunately don't know Ant enough to make this portable, but I'd probably also split this into two <compilerarg value=""/> –for -processorpath then its value to account for possible spaces in paths, which are quite common on Windows)

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented on 3f2ca43 Sep 4, 2018

Choose a reason for hiding this comment

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

Actually, this should probably use a <path> instead of properties for the error_prone_core.jar and jformatstring.jar dependencies; this could also be used for javac.jar (see https://ant.apache.org/manual/Tasks/javac.html#bootstrap)

Something like:

<path id="processorpath.ref">
  <pathelement location="${user.home}/.m2/repository/com/google/errorprone/error_prone_core/2.3.2-SNAPSHOT/error_prone_core-2.3.2-SNAPSHOT-with-dependencies.jar"/>
  <pathelement location="${user.home}/.m2/repository/com/google/code/findbugs/jFormatString/3.0.0/jFormatString-3.0.0.jar"/>
  <!-- Add annotation processors and Error Prone custom checks here if needed -->
</path>
…
      <compilerarg value="-processorpath"/>
      <compilerarg pathref="processorpath.ref"/>

@don-vip
Copy link
Contributor

@don-vip don-vip commented on 3f2ca43 Sep 4, 2018

Choose a reason for hiding this comment

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

@tbroyer thanks a lot! I confirm the portable solution (path+pathref) works nicely :)

@don-vip
Copy link
Contributor

@don-vip don-vip commented on 3f2ca43 Sep 5, 2018

Choose a reason for hiding this comment

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

@cushon I'm using the shaded core-with-dependencies.jar but it only contains "licence-compatible" dependencies, unlike previous ant jar which contained everything needed to run error_prone.

@don-vip
Copy link
Contributor

@don-vip don-vip commented on 3f2ca43 Sep 5, 2018

Choose a reason for hiding this comment

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

We're using this shade configuration for our ant build:

<configuration>
  <shadedArtifactAttached>true</shadedArtifactAttached>
  <shadedClassifierName>with-dependencies</shadedClassifierName>
  <createDependencyReducedPom>false</createDependencyReducedPom>
  <artifactSet>
    <excludes>
      <exclude>com.google.errorprone:javac</exclude>
    </excludes>
  </artifactSet>
</configuration>

@cushon
Copy link
Collaborator Author

@cushon cushon commented on 3f2ca43 Sep 14, 2018

Choose a reason for hiding this comment

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

I updated the ant installation instructions to match: 722be59 2e0d95a. Thanks @tbroyer!

Please sign in to comment.