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

[Stefan Karlsson]

Looks good.

/StefanK

On 12/14/2010 04:20 PM, Staffan Larsen wrote:
>
> Updated webrev: http://cr.openjdk.java.net/~sla/7006354/webrev.02/
>
> Changes in this version:
>
> - The generated files were missing from the VS project
>
> - build_vm_def.sh did not work in VS2010!
>
> - Added verification that cygwin or mks is in path when running create.bat
>
> - Use javac from then build-jdk if not in PATH or JAVA_HOME
>
> - Moved visual studio projects into build\vs directory (and reverted 
> .hgignore)
>
> - Verified in VS2003
>
> Thanks,
>
> /Staffan
>
> *From:*Staffan Larsen
> *Sent:* den 14 december 2010 1:03
> *To:* Kelly Ohair
> *Cc:* hotspot-dev at openjdk.java.net
> *Subject:* RE: Review request (M): Updates to Visual Studio project 
> creation and development launcher
>
> Thanks for the detailed review - my comments inline prefixed by [SL].
>
> *From:*Kelly O'Hair
> *Sent:* den 13 december 2010 6:15
> *To:* Staffan Larsen
> *Cc:* hotspot-dev at openjdk.java.net
> *Subject:* Re: Review request (M): Updates to Visual Studio project 
> creation and development launcher
>
> Makefiles are kind of hard to review, so this is the best I can do...
>
> Just curious, have you tried building this with CYGWIN?
>
> [SL] Yes. However, I don't have MKS so I haven't tried that. Anyone 
> want to give it a shot?
>
> http://cr.openjdk.java.net/~sla/7006354/webrev.00/make/windows/build_vm_def.sh.sdiff.html 
> <http://cr.openjdk.java.net/%7Esla/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 
> <http://cr.openjdk.java.net/%7Esla/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).
>
> [SL] I don't get any NUL files created with cygwin. I'd love some help 
> on a better construct for this though. The only reason for the NUL is 
> to keep mkdir quiet if the directory already exists. I'd also like to 
> have the mkdir in only one rule, but can't figure out how to do it.
>
> http://cr.openjdk.java.net/~sla/7006354/webrev.00/make/windows/makefiles/product.make.sdiff.html 
> <http://cr.openjdk.java.net/%7Esla/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.
>
> [SL] The 'hotspot.exe' (on posix it is a shell script called 
> 'hotspot') development launcher was something I added in CR6981484. It 
> is only for simplifying launching the JVM du! ring development and 
> does not ship.
>
> http://cr.openjdk.java.net/~sla/7006354/webrev.00/src/os/posix/launcher/java_md.c.sdiff.html 
> <http://cr.openjdk.java.net/%7Esla/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, 
> n! ot 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". ;^)
>
> [SL] Not sure I understand what the problem with the printf is. In any 
> case, this code is only used during development and never shipped.
>
> http://cr.openjdk.java.net/~sla/7006354/webrev.00/src/os/posix/launcher/launcher.script.sdiff.html 
> <http://cr.openjdk.java.net/%7Esla/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.
>
> [SL] This launcher script is only used on posix platforms, so cygwin 
> shouldn't be a problem. On windows we! use the executable instead 
> (hotspot.exe).
>
> http://cr.openjdk.java.net/~sla/7006354/webrev.00/src/os/windows/launcher/java_md.c.sdiff.html 
> <http://cr.openjdk.java.net/%7Esla/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 par! 
> t 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 
> <http://cr.openjdk.java.net/%7Esla/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.
>
> [SL] I see now how confusing this code turned out. What I am trying to 
> achieve is 1) a way for the development launcher (hotspot.exe) to 
> automatically find the JDK to run against and 2) a way to override 
> this. My first implementation used JAVA_HOME for this, but that wasn't 
> a particular good choice since it can point to just about anything 
> (including JRockit in my case - which didn't work well :-) ).
>
> So my update here uses a generated file in the build directory 
> (jdkpath.txt) which is setup by the create.bat script. This default 
> can be overridden by setting ALT_JAVA_HOME before running the 
> launcher. ALT_JAVA_HOME was a name I invented (or inherited from 
> JRock! it, actually).
>
> Now, the old 'gamma' launcher on posix platforms (which is still 
> available and used by the 'hotspot' shell script) used JAVA_HOME to 
> find the JDK and there was code in the JVM to see that the gamma 
> launcher was used and to check JAVA_HOME. I didn't want to change this 
> code, and consequently I still use JAVA_HOME to communicate the JDK 
> location to the JVM (on windows as well).
>
> I don't really like this, but I didn't want to change the behavior of 
> the gamma launcher.
>
> So the flow is currently:
>
> Windows: (jdkpath.txt or ALT_JAVA_HOME) -> hotspot.exe -> JAVA_HOME -> 
> jvm.dll
>
> Posix: (jdkpath.sh or ALT_JAVA_HOME) -> hotspot (script) -> JAVA_HOME 
> -> gamma (executable) -> JAVA_HOME -> libjvm.so
>
> 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. ;^)
>
> [SL] This was done automatically by eclipse, but it is nice.
>
> /Staffan
>
> -kto
>
> On Dec 13, 2010, at 5:06 AM, Staffan Larsen wrote:
>
> Webrev:http://cr.openjdk.java.net/~sla/7006354/webrev.00/ 
> <http://cr.openjdk.java.net/%7Esla/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 conver! sion 
> 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/20101215/a8fda9b0/attachment-0001.html