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

Tue Dec 14 07:20:42 PST 2010

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

  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).

[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

  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 during development and does not ship. 

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". ;^)

[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

  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

  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.

[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 JRockit, 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/

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/20101214/d602df12/attachment-0001.html 