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
