please review 7122061: enable -Werror in various jdk build steps

[Stuart Marks]

On 12/15/11 9:23 PM, David Holmes wrote:
> On 16/12/2011 12:59 PM, Stuart Marks wrote:
>> http://cr.openjdk.java.net/~smarks/reviews/7122061/webrev.0/
>
> Looks fine but ...
>
>> This defines the JAVAC_MAX_WARNINGS and JAVAC_WARNINGS_FATAL variables
>> in a variety of Makefiles within the jdk repository. This essentially
>> adds -Xlint:all -Werror to the javac command lines, so that henceforth
>> if any lint warnings are introduced, the build will fail.
>>
>> I'm applying these flags only to build steps that are already warnings
>> free. (Last week's warnings cleanup effort cleared warnings from four
>> build steps, in addition to knocking off a couple thousand warnings
>> across the build.) I've applied this change and have built successfully
>> on all platforms.
>>
>> With this change, 57 of the 93 javac build steps in the jdk repo are now
>> lint warning free, and are protected from the introduction of new errors
>> through the use of -Werror.
>
> ... as we're past the half-way point should we not set these by default and
> change the non-warning free steps to override them? That way as an area becomes
> warning free we would include the removal of the override as part of the
> changeset. That way when we get to being warning-free there should only be one
> occurrence of:
>
> JAVAC_MAX_WARNINGS = true
> JAVAC_WARNINGS_FATAL = true
>
> in the whole build system.

Yes, I think this is a good thing to consider. I had thought about this a bit 
and felt that it would be more appropriate to make a change after all (or at 
least almost all) the warnings had been cleaned up, not just the majority, 
though when we should make such a change deserves discussion. Jon Gibbons also 
pointed out that we should coordinate with the new build system effort on 
build-infra.

It's also a small puzzle to figure out the best way to do this without doing 
too much damage to the current makefile system. I think we now know how much 
damage even an innocuous change to a makefile can do. :-)

That said, here's a possible approach.

The current scheme has the leaf Makefile define these JAVAC_ variables *before* 
inclusion of make/common/Defs.gmk. This file ends up including 
make/common/shared/Defs-java.gmk, which tests these variables and adds options 
like -Xlint:all and -Werror if they equal true.

Unfortunately in this scheme it's not possible to supply a default value for a 
variable that's overridden by the leaf Makefile, since the leaf Makefile is 
read first.

There's an alternate approach that has a similar effect, which is to use the 
make construct ?= that means "set if not already set." Thus, in Defs-java.gmk 
we can do something like

     JAVAC_WARNINGS_FATAL ?= true
     ifeq ($(JAVAC_WARNINGS_FATAL), true)
         JAVACFLAGS += -Werror
     endif

and any leaf Makefile that doesn't want warnings to be fatal does

     JAVAC_WARNINGS_FATAL = false

prior to including Defs.gmk. I think this will work, but I haven't tested it. I 
don't think this construct has been used anywhere in the make system yet, though.

The alternative is to invert the sense of the test and rename the variable, so 
the leaf Makefiles would do this:

     JAVAC_WARNINGS_NONFATAL = true

and Defs-java.gmk would do this:

     ifneq ($(JAVAC_WARNINGS_NONFATAL), true)
         JAVACFLAGS += -Werror
     endif

This uses more conventional Make constructs, at the expense of having the 
double negative "if not non-fatal, add the -Werror option." I'm not sure which 
approach people would find preferable.

In any case there are other issues with -Werror raised elsewhere in this 
thread, which I'll need to address before proceeding with this.

s'marks