RFR: JDK-8042707: Source changes needed to build JDK 9 with Visual Studio 2013 (VS2013)

[Magnus Ihse Bursie]

On 2015-01-08 15:55, Erik Joelsson wrote:
> Hello,
>
> Please review this patch, which adds support for building with 
> different versions of Visual Studio and in particular adds support for 
> VS2013. In order to control which version to use, I've introduced a 
> new configure parameter "--with-toolchain-version". On windows, this 
> parameter will have the valid values 2010, 2012 and 2013. The default 
> is still 2010. Note that 2012 was added for convenience, but has not 
> been tested to actually work. The longer term goal is to switch the 
> official compiler used for JDK 9 to VS2013. This is just the first step.
>
> The main difference that needed to be addressed was that 
> _STATIC_CPPLIB is no longer supported since VS2012, so we will now 
> have to bundle another runtime dll (MSVCP). Also, since the build 
> needs to be compatible with multiple versions of VS, we can no longer 
> hard code the name msvcr100.dll. I changed the names of the dlls into 
> macros that get added to the preprocessor command line. Note that the 
> implementation for msvcp*.dll in java_md.c could perhaps be more 
> elegant. I'm not familiar enough with the APIs, but if someone would 
> like to improve on it, please do.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8042707
> Webrevs:
> http://cr.openjdk.java.net/~erikj/8042707/webrev.root.01/
> http://cr.openjdk.java.net/~erikj/8042707/webrev.jdk.01/

It looks basically good to me. Some remarks on toolchain_windows.m4, though.

In TOOLCHAIN_CHECK_POSSIBLE_VISUAL_STUDIO_ROOT:
# TODO: improve detection for other versions of VS

This seems to be done now, so perhaps this can be removed :)
---
Why is "vs_version" lowercase? It might be better to have local 
variables in lower case (or not, I'm not sure we are consistent on 
this?) but this breaks with the rest of the variables in the function 
and looks strange, like it was intentionally signalling something. (This 
goes for the vs_version in the other functions as well.)
---
# Work around the insanely named ProgramFiles(x86) env variable
PROGRAMFILES_X86="`env | $SED -n 's/^ProgramFiles(x86)=//p'`"

Yay! :-) I spent some time going crazy about that one before I gave up. 
Never thought of that solution.
---
# FIXME will just assume default Visual Studio version
if test "x$with_tools_dir" != x; then
TOOLCHAIN_CHECK_POSSIBLE_VISUAL_STUDIO_ROOT([$with_tools_dir/../..],

This seems broken. Have you tested it? You have to pass a version as the 
first argument to TOOLCHAIN_CHECK_POSSIBLE_VISUAL_STUDIO_ROOT, yes? I 
think we need to examine an explicitely given toolchain to have it's 
version determined. Otherwise, the msvcr/msvcp handling code will not 
function correctly. I suggest that we first check if 
--with-toolchain-version is set and if so, respect it. Otherwise, check 
the path given for the known names (VS_VS_INSTALLDIR_*), and if none 
matches, abort and complain that version must be specified.

Hm, actually, maybe the entire block of testing with_tools_dir should be 
lifted up into TOOLCHAIN_FIND_VISUAL_STUDIO and handled separately..? It 
doesn't really fit into TOOLCHAIN_FIND_VISUAL_STUDIO_BAT_FILE anyhow.

/Magnus