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
