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
