Daniel, Erik
thanks for the reviews and sorry taking so long to respond.
About the indentation: I understand the convention used in the new build
system and I think it makes a lot of sense, but the file
hotspot/make/Makefile clearly does not use this convention :)
I would prefer to use the same kind of indentation as the rest of the
file, even though as Dan noted, the file is not consistent with itself.
It seems like most of the file is using 2 spaces for ifeq (something)
and then just <tab> or <tab+2 spaces> for recipes depending on logical
level.
Dan and Erik, are you fine with keeping the indentation as it is or do
you prefer that I update the change to use the convention of the new
build system?
Thanks,
Erik
On 02/17/2014 09:09 AM, Erik Joelsson wrote:
Hello,
The change looks good to me too.
Regarding indentation, for the rest of the JDK, we indent rules with
make ifeqs like this:
target: sources
<8 spaces>ifeq (something)
<tab+2 spaces>recipe line
<8 spaces>endif
Our reasoning is that it should still look like a recipe when glancing
over it quickly (so 8 characters base indentation). After that we apply
our standard 2 chars per logical level. Since make regards tab
characters as special, we have to use space for non recipe lines.
The rest of our guidelines can be found here:
http://openjdk.java.net/groups/build/doc/code-conventions.html
/Erik
On 2014-02-14 17:45, Daniel D. Daugherty wrote:
> http://cr.openjdk.java.net/~ehelin/8033580/webrev.03/
make/Makefile
No comments.
Makefile style question:
Since all this ifeq(...) logic is around Makefile rules,
should the rules themselves be indented by one tab or by
one tab and some number of spaces.
I scrolled around the Makefile and I don't really see
consistent indenting of the rule lines themselves...
If you change the indenting, I don't see a reason for
another round of review.
Dan
On 2/14/14 6:21 AM, Erik Helin wrote:
Dan,
On 2014-02-14 00:29, Daniel D. Daugherty wrote:
> http://cr.openjdk.java.net/~ehelin/8033580/webrev.01/
make/Makefile
+ ifeq ($(OSNAME), bsd)
+ $(RM) -rf $(EXPORT_SERVER_DIR)/libjvm.dylib.dSYM
The above needs to be:
ifeq ($(OS_VENDOR), Darwin)
$(RM) -rf $(EXPORT_SERVER_DIR)/libjvm.dylib.dSYM
I don't think that BSD uses .dylib stuff; that's MacOS X specific.
You are right, thanks for the correction. Please see new webrev at:
http://cr.openjdk.java.net/~ehelin/8033580/webrev.03/
Again, same testing as for prior patches were run.
Thanks,
Erik
Dan
On 2/13/14 1:59 PM, Erik Helin wrote:
Hi Dan,
thanks for reviewing! See my replies inline.
On 02/13/2014 12:40 AM, Daniel D. Daugherty wrote:
> http://cr.openjdk.java.net/~ehelin/8033580/webrev.00/
make/Makefile
277 ifeq ($(ZIP_DEBUGINFO_FILES),1)
278 ifeq ($(OSNAME), windows)
279 $(RM) -f $(EXPORT_SERVER_DIR)/jvm.map
$(EXPORT_SERVER_DIR)/jvm.pdb
280 else
281 $(RM) -f $(EXPORT_SERVER_DIR)/libjvm.debuginfo
282 endif
On MacOS X, the debuginfo will be in libjvm.dylib.dSYM so
you'll need a MacOS X specific rule. You don't need an
update for the JVM_VARIANT_CLIENT version because MacOS X
doesn't support the Client VM, but if it did...
Thanks for catching this! I've uploaded a new webrev at:
http://cr.openjdk.java.net/~ehelin/8033580/webrev.01/
The same testing was done as for the first patch.
On 02/13/2014 12:40 AM, Daniel D. Daugherty wrote:
So the above change handles libjvm, but what about the
other libraries exported by HotSpot? libjsig, libjvm_db,
and libjvm_dtrace come to mind...
As we discussed, I would like to implement this for libjvm first and
then take care of the other libraries in a separate patch.
Thanks,
Erik
Dan
On 2/12/14 8:03 AM, Erik Helin wrote:
Hi all,
this patch changes how old debug information copied from
IMPORT_JDK is
treated.
When running the copy_*_jdk target, HotSpot's makefiles copies the
entire IMPORT_JDK folder, including additional files (such as
unzipped
debug information). The export_*_jdk targets will then, via the
generic_export target, copy the build artifacts via implicit
rules to
the correct destination in hotspot/build/JDK_IMAGE_DIR.
The bug arises when IMPORT_JDK contains unzipped debug info
(libjvm.debuginfo) and the make target produces zipped debug info
(libjvm.diz), or vice versa. hotspot/build/JDK_IMAGE_DIR will then
contain both libjvm.debuginfo and libjvm.diz, but only one of them
will match libjvm.so.
Finally, the JPRT make targets jprt_build_* just zips
hotspot/build/$(JDK_IMAGE_DIR). The zipped JPRT bundle will end up
having different zipped and unzipped debug info, since the
IMPORT_JDK
in JPRT contains libjvm.debuginfo and we build libjvm.diz by
default.
This patch removes the debug info that we did *not* build. If we
build
libjvm.diz, then libjvm.debuginfo will be removed (if it exists).
Correspondingly, if we build libjvm.debuginfo, then libjvm.diz
will be
removed (if it exists).
Patch:
http://cr.openjdk.java.net/~ehelin/8033580/webrev.00/
Bug:
https://bugs.openjdk.java.net/browse/JDK-8033580
Testing:
- Building in JPRT
- Building Linux 32-bit locally on a Linux 64-bit machine
- Building Linux 64-bit locally on a Linux 64-bit machine
For all of the above, verify that only the correct debug info is
present in the output.
Thanks,
Erik
