Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io (original) (raw)

Andrew Hughes ahughes at redhat.com
Fri Aug 24 09:42:43 UTC 2012

----- Original Message -----

On 8/23/12 5:12 PM, Andrew Hughes wrote: > Dan Xu wrote: >> Please review the fix of CR 7193406 at >> http://cr.openjdk.java.net/~dxu/7193406/webrev/. >> And it enables fatal warning flag in the following make file. >> >> make/java/jar/Makefile > > Please don't do this, at least not unconditionally.

Right, we had been enabling fatal warnings in the various makefiles but we've stopped doing so since it has started to cause problems. The intent was to enable fatal warnings by default in individual makefiles, as those areas of the build were cleared of warnings. That way, the introduction of a new warning into a warnings-free area would break the build.

I understand the motivation and why the current setup would be needed in transition.

However, once the whole build is warning free, would it not be preferable to remove these and just set JAVAC_WARNINGS_FATAL=true when doing development builds?

The problem I see is someone new coming to OpenJDK and not being able to simply build it (with no changes) because a new warnings has appeared and is being treated as an error. This is less of a problem with javac as we control its development, and the JDK will use the javac built in the langtools step in most cases. But, generally, -Werror is something you should choose to enable, with the intention of fixing failures, not something that should be forced on everyone building the code.

On a related topic, it would be nice if javadoc could also support -Werror as I constantly see warnings reappearing in the documentation.

The problem is that because of implicit compilation, as code is modified, files can shift around being compiled by different Makefiles. Thus an apparently innocuous change might cause a file with warnings to be built by a different javac run from a makefile that has fatal warnings enabled, causing an unexpected build breakage. Anyway, Dan, please don't enable this flag in this (or any other) Makefile. Sorry, I thought I had mentioned this to you before. > At the very least, these should not be forced on if the user > has explicitly built with them set to false. If I set > JAVACWARNINGSFATAL=false, I don't expect part of the build > to ignore this and use -Werror, possibly then causing build > failures. Yes, it should always be possible to override this by specifying JAVACWARNINGSFATAL=false on the make command line. This overriding should work if the value is specified directly in a Makefile, e.g. JAVACWARNINGSFATAL = true However, there are several cases where the following occurs: SUBDIRSMAKEFLAGS += JAVACWARNINGSFATAL=true and this is not overridable on the command line. That's wrong. If these are causing problems for you, please do submit patches.

Yes, that's what we have in java/tools and is why JAVAC_WARNINGS_FATAL=false doesn't completely remove -Werror at present. I'll post a webrev of the change I have for this.

Although we still occasionally run into problems with implicit compilation causing unexpected build failures, I don't want to remove the fatal warnings settings wholesale. The settings in place now do seem to be keeping at least some parts of the build warning-free. The new build system will change all of this completely, of course. I don't think they have a solution yet for applying different flags to different parts of the build. >> In FilePermission.java file, I make one change to its method >> signature, >> >> public Enumeration elements() ==> public >> Enumeration >> elements() > > It's in a package-private class so I doubt it. > > Even if it wasn't, a new major release is the perfect time to fix > these issues. Yes, this one is fine because it's a private class. For warnings fixes we're trying to avoid making any API changes, since those have to go through some additional process steps. s'marks

-- Andrew :)

Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com)

PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07

More information about the core-libs-dev mailing list