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
