Fix compiler warnings #13410

rjernst · 2015-09-09T05:18:47Z

We have a handful of compiler warnings, mostly because of passing an
array to varargs methods. This change fixes these warnings and adds
-Werror so we don't get anymore of these warnings.

Note this does not enable deprecation or unchecked type warnings, so
these remain "hidden". We should work towards removing those as well,
but this is a first step.

We have a handful of compiler warnings, mostly because of passing an array to varargs methods. This change fixes these warnings and adds -Werror so we don't get anymore of these warnings. Note this does *not* enable deprecation or unchecked type warnings, so these remain "hidden". We should work towards removing those as well, but this is a first step.

rmuir · 2015-09-09T05:23:11Z

Looks great!

Note this does not enable deprecation or unchecked type warnings, so
these remain "hidden". We should work towards removing those as well,
but this is a first step.

Good, it at least prevents warnings of other types from stacking up. How bad is the unchecked situation? We should really open a followup for that one.

Deprecation is more complicated, especially if we want to deprecate our own apis. That one is less important I think.

rjernst · 2015-09-09T05:24:56Z

How bad is the unchecked situation?

Hundreds of warnings...well, at least 100

rmuir · 2015-09-09T05:38:29Z

Can we also turn on -Xlint ? I don't care if we then have to add e.g. -Xlint:-serial to disable some of the stupid ones, or ones that need fixes, but it would be nice to know where we stand.

rjernst · 2015-09-09T07:22:28Z

I turned on -Xlint and disabled the frequent ones I found, and fixed a handful of easy ones. There are some tricky ones we should really look into...

However, while core main/test now passes compile, I get a failure compiling tests for distribution/tar. But there is no warning, or any error at all... @dadoonet Could you try my branch and see if you understand why? I've run with -X but I don't see anything that explains why the failure occurred.

s1monw · 2015-09-09T07:24:44Z

However, while core main/test now passes compile, I get a failure compiling tests for distribution/tar. But there is no warning, or any error at all... @dadoonet Could you try my branch and see if you understand why? I've run with -X but I don't see anything that explains why the failure occurred.

this is likely your JVM / Compiler we had problems her with java version "1.8.0_31" so if you run that I'd upgrade

s1monw · 2015-09-09T07:48:32Z

this is really weird if you run mvn clean test-compile -X you get the classpath it uses etc. to pass to javac. If I do this manually I get the following error:

warning: [path] bad path element "/Users/simon/projects/elasticsearch/distribution/tar/target/classes": no such file or directory
error: warnings found and -Werror specified
1 error
1 warning

creating target/classes fixes the issue

s1monw · 2015-09-09T07:54:17Z

hmm without -Xlint:all it passes

dadoonet · 2015-09-09T08:26:27Z

Funny. The same is happening in plugins but not for all plugins...
Analysis are working fine. Cloud-azure is failing...

warning: [path] bad path element "/Users/dpilato/.m2/repository/com/sun/xml/bind/jaxb-impl/2.2.3-1/jaxb-api.jar": no such file or directory
warning: [path] bad path element "/Users/dpilato/.m2/repository/com/sun/xml/bind/jaxb-impl/2.2.3-1/activation.jar": no such file or directory
warning: [path] bad path element "/Users/dpilato/.m2/repository/com/sun/xml/bind/jaxb-impl/2.2.3-1/jsr173_1.0_api.jar": no such file or directory
warning: [path] bad path element "/Users/dpilato/.m2/repository/com/sun/xml/bind/jaxb-impl/2.2.3-1/jaxb1-impl.jar": no such file or directory
error: warnings found and -Werror specified
1 error
4 warnings

Looking at this ATM.

dadoonet · 2015-09-09T08:40:58Z

@rjernst Adding <arg>-Xlint:-path</arg> fixes the issue.

And BTW this is cool stuff because now we can see some cloud-azure code issues! :)

[INFO] -------------------------------------------------------------
[WARNING] COMPILATION WARNING : 
[INFO] -------------------------------------------------------------
[WARNING] /Users/dpilato/Documents/Elasticsearch/dev/es-master/elasticsearch/plugins/cloud-azure/src/test/java/org/elasticsearch/cloud/azure/AzureComputeServiceSimpleMock.java:[50,23] [static] static variable should be qualified by type name, AzureModule, instead of by an expression
[WARNING] /Users/dpilato/Documents/Elasticsearch/dev/es-master/elasticsearch/plugins/cloud-azure/src/test/java/org/elasticsearch/cloud/azure/AbstractAzureRepositoryServiceTestCase.java:[50,23] [static] static variable should be qualified by type name, AzureModule, instead of by an expression
[WARNING] /Users/dpilato/Documents/Elasticsearch/dev/es-master/elasticsearch/plugins/cloud-azure/src/test/java/org/elasticsearch/cloud/azure/AzureComputeServiceTwoNodesMock.java:[54,23] [static] static variable should be qualified by type name, AzureModule, instead of by an expression
[INFO] 3 warnings

dadoonet · 2015-09-09T08:50:07Z

Just wondering if we should add a maven property like

<lint.level>-Werror</lint.level>

And then define instead of <arg>-Werror</arg>:

<arg>${xlint.level}</arg>

So we could potentially run: mvn clean install -Dlint.level= which will not stop the build but WARN errors

dadoonet · 2015-09-09T08:58:33Z

@rjernst If you don't want to bypass path checks for each module, then add in parent pom:

<properties>
    <xlint.path></xlint.path>
</properties>

And:

                <plugin>
                    <groupId>org.apache.maven.plugins</groupId>
                    <artifactId>maven-compiler-plugin</artifactId>
                    <version>3.3</version>
                    <configuration>
                        <fork>true</fork>
                        <maxmem>512m</maxmem>
                        <!-- REMOVE WHEN UPGRADE:
                             see https://jira.codehaus.org/browse/MCOMPILER-209 it's a bug where
                             incremental compilation doesn't work unless it's set to false causeing
                             recompilation of the entire codebase each time without any changes. Should
                             be fixed in version > 3.1
                         -->
                        <useIncrementalCompilation>false</useIncrementalCompilation>
                        <showWarnings>true</showWarnings>
                        <compilerArgs>
                            <arg>-XDignore.symbol.file</arg>
                            <arg>-Xlint:all</arg>
                            <arg>${xlint.path}</arg>
                            <arg>-Xlint:-cast</arg>
                            <arg>-Xlint:-deprecation</arg>
                            <arg>-Xlint:-fallthrough</arg>
                            <arg>-Xlint:-overrides</arg>
                            <arg>-Xlint:-rawtypes</arg>
                            <arg>-Xlint:-serial</arg>
                            <arg>-Xlint:-try</arg>
                            <arg>-Xlint:-unchecked</arg>
                            <arg>-Werror</arg>
                        </compilerArgs>
                    </configuration>
                </plugin>

In distribution/pom.xml:

<properties>
    <xlint.path>-Xlint:-path</xlint.path>
</properties>

Same in cloud-azure (and may be some others)...

rjernst · 2015-09-09T08:59:34Z

Just wondering if we should add a maven property

No, I don't think we should do that, because the next step is "let's set the default back to warn". This PR fixes issues, we don't want to go backwards from there. And there are some we really need to investigate (see some of the TODOs I added in this).

rjernst · 2015-09-09T09:03:04Z

@dadoonet thanks for the suggestion on parameterizing -Xling:-path. I actually want to just be able to add additional compiler args within a child pom, eg core. This way the warning removals can be isolated to the modules that need them, instead of allowing leniency in all modules.

dadoonet · 2015-09-09T09:14:37Z

@rjernst then add in parent pom:

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-compiler-plugin</artifactId>
    <version>3.3</version>
    <configuration>
        <fork>true</fork>
        <maxmem>512m</maxmem>
        <!-- REMOVE WHEN UPGRADE:
             see https://jira.codehaus.org/browse/MCOMPILER-209 it's a bug where
             incremental compilation doesn't work unless it's set to false causeing
             recompilation of the entire codebase each time without any changes. Should
             be fixed in version > 3.1
         -->
        <useIncrementalCompilation>false</useIncrementalCompilation>
        <showWarnings>true</showWarnings>
        <compilerArgs>
            <arg>-XDignore.symbol.file</arg>
            <arg>-Xlint:all</arg>
            <arg>-Xlint:-path</arg>
            <arg>-Werror</arg>
        </compilerArgs>
    </configuration>
</plugin>

In core/pom.xml:

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-compiler-plugin</artifactId>
    <configuration>
        <compilerArgs>
            <arg>-XDignore.symbol.file</arg>
            <arg>-Xlint:all</arg>
            <arg>-Xlint:-path</arg>
            <arg>-Werror</arg>
            <arg>-Xlint:-cast</arg>
            <arg>-Xlint:-deprecation</arg>
            <arg>-Xlint:-fallthrough</arg>
            <arg>-Xlint:-overrides</arg>
            <arg>-Xlint:-rawtypes</arg>
            <arg>-Xlint:-serial</arg>
            <arg>-Xlint:-try</arg>
            <arg>-Xlint:-unchecked</arg>
        </compilerArgs>
    </configuration>
</plugin>

But you don't really inherit from parent pom. You have to declare again all compilerArgs.

dadoonet · 2015-09-09T09:53:36Z

Ok so here is a better solution.

In /pom.xml:

    <properties>
        <xlint.options/>
    </properties>

...

                <plugin>
                    <groupId>org.apache.maven.plugins</groupId>
                    <artifactId>maven-compiler-plugin</artifactId>
                    <version>3.3</version>
                    <configuration>
                        <fork>true</fork>
                        <maxmem>512m</maxmem>
                        <!-- REMOVE WHEN UPGRADE:
                             see https://jira.codehaus.org/browse/MCOMPILER-209 it's a bug where
                             incremental compilation doesn't work unless it's set to false causeing
                             recompilation of the entire codebase each time without any changes. Should
                             be fixed in version > 3.1
                         -->
                        <useIncrementalCompilation>false</useIncrementalCompilation>
                        <showWarnings>true</showWarnings>
                        <compilerArgs>
                            <arg>-XDignore.symbol.file</arg>
                            <arg>-Xlint:all</arg>
                            <arg>-Xlint:-path</arg>
                            <arg>${xlint.options}</arg>
                            <arg>-Werror</arg>
                        </compilerArgs>
                    </configuration>
                </plugin>

In core/pom.xml:

    <properties>
        <xlint.options>-Xlint:-cast,-deprecation,-fallthrough,-overrides,-rawtypes,-serial,-try,-unchecked</xlint.options>
    </properties>

That said, there are also some deprecated classes in plugins, so I would also add in parent pom -deprecation option.

rjernst · 2015-09-09T19:48:49Z

@dadoonet @s1monw Thanks for maven help. I've added suppressions and ignores specific to each module where needed. I think this is ready.

rjernst · 2015-09-09T21:04:48Z

Pushing as this really hasn't changed other than limiting which xlint options are used.

Fix compiler warnings

rjernst added :Delivery/Build Build or test infrastructure v2.1.0 v5.0.0-alpha1 labels Sep 9, 2015

Add Xlint options and fix some warnings

b04160d

Add xlint ignores for warning classes, where appropriate.

9e8a90a

rjernst added a commit that referenced this pull request Sep 9, 2015

Merge pull request #13410 from rjernst/you_were_warned

9d71f07

Fix compiler warnings

rjernst merged commit 9d71f07 into elastic:master Sep 9, 2015

rjernst changed the title ~~Build: Fix compiler warnings~~ Fix compiler warnings Sep 9, 2015

rjernst deleted the you_were_warned branch September 9, 2015 21:05

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compiler warnings #13410

Fix compiler warnings #13410

rjernst commented Sep 9, 2015

rmuir commented Sep 9, 2015

rjernst commented Sep 9, 2015

rmuir commented Sep 9, 2015

rjernst commented Sep 9, 2015

s1monw commented Sep 9, 2015

s1monw commented Sep 9, 2015

s1monw commented Sep 9, 2015

dadoonet commented Sep 9, 2015

dadoonet commented Sep 9, 2015

dadoonet commented Sep 9, 2015

dadoonet commented Sep 9, 2015

rjernst commented Sep 9, 2015

rjernst commented Sep 9, 2015

dadoonet commented Sep 9, 2015

dadoonet commented Sep 9, 2015

rjernst commented Sep 9, 2015

rjernst commented Sep 9, 2015

Fix compiler warnings #13410

Fix compiler warnings #13410

Conversation

rjernst commented Sep 9, 2015

rmuir commented Sep 9, 2015

rjernst commented Sep 9, 2015

rmuir commented Sep 9, 2015

rjernst commented Sep 9, 2015

s1monw commented Sep 9, 2015

s1monw commented Sep 9, 2015

s1monw commented Sep 9, 2015

dadoonet commented Sep 9, 2015

dadoonet commented Sep 9, 2015

dadoonet commented Sep 9, 2015

dadoonet commented Sep 9, 2015

rjernst commented Sep 9, 2015

rjernst commented Sep 9, 2015

dadoonet commented Sep 9, 2015

dadoonet commented Sep 9, 2015

rjernst commented Sep 9, 2015

rjernst commented Sep 9, 2015