Adding makefiles to make/tools is not needed for the new build. Either
remove those changes or make a complete port to the old build. I'm not
pushing for porting this to the old build at this point since missing
this will not cause the old build to stop working, it will just diverge
the resulting bits a bit more.
/Erik
On 2013-10-09 19:19, Sean Mullan wrote:
Updated webrev:
http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.01/
Let me know if there are any more comments, otherwise I will plan to
push tomorrow.
Thanks,
Sean
On 10/09/2013 09:20 AM, Sean Mullan wrote:
On 10/09/2013 05:14 AM, Erik Joelsson wrote:
Hello Sean,
On 2013-10-09 06:33, David Holmes wrote:
Hi Sean,
Not a full review.
On 9/10/2013 5:52 AM, Sean Mullan wrote:
Please review the fix for the following bug:
https://bugs.openjdk.java.net/browse/JDK-8007292
This bug requires build changes and a new build tool to add
additional
restricted packages to the java.security file which are not part of
OpenJDK. These packages are only added when doing a build
including the
open and closed sources.
The restricted packages and new test are in the closed sources and
will
be reviewed separately.
webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.00/
Based on your description and the ifndef OPENJDK it sounds to me that
this doesn't belong in the OpenJDK makefiles.
I agree in principle, but the pattern on how to handle this isn't well
established yet and something we need to improve on. In this case we
would need to make changes on the open side anyway to provide hooks for
overrides of the behavior. I'm willing to accept this for now.
Thanks. Also, keep in mind that this hook allows each vendor,etc to add
additional proprietary or internal packages to the restricted packages
properties for their own build. So I think it is useful in general in
that respect.
That aside I would think the CP+RM could be changed to a MV.
Agreed.
Right. Will do.
I would prefer the TOOL_ADDTO... line to be moved to
jdk/makefiles/Tools.gmk with the other similar definitions, even if it
is only used here atm.
Ok.
In the tool this code doesn't show correct use of try-with-resources:
51 try (BufferedReader br = new BufferedReader(new
FileReader(args[0]));
52 BufferedWriter bw = new BufferedWriter(new
FileWriter(args[1]))) {
The FileReader and FileWriter should also be covered by TWR:
try (FileReader fr = new FileReader(args[0]);
BufferedReader br = new BufferedReader(fr);
FileWriter fw = new FileWriter(args[1]);
BufferedWriter bw = new BufferedWriter(fw)) {
I'm not familiar with the try-with-resources, but calling close on a
BufferedReader/writer will close the underlying reader/writer so
nothing
will be left open, will it not?
That's what I thought as well. David?
Finally do we still use make/tools/Makefile in the new build?
No, we don't. If you want to add old build support for this, you would
also need to add usage of the tool to the correct old makefile.
I don't think it's necessary to add this to the old build at this point.
I'll post another webrev later in the day with these updates.
Thanks,
Sean
