Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rmuir
Copy link
Member

@rmuir rmuir commented Jul 8, 2025

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:

> Task :lucene:core:compileMain24Java
/home/rmuir/workspace/lucene/lucene/core/src/java24/org/apache/lucene/internal/vectorization/PanamaVectorizationProvider.java:52: error: [FormatString] missing argument for format specifier '%s'
        String.format(
                     ^
    (see https://errorprone.info/bugpattern/FormatString)
1 error

> Task :lucene:core:compileMain24Java FAILED

Previously these sources were excluded. But these are where we'd like to
have the most checks.
Copy link

github-actions bot commented Jul 8, 2025

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.

Copy link
Contributor

@dweiss dweiss left a 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
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 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).

@uschindler
Copy link
Contributor

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?

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.

@uschindler
Copy link
Contributor

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.

@rmuir
Copy link
Member Author

rmuir commented Jul 8, 2025

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

That's where the "front lines" are for new vectors code or modifications in pull requests.

@uschindler
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants