Thanks Erik! Looks okay to me.
David
On 15/03/2016 8:36 PM, Erik Joelsson wrote:
Hello,
New webrev where "closed" is replaced with "custom".
http://cr.openjdk.java.net/~erikj/8151653/webrev.hotspot.02/
Also, to clarify the refactoring of trace.xml here on the list too. I
needed to create 2 separate entry points, one open and one closed
trace.xml. The way xinclude works, I can't just include the open
trace.xml from the closed one, I have to refactor them to instead
include the same content. That's why I moved the contents of the current
open trace.xml to two new files, one for each XML-tag at that level in
the XML.
/Erik
On 2016-03-15 01:46, David Holmes wrote:
On 14/03/2016 11:22 AM, David Holmes wrote:
Hi Erik,
On 12/03/2016 2:31 AM, Erik Joelsson wrote:
Hello,
When building hotspot with closed sources present and configuring with
--enable-openjdk-only, various closed parts are included in the build
anyway, at least on Windows. This needs to be fixed in preparation for
the new hotspot build for build output comparisons to be meaningful
between the old and new.
The major culprit here, which applies to all platforms, is the trace
source generation. The base trace.xml uses XInclude to explicitly and
unconditionally include closed xml files if present. I'm fixing this by
introducing a closed version of trace.xml which includes the open and
closed parts, while the open trace.xml only includes the open parts.
You've also done a significant amount of refactoring of that file and
split it into three parts. It's hard to spot the actual functional
differences in all that.
Sorry that was a bit terse. It would have been clearer if you had
explained about the refactoring. I can see why the refactoring was
needed.
Thanks,
David
-----
Given we have distinct directories from which we locate files it is a
pity to introduce something like this:
XML_DEPS += $(TraceAltSrcDir)/traceeventsclosed.xml
where traceevents.xml is now traceeventsclosed.xml. This "alt src"
mechanism was supposed to abstract away the details of any particular
alternative configuration so that we didn't hardcode "closed" all over
the place. Other community members are supposed to be able to utilize
these mechanisms for their own customized build environments.
The rest of the changes are just for Windows, making sure the OPENJDK
variable is propagated into the nmake build and making all conditionals
on including closed source also check that variable.
make/windows/build.make
The combination !openjdk && !exists "closed" should be an error.
As a meta-comment I think we've lost the plot somewhat with these "alt"
locations and how we manage them. The Oracle "closed" variants should
only be used when not trying to build OpenJDK (even if the files exist
in a forest), but other custom implementations may work in conjunction
with an OpenJDK build. To that end the "do nothing if building OpenJDK"
should be located within the "alt" files themselves, not at the point of
inclusion/use in the open files.
David
-----
Bug: https://bugs.openjdk.java.net/browse/JDK-8151653
Webrev:
http://cr.openjdk.java.net/~erikj/8151653/webrev.hotspot.01/index.html
/Erik
