-
Notifications
You must be signed in to change notification settings - Fork 46
Package OpenRewrite recipes in separate JAR, targeting Java 8 #1270
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before:
$ find error-prone-contrib -name "*.class" | xargs file | grep -oP 'version .*?$' | sort | uniq -c
1857 version 61.0
After:
$ find error-prone-contrib -name "*.class" | xargs file | grep -oP 'version .*?$' | sort | uniq -c
1766 version 52.0 (Java 1.8)
91 version 61.0
pom.xml
Outdated
<arg>--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg> | ||
</compilerArgs> | ||
<excludes> | ||
<exclude>**/*Rules.java</exclude> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One might think "why not customize the build only for *Recipes.java
files?" This appears not to work, because at the start of the compilation those files don't yet exist; this a maven-compiler-plugin
feature; not a javac
feature. Open to suggestions, of course!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this start! In my earlier exploration I had wondered if we could use <proc>only</proc>
to only generate the Java 8 compatible sources first (using Java 17/21), optionally to a different output, and then in a second pass compile those generated sources using Java 8 as target.
Might that perhaps help not to have this set of excludes and more targeted compilation for specific targets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting idea, to use <proc>only</proc>
! I just tried some variations of this, but unfortunately that seems to interact badly (different kinds of errors) with subsequent steps (using either <proc>node</proc>
or <proc>full</proc>
).
I then tried the "different output" route, where a JAR with a custom classifier (recipes
) is created: error-prone-contrib-0.17.1-SNAPSHOT-recipes.jar
. I push that change; would this work for OpenRewrite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A separate jar would work perfectly for us; Might help make for a lighter distribution for EPS itself as well. Thanks a lot!
Looks good. No mutations were possible for these changes. |
It's bed time, but this is surprising:
Need to understand how after packaging the |
Indeed, running |
Ah, the
Works 💪 |
Looks good. No mutations were possible for these changes. |
Looks good. No mutations were possible for these changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Looks good. No mutations were possible for these changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice improvements @Stephan202 🚀 !
Found two small typos :).
pom.xml
Outdated
<!-- OpenRewrite recipe sources, generated by the | ||
`rewrite-templating` annotation processor and | ||
identified as classes whose name ends in `Recipes`, are | ||
compiled to target Java 8 for compatiblity with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compiled to target Java 8 for compatiblity with the | |
compiled to target Java 8 for compatibility with the |
pom.xml
Outdated
</execution> | ||
<execution> | ||
<!-- Creates a custom JAR with Java 8-compatible | ||
OpenRewrite recipe classess. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenRewrite recipe classess. --> | |
OpenRewrite recipe classes. --> |
Looks good. No mutations were possible for these changes. |
1fed911
to
7f6d9a7
Compare
Looks good. No mutations were possible for these changes. |
7f6d9a7
to
ce0611e
Compare
Looks good. No mutations were possible for these changes. |
|
See openrewrite/rewrite-third-party#11 for context.
Opening this PR as draft, as it introduces an unacceptable self-check regression: it appears that the Refaster bug pattern no longer flags violations during our self check. I validated that it is executed, so the issue appears to be somewhere inside Refaster's (upstream, out-of-the-box) matching logic. Requires further investigation. We rely quite a lot on this feature, so IMHO the PR can't be merged without resolving this limitation.Went for a different solution: instead of attempting to produce Java 8 class files in the same location where previously we had Java 17 files, the recipes are now packaged into a separate JAR, with classifierrecipes
.Suggested commit message:
CC @timtebeek.