JDK-8211307: Add prefix to build tools paths #283
Conversation
|
@tiainen , do you have any comments for this PR? |
There was a problem hiding this comment.
I did full test builds (including media and webkit) on all three platforms, both with and without setting BUILD_TOOLS_DOWNLOAD_SCRIPT to a script that defines toolchainDir. I ran into one problem on Windows when using the default toolchain with VS2017 installed in the default location:
> Task :web:compileNativeWin
python: can't open file 'Tools/Scripts/update-vswhere.py': [Errno 2] No such file or directory
+ cmake -DPORT="Java" -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_BUILD_TYPE=Release -G "Visual Studio 15 2017 Win64" -DCMAKE_GENERATOR_TOOLSET="host=x64" -DSHOW_BINDINGS_GENERATION_PROGRESS=1 -DENABLE_EXPERIMENTAL_FEATURES=ON -DENABLE_TOOLS=1 -DCMAKE_C_COMPILER=C:/Program Files (x86)/Microsoft Visual Studio/2017/Professional/VC/Tools/MSVC/14.10.25017/bin/HostX64/x64/cl.exe -DJAVAFX_RELEASE_VERSION=12 "rt\modules\javafx.web\src\main\native"
sh: -c: line 0: syntax error near unexpected token `('
sh: -c: line 0: `cmake -DPORT="Java" -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_BUILD_TYPE=Release -G "Visual Studio 15 2017 Win64" -DCMAKE_GENERATOR_TOOLSET="host=x64" -DSHOW_BINDINGS_GENERATION_PROGRESS=1 -DENABLE_EXPERIMENTAL_FEATURES=ON -DENABLE_TOOLS=1 -DCMAKE_C_COMPILER=C:/Program Files (x86)/Microsoft Visual Studio/2017/Professional/VC/Tools/MSVC/14.10.25017/bin/HostX64/x64/cl.exe -DJAVAFX_RELEASE_VERSION=12 "rt\modules\javafx.web\src\main\native"'
> Task :web:compileNativeWin FAILED
FAILURE: Build failed with an exception.
* Where:
Build file 'rt\build.gradle' line: 3200
* What went wrong:
Execution failed for task ':web:compileNativeWin'.
> Process 'command 'perl'' finished with non-zero exit value 1
It looks like a problem of not escaping a file path that includes spaces or special chars -- a ( in this case.
buildSrc/win.gradle
Outdated
| @@ -133,7 +137,9 @@ ext.WINDOWS_NATIVE_COMPILE_ENVIRONMENT = [ | |||
| ]; | |||
| def msvcVer = System.getenv("MSVC_VER") ?: "14.10.25017" | |||
| def msvcBinDir = "" | |||
| if (winVsVer == 150) { | |||
| if (hasProperty('toolchainDir')) { | |||
| msvcBinDir = "$WINDOWS_VS_VSINSTALLDIR/VC/bin/${IS_64 ? 'x64' : 'x86'}" | |||
There was a problem hiding this comment.
The msvcRedstDir will likely need similar treatment (i.e., a different directory when using toolchainDir).
There was a problem hiding this comment.
Exactly. I will fix it.
|
I have no further comments. I've had a look through the changes and they look okay to me. Only valuable feedback would be to do what Kevin has already done. |
|
Thanks @kevinrushforth & @tiainen for reviewing this PR. @kevinrushforth , yes the failure is due to space in file path. |
|
@kevinrushforth Addressed the review comments in the new commit. Please take a look. Thanks. |
| String msvcRedstDir = (IS_64 | ||
| ? "$WINDOWS_VS_VSINSTALLDIR/VC/Redist/MSVC/$msvcRedistVer/x64" | ||
| : "$WINDOWS_VS_VSINSTALLDIR/VC/Redist/MSVC/$msvcRedistVer/x86") | ||
| def msvcRedstDir |
There was a problem hiding this comment.
I think msvcRedstDir can be still string ? I see that msvcRedstDir is not used outside of win.gradle and no need to be a def?
There was a problem hiding this comment.
I see that msvcRedstDir is not used outside of win.gradle and no need to be a def?
def defines a local variable, so I see no problem with this.
There was a problem hiding this comment.
Yes, as @kevinrushforth mentioned, it is nothing to do with the scope, it is almost equvalent to C++ auto / Java's var.
| String winSdkDllDir = (IS_64 | ||
| ? "$WINDOWS_VS_WINSDKDLLINSTALLDIR/x64" | ||
| : "$WINDOWS_VS_WINSDKDLLINSTALLDIR/x86") | ||
| def winSdkDllDir = "$WINDOWS_VS_WINSDKDLLINSTALLDIR/$CPU_BITS" |
There was a problem hiding this comment.
I think winSdkDllDir can be a string..
|
|
||
| def WINDOWS_DLL_VER = WINDOWS_VS_VER | ||
| ext.MSVCR = null | ||
| ext.MSVCP = null | ||
|
|
||
| def windowsCRTVer = System.getenv("WINDOWS_CRT_VER") ?: "150" | ||
| def windowsCRTVer = System.getenv("WINDOWS_CRT_VER") ?: WINDOWS_CRT_VER | ||
| if (WINDOWS_VS_VER == "150") { |
There was a problem hiding this comment.
if you are replacing 150 with "WINDOWS_CRT_VER", you can do this at all places like if (WINDOWS_VS_VER == WINDOWS_CRT_VER) {
There was a problem hiding this comment.
There are multiple versions need to be considered, 1. VS version, 2. CRT version & 3. DLL version. What we are comparing here is VS version, it should be kept as it is, otherwise it may cause issues when using older VS variants.
| : "$WINDOWS_VS_VSINSTALLDIR/VC/Tools/MSVC/$msvcVer/bin/HostX86/x86") | ||
| if (hasProperty('toolchainDir')) { | ||
| msvcBinDir = "$WINDOWS_VS_VSINSTALLDIR/VC/bin/$CPU_BITS" | ||
| } else if (winVsVer == 150) { |
There was a problem hiding this comment.
You can compare with WINDOWS_CRT_VER ?
There was a problem hiding this comment.
You can compare with WINDOWS_CRT_VER ?
This would be an intrusive and possibly risky change. Let's leave it as is for now.
| @@ -62,6 +62,13 @@ if (IS_DEBUG_NATIVE) { | |||
| linkFlags += "-g" | |||
| } | |||
|
|
|||
| def toolchainDir | |||
There was a problem hiding this comment.
can toolchainDir be String instead of def ?
| @@ -126,9 +128,15 @@ def linkFlags = [ | |||
| "-framework", "Security", | |||
| "-dynamiclib", "-lobjc"].flatten(); | |||
|
|
|||
| def toolchainDir | |||
There was a problem hiding this comment.
can toolchainDir be String instead of def ?
Currently JavaFX uses build tools{MSVC, GCC, Clang, CMake,..etc} from standard system paths. Adding prefix to each of those tools would help to have a custom build tools without disturbing standard tools.
This fix provides a new gradle property named BUILD_TOOLS_DOWNLOAD_SCRIPT, which could be used to point to a supplement gradle file which can have the logic to setup the custom toolchain.
In additional to provisioning toolchain prefix, this patch also has changes to javafx.web module to workaround "command line is too long" issue on windows when custom toolchain path length is long.