RFR: JDK-8042707: Source changes needed to build JDK 9 with Visual Studio 2013 (VS2013)
Magnus Ihse Bursie
magnus.ihse.bursie at oracle.com
Tue Jan 13 16:04:56 UTC 2015
On 2015-01-13 09:41, Erik Joelsson wrote:
> New webrev for root repo:
Fixes look good to me. Thanks!
> On 2015-01-09 15:45, Magnus Ihse Bursie wrote:
>> It looks basically good to me. Some remarks on toolchain_windows.m4,
>> 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.)
> Changed to upper case. Also introduced a common TOOLCHAIN_VERSION
> variable that is used in any non windows specific m4 file.
>> # 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.
> I think I found that on StackOverflow or similar site so can't claim
>> # FIXME will just assume default Visual Studio version
>> if test "x$with_tools_dir" != x; then
>> 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.
> I tinkered some with this, but ended up putting it back in
> TOOLCHAIN_FIND_VISUAL_STUDIO_BAT_FILE because it was simpler. It works
> now, but setting --with-tools-dir assumes you are pointing to the
> default version of Visual Studio. If you aren't, you must also set
> --with-toolchain-version for it to behave correctly. I suppose we
> could implement detection logic that would figure out which version it
> was, but I would rather not spend time on it when it's not the
> preferred way to configure this.
More information about the build-dev