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/
