The technical part of the patch looks good (as previously, I've only checked the build system).
The overshadowing issue is still legal, though. I can't see how this patch can be accepted without legal clearance. /Magnus 27 jan 2014 kl. 22:48 skrev Elliott Baron <eba...@redhat.com>: > Hi Magnus, > > On 01/15/2014 06:08 AM, Magnus Ihse Bursie wrote: >> On 2014-01-15 03:38, Wang Weijun wrote: >>> Hi Elliott >>> >>> Great to see this again. I’ll come back to this later. There are some >>> urgent issues I have to deal with at this moment. I’ll also need to get >>> those legal advices regarding pkg.m4 etc. >> >> I see some issues and questions about this patch. >> >> First of all, and I believe this was discussed the last time this patch was >> around, is that there might be legal question marks about including >> build-aux/krb5.m4. This code is written by someone who has not, to my >> knowledge, signed the OCA. Unfortunately, legal issues tend to shadow all >> technical issues, so you might want to start by getting this one solved. >> >> On the technical level, given that the krb5.m4 legality is cleared, I see: >> * The patch does not seem to be updated to the removed old build system. >> make/sun/security is no more. >> * Whitespace and indentation seems to be incorrect in several places in >> help.m4, libraries.m4 and SecurityLibraries.gmk. Please check surrounding >> code, or look at the guidelines here: >> http://mail.openjdk.java.net/pipermail/build-dev/2013-October/010477.html >> >> I have only looked at the build part of the patch. > > How does this revised webrev look [1]? I have fixed indentation and removed > remnants of the old build system. I have also fixed the test's Makefile, > which no longer worked due to the removal of the old build system. I would > like to point out that like the inheritedChannel test which this test is > derived from, this test expects pre-built libraries to be checked into > version control. I understand this is required due to test systems not > necessarily having the required build environment. > > Thanks, > Elliott > > [1] http://icedtea.classpath.org/~ebaron/webrevs/krb5-default-ccache/02/
