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

[Magnus Ihse Bursie]

On 2015-01-13 09:41, Erik Joelsson wrote:
> Hello,
>
> New webrev for root repo:
> http://cr.openjdk.java.net/~erikj/8042707/webrev.root.02/

Fixes look good to me. Thanks!

/Magnus

>
> On 2015-01-09 15:45, Magnus Ihse Bursie wrote:
>>
>> 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 :)
> Done.
>> ---
>> 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 
> credit.
>> ---
>> # 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.
>>
> 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.
>
> /Erik