-
Notifications
You must be signed in to change notification settings - Fork 1.2k
build: check vectors code with error-prone #14917
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
base: main
Are you sure you want to change the base?
Conversation
Previously these sources were excluded. But these are where we'd like to have the most checks.
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
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.
I think the reason for this exclusion is that error prone didn't handle newer java versions... so if we have a java25+ source folder for mr-jar, it may still be applicable.
I wonder if we should just move the existing java24 folder sources into the main location and keep the exclusion here? Or maybe add a separate build option that will explicitly say up to which Java version errorprone can be included/ is compatible with?
@@ -77,12 +77,6 @@ allprojects { prj -> | |||
} | |||
|
|||
tasks.withType(JavaCompile).configureEach { task -> | |||
// Disable errorprone on the MR-JAR tasks |
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.
i think this was added when the MR-JAR code required newer JDK to be installed in former times. This is now obsolete as we have APIJARs and on top at moment we compile with default compiler (24).
The exclusion had another reason. Nowadays we compile the MR-JAR part with default compiler always, in former times we stripped preview flags afterwars and required newer compiler for later java versions. This is no longer the case, the MR-JAR parts are compiled with module patching and default compiler, so the exclusion is obsolete. No problem in removing the check it is a relic, separating the Java classes should still be done. |
The separate sourceSet is important also to not let incubator classes enter public APIs. The code separattion makes it isolated (and we have the stub compilation so when later versions change we have no issue and can enable MR-JAR again. |
I figured there was a reason checks were disabled: I think even if we have to disable these checks in stable branches, it would be valuable if we can keep them functional for That's where the "front lines" are for new vectors code or modifications in pull requests. |
Yes the reason was at very early times that we needed to compile with newer JDKs (19, 20, 21) while the main branch at that time was on 11 or 17. This lead to incomptibilities so we left it out. In the current stable branch 10.x we can backport that (should not hurt, just try it out). Reason is: We compile with same compiler and only use stub jars. I fact, it is my fault that I forgot to reenable the check when I changed to apijars for compilation. +1 to backport this. |
Previously these sources were excluded. But these are where we'd like to have the most checks.
Along with the PR on #14916 I'm finally able to fail the build statically on my bad printf string: