Review request (M): Updates to Visual Studio project creation and development launcher

[Kelly O'Hair]

Makefiles are kind of hard to review, so this is the best I can do...

Just curious, have you tried building this with CYGWIN?

http://cr.openjdk.java.net/~sla/7006354/webrev.00/make/windows/build_vm_def.sh.sdiff.html

   Not a big deal, and not related to your change, but as we move  
forward, I'd like to see MKS_HOME
   changed to something more generic, like TOOLS_BINDIR or something  
like that.
   It's not necessarily MKS and it is not a product home directory  
either. But I know this is historic stuff.

http://cr.openjdk.java.net/~sla/7006354/webrev.00/make/windows/makefiles/launcher.make.sdiff.html

   The NUL used here is a Windows/MKS thing, and is considered a  
filename in CYGWIN.
   So this needs to be put into a variable or something, using NUL  
when MKS is used and /dev/null when
   CYGWIN is used. Otherwise builds with CYGWIN will leave NUL files  
scattered all over the place.
   (Inside the bat files, I think NUL is the right thing to use, but  
I'm not a bat file expert).

http://cr.openjdk.java.net/~sla/7006354/webrev.00/make/windows/makefiles/product.make.sdiff.html

   The name is hotspot.exe? Is this the temp launcher that does not  
ship in the jdk image?
   Have we always had an exe name with "hotspot" in it? Not sure I  
know that it is wrong, just seems strange.

http://cr.openjdk.java.net/~sla/7006354/webrev.00/src/os/posix/launcher/java_md.c.sdiff.html

   The secure coding class (which I very recently and painfully  
completed) seemed to imply that a
   printf like this is insecure. Not sure I agree, but I suspect we  
should change to use a format string
   before some tool complains about it. I'll also bet that this is  
done all over the place already by everybody. :^(
   Of course the getenv() is also considered a problem too. But again,  
not your issue here, but it might
   be a good idea in the future for the hotspot team to encapsulate  
use of getenv so there is one place to say
   '@IgnoreThisGetenv call". ;^)

http://cr.openjdk.java.net/~sla/7006354/webrev.00/src/os/posix/launcher/launcher.script.sdiff.html

   Use of pwd with CYGWIN could get you a path like /cygdrive/c/  
instead of C:/ (from MKS).
   In this case I think it's ok, but pwd can be a problem in Makefiles  
when we allow the use of
   either MKS or CYGWIN for builds.

http://cr.openjdk.java.net/~sla/7006354/webrev.00/src/os/windows/launcher/java_md.c.sdiff.html

   More printf's without format strings.
   I'm confused by this code. So the env variable "JAVA_HOME" is not  
used anymore and it reads it
   from a file now? And then you set the JAVA_HOME environment  
variable with the value?
   Why is this being done?
   Maybe I don't care because the gamma launcher isn't shipped as part  
of the jdk (right?).
   But this seems really unusual to me.

http://cr.openjdk.java.net/~sla/7006354/webrev.00/src/os/windows/vm/os_windows.cpp.sdiff.html

   Has ALT_JAVA_HOME always been around? Is it just for this gamma  
launcher?
   I've never seen any documentation on a ALT_JAVA_HOME variable  
spelling, just curious if it
   creeps into the jdk product at all.


On the java code, I didn't review it in detail but appreciate the  
change from wildcard imports to explicit
imports, always seems so much more accurate that way. ;^)

-kto


On Dec 13, 2010, at 5:06 AM, Staffan Larsen wrote:

> Webrev: http://cr.openjdk.java.net/~sla/7006354/webrev.00/
>
> This update makes it easier to work with Visual Studio projects.
>
> After this update you can create a VS project by:
> 1)      Run “vsvars32.cmd”
> 2)      Run “make\windows\create.bat <path to JDK to use for running  
> against>”
> From inside Visual Studio you can open the generated vsbuild 
> \jvm.vcproj. Edit the properties to set the debugging command to “$ 
> (TargetDir)\hotspot.exe”. Compile and run/debug from inside VS.
>
> Limitations:
> ·         Changes have only been tested on VS2008 and VS2010.
> ·         The script do not generate VS2010 project, but the  
> conversion wizard in VS2010 can read the generated VS2008 project  
> file.
> ·         Only 32-bit targets work.
>
> This change includes the proposed patch in CR7002870.
>
> Thanks,
> /Staffan
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-dev/attachments/20101213/35f959bd/attachment-0001.html