You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "Dawid Weiss (Jira)" <ji...@apache.org> on 2020/06/19 09:23:00 UTC
[jira] [Comment Edited] (LUCENE-9411) Fail complation on warnings
[ https://issues.apache.org/jira/browse/LUCENE-9411?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17140358#comment-17140358 ]
Dawid Weiss edited comment on LUCENE-9411 at 6/19/20, 9:22 AM:
---------------------------------------------------------------
The patch includes this:
{code}
+configure(allprojects) {
+ plugins.withType(JavaPlugin) {
+ dependencies {
+ compileOnly 'com.google.code.findbugs:annotations:3.0.1'
+ compileOnly 'com.google.errorprone:error_prone_annotations:2.1.3'
+ }
+ }
+}
{code}
This is wrong, it shouldn't be applied everywhere. And where these are needed, they should be applied selectively (more below).
bq. A secondary question is why, after a build, I have references in .gradle/caches to:
Don't look in there, who cares what's in gradle's cache (and where it comes from or when it was downloaded). If you wish to inspect project dependencies in each configuration, run:
{code}
gradlew dependencies > output.log
{code}
and then look at where a given dependency is mentioned (which configuration, which project). I only
see this mention of error_prone_annotations: as part of guava:
{code}
+--- com.google.guava:guava -> 25.1-jre
| | +--- com.google.code.findbugs:jsr305:3.0.2
| | +--- org.checkerframework:checker-qual:2.0.0
| | +--- com.google.errorprone:error_prone_annotations:2.1.3
| | +--- com.google.j2objc:j2objc-annotations:1.1
{code}
Alternatively, you can grep versions.lock and query for the hash:
{code}
com.google.errorprone:error_prone_annotations:2.1.3 (1 constraints: 180aebb4)
{code}
so:
{code}
gradlew why --hash=180aebb4
{code}
which prints:
{code}
> Task :why
com.google.errorprone:error_prone_annotations:2.1.3
com.google.guava:guava -> 2.1.3
{code}
All this is explained in "help/dependencies.txt".
The mysterious warnings you see - like this one:
{code}
C:\Users\dweiss\.gradle\caches\modules-2\files-2.1\org.apache.zookeeper\zookeeper\3.5.7\12bdf55ba8be7fc891996319d37f35eaad7e63ea\zookeeper-3.5.7.jar(/org/apache/zookeeper/ClientCnxn.class): warning: Cannot find annotation method 'value()' in type 'SuppressFBWarnings': class file for edu.umd.cs.findbugs.annotations.SuppressFBWarnings not found
{code}
results from incomplete classpath dependencies exported by zookeeper's JARs. So there are classes in from zookeeper-3.5.7.jar referencing annotations that
are not found on classpath, giving javac a headache. Note the warning mentions this class:
edu.umd.cs.findbugs.annotations.SuppressFBWarnings
which isn't part of JSR305 - it's part of a different package (annotations). So we have to provide it but not export it. This patch does it for Solrj:
{code}
diff --git a/solr/solrj/build.gradle b/solr/solrj/build.gradle
index e8c8a072d05..84c39ce1fe8 100644
--- a/solr/solrj/build.gradle
+++ b/solr/solrj/build.gradle
@@ -54,6 +54,10 @@ dependencies {
exclude group: "log4j", module: "log4j"
exclude group: "org.slf4j", module: "slf4j-log4j12"
})
+ // Zookeeper classes reference this although it's not part of the exported classpath
+ // which causes odd warnings during compilation. Shut it up with an explicit-version
+ // compile-only dependency (!).
+ compileOnly 'com.google.code.findbugs:annotations:3.0.1'
api('org.codehaus.woodstox:woodstox-core-asl', {
exclude group: "javax.xml.stream", module: "stax-api"
{code}
If the same problem propagates to other projects (I didn't check) then I'd create a separate file gradle/solr/zookeeper-hack.gradle and apply this compileOnly dependency to
a set of selected projects without polluting the classpath globally like you had in your patch.
The second problem is this:
{code}
> Task :solr:solrj:compileJava
warning: [rawtypes] found raw type: Map
missing type arguments for generic class Map<K,V>
where K,V are type-variables:
K extends Object declared in interface Map
V extends Object declared in interface Map
{code}
And this is much more tricky. I enabled vebose javac logging with:
{code}
tasks.withType(JavaCompile) {
options.verbose true
}
{code}
and then saw this:
{code}
[wrote O:\repos\lucene-gradle-master\solr\solrj\build\classes\java\main\org\apache\solr\common\util\Template.class]
[checking org.apache.solr.common.util.JsonSchemaValidator]
warning: [rawtypes] found raw type: Map
missing type arguments for generic class Map<K,V>
where K,V are type-variables:
K extends Object declared in interface Map
V extends Object declared in interface Map
{code}
which has these declarations in the static block:
{code}
VALIDATORS.put("items", pair -> new ItemsValidator(pair.first(), (Map) pair.second()));
{code}
Seems like the cast is to blame here (why javac doesn't report the offending file is probably a bug in javac). Changing
the code shape to this works, but I'd honestly try to correct it rather than just suppress warnings -- it smells fishy.
{code}
+ static final Map<String, Function<Pair<Map,Object>, Validator>> VALIDATORS = foo();
+
+ @@SuppressWarnings({"rawtypes"})
+ private static Map<String, Function<Pair<Map,Object>, Validator>> foo() {
+ Map<String, Function<Pair<Map,Object>, Validator>> validators = new HashMap<>();
+ validators.put("items", pair -> new ItemsValidator(pair.first(), (Map) pair.second()));
+ validators.put("enum", pair -> new EnumValidator(pair.first(), (List) pair.second()));
+ validators.put("properties", pair -> new PropertiesValidator(pair.first(), (Map) pair.second()));
+ validators.put("type", pair -> new TypeValidator(pair.first(), pair.second()));
+ validators.put("required", pair -> new RequiredValidator(pair.first(), (List)pair.second()));
+ validators.put("oneOf", pair -> new OneOfValidator(pair.first(), (List)pair.second()));
+ return validators;
}
public List<String> validateJson(Object data) {
{code}
Finally, as for adding -Werror I think this should be part of global javac: if it should fail, let it fail early. There is a way to add it *only* if precommit task is running but I don't see why we should provide such a loophole.
was (Author: dweiss):
The patch includes this:The patch includes this:
{code}+configure(allprojects) \{+ plugins.withType(JavaPlugin) {+ dependencies {+ compileOnly 'com.google.code.findbugs:annotations:3.0.1'+ compileOnly 'com.google.errorprone:error_prone_annotations:2.1.3'+ }+ }+}\{code}
This is wrong, it shouldn't be applied everywhere. And where these are needed, they should be applied selectively (more below).
bq. A secondary question is why, after a build, I have references in .gradle/caches to:
Don't look in there, who cares what's in gradle's cache (and where it comes from or when it was downloaded). If you wish to inspect project dependencies in each configuration, run:
{code}gradlew dependencies > output.log\{code}
and then look at where a given dependency is mentioned (which configuration, which project). I only see this mention of error_prone_annotations: as part of guava:
{code}+--- com.google.guava:guava -> 25.1-jre| | +--- com.google.code.findbugs:jsr305:3.0.2| | +--- org.checkerframework:checker-qual:2.0.0| | +--- com.google.errorprone:error_prone_annotations:2.1.3| | +--- com.google.j2objc:j2objc-annotations:1.1\{code}
Alternatively, you can grep versions.lock and query for the hash:
{code}com.google.errorprone:error_prone_annotations:2.1.3 (1 constraints: 180aebb4) \{code}
so:\{code}gradlew why --hash=180aebb4\{code}which prints:\{code}> Task :whycom.google.errorprone:error_prone_annotations:2.1.3 com.google.guava:guava -> 2.1.3\{code}All this is explained in "help/dependencies.txt".
The mysterious warnings you see - like this one:\{code}C:\Users\dweiss\.gradle\caches\modules-2\files-2.1\org.apache.zookeeper\zookeeper\3.5.7\12bdf55ba8be7fc891996319d37f35eaad7e63ea\zookeeper-3.5.7.jar(/org/apache/zookeeper/ClientCnxn.class): warning: Cannot find annotation method 'value()' in type 'SuppressFBWarnings': class file for edu.umd.cs.findbugs.annotations.SuppressFBWarnings not found\{code}
results from incomplete classpath dependencies exported by zookeeper's JARs. So there are classes in from zookeeper-3.5.7.jar referencing annotations thatare not found on classpath, giving javac a headache. Note the warning mentions this class:
edu.umd.cs.findbugs.annotations.SuppressFBWarnings
which isn't part of JSR305 - it's part of a different package (annotations). So we have to provide it but not export it. This patch does it for Solrj:\{code}diff --git a/solr/solrj/build.gradle b/solr/solrj/build.gradleindex e8c8a072d05..84c39ce1fe8 100644--- a/solr/solrj/build.gradle+++ b/solr/solrj/build.gradle@@ -54,6 +54,10 @@ dependencies \{ exclude group: "log4j", module: "log4j" exclude group: "org.slf4j", module: "slf4j-log4j12" })+ // Zookeeper classes reference this although it's not part of the exported classpath+ // which causes odd warnings during compilation. Shut it up with an explicit-version+ // compile-only dependency (!).+ compileOnly 'com.google.code.findbugs:annotations:3.0.1'
api('org.codehaus.woodstox:woodstox-core-asl', \{ exclude group: "javax.xml.stream", module: "stax-api"{code}
If the same problem propagates to other projects (I didn't check) then I'd create a separate file gradle/solr/zookeeper-hack.gradle and apply this compileOnly dependency to a set of selected projects without polluting the classpath globally like you had in your patch.
The second problem is this:\{code}> Task :solr:solrj:compileJavawarning: [rawtypes] found raw type: Map missing type arguments for generic class Map<K,V> where K,V are type-variables: K extends Object declared in interface Map V extends Object declared in interface Map\{code}
And this is much more tricky. I enabled vebose javac logging with:\{code}tasks.withType(JavaCompile) \{ options.verbose true}{code}
and then saw this:\{code}[wrote O:\repos\lucene-gradle-master\solr\solrj\build\classes\java\main\org\apache\solr\common\util\Template.class][checking org.apache.solr.common.util.JsonSchemaValidator]warning: [rawtypes] found raw type: Map missing type arguments for generic class Map<K,V> where K,V are type-variables: K extends Object declared in interface Map V extends Object declared in interface Map\{code}
which has these declarations in the static block:\{code} VALIDATORS.put("items", pair -> new ItemsValidator(pair.first(), (Map) pair.second()));\{code}
Seems like the cast is to blame here (why javac doesn't report the offending file is probably a bug in javac). Changingthe code shape to this works, but I'd honestly try to correct it rather than just suppress warnings -- it smells fishy.
{code}diff --git a/solr/solrj/src/java/org/apache/solr/common/util/JsonSchemaValidator.java b/solr/solrj/src/java/org/apache/solr/common/util/JsonSchemaValidator.javaindex 178503e990c..b423bc6f89a 100644--- a/solr/solrj/src/java/org/apache/solr/common/util/JsonSchemaValidator.java+++ b/solr/solrj/src/java/org/apache/solr/common/util/JsonSchemaValidator.java@@ -62,15 +62,18 @@ public class JsonSchemaValidator \{ }
@SuppressWarnings(\{"rawtypes"})- static final Map<String, Function<Pair<Map,Object>, Validator>> VALIDATORS = new HashMap<>();-- static \{- VALIDATORS.put("items", pair -> new ItemsValidator(pair.first(), (Map) pair.second()));- VALIDATORS.put("enum", pair -> new EnumValidator(pair.first(), (List) pair.second()));- VALIDATORS.put("properties", pair -> new PropertiesValidator(pair.first(), (Map) pair.second()));- VALIDATORS.put("type", pair -> new TypeValidator(pair.first(), pair.second()));- VALIDATORS.put("required", pair -> new RequiredValidator(pair.first(), (List)pair.second()));- VALIDATORS.put("oneOf", pair -> new OneOfValidator(pair.first(), (List)pair.second()));+ static final Map<String, Function<Pair<Map,Object>, Validator>> VALIDATORS = foo();++ @SuppressWarnings({"rawtypes"})+ private static Map<String, Function<Pair<Map,Object>, Validator>> foo() \{+ Map<String, Function<Pair<Map,Object>, Validator>> validators = new HashMap<>();+ validators.put("items", pair -> new ItemsValidator(pair.first(), (Map) pair.second()));+ validators.put("enum", pair -> new EnumValidator(pair.first(), (List) pair.second()));+ validators.put("properties", pair -> new PropertiesValidator(pair.first(), (Map) pair.second()));+ validators.put("type", pair -> new TypeValidator(pair.first(), pair.second()));+ validators.put("required", pair -> new RequiredValidator(pair.first(), (List)pair.second()));+ validators.put("oneOf", pair -> new OneOfValidator(pair.first(), (List)pair.second()));+ return validators; }
public List<String> validateJson(Object data) \{{code}
Finally, as for adding -Werror I think this should be part of global javac: if it should fail, let it fail early. There is a way to add it *only* if precommit task is running but I don't see why we should provide such a loophole.
> Fail complation on warnings
> ---------------------------
>
> Key: LUCENE-9411
> URL: https://issues.apache.org/jira/browse/LUCENE-9411
> Project: Lucene - Core
> Issue Type: Improvement
> Components: general/build
> Reporter: Erick Erickson
> Assignee: Erick Erickson
> Priority: Major
> Labels: build
> Attachments: LUCENE-9411.patch, annotations-warnings.patch
>
>
> Moving this over here from SOLR-11973 since it's part of the build system and affects Lucene as well as Solr. You might want to see the discussion there.
> We have a clean compile for both Solr and Lucene, no rawtypes, unchecked, try, etc. warnings. There are some peculiar warnings (things like SuppressFBWarnings, i.e. FindBugs) that I'm not sure about at all, but let's assume those are not a problem. Now I'd like to start failing the compilation if people write new code that generates warnings.
> From what I can tell, just adding the flag is easy in both the Gradle and Ant builds. I still have to prove out that adding -Werrors does what I expect, i.e. succeeds now and fails when I introduce warnings.
> But let's assume that works. Are there objections to this idea generally? I hope to have some data by next Monday.
> FWIW, the Lucene code base had far fewer issues than Solr, but common-build.xml is in Lucene.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org