New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix installation of glslangValidator link/copy #3515
base: main
Are you sure you want to change the base?
Fix installation of glslangValidator link/copy #3515
Conversation
The generated cmake_install.cmake contains lines like file(INSTALL DESTINATION "${CMAKE_INSTALL_PREFIX}/bin" TYPE EXECUTABLE FILES "C:/vsg/glslang/build/StandAlone/Release/glslang.exe") Note that it uses the install-time value of CMAKE_INSTALL_PREFIX, not the configure-time one. The line which copies glslang/creates a symlink should use the same path - if it uses a different one, then glslang won't be there to copy, or will be the wrong instance. KhronosGroup#3283 attempted to fix the same problem, but only did so sometimes. My understanding is that on some people's setups, the combined install-time value of DESTDIR and the configure-time install prefix was equivalent to the install-time install prefix, but it's better to just use the install-time install prefix. https://cmake.org/cmake/help/latest/envvar/DESTDIR.html says you can't use DESTDIR on Windows in the first place. Having the backslash before the dollar sign means CMAKE_INSTALL_PREFIX will not be evaluated at configure-time. Putting the whole expression in quotes solves a second problem where the install would fail if CMAKE_INSTALL_PREFIX contained spaces, as it does by default on Windows. Before the other changes in this MR, it was spaces in the configure-time variable that would cause problems, and after it was ones in the install-time variable.
Upon further inspection, the problem might be more complicated than I first thought. When I generate a Ninja Multi-Config MSYS2 build system, the generated file(INSTALL DESTINATION "${CMAKE_INSTALL_PREFIX}/bin" TYPE EXECUTABLE FILES "C:/vsg/glslang/build/GCC 13.2.0 x86_64-w64-mingw32 (ucrt64)/StandAlone/Release/glslang.exe")
if(EXISTS "$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/bin/glslang.exe" AND
NOT IS_SYMLINK "$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/bin/glslang.exe")
if(CMAKE_INSTALL_DO_STRIP)
execute_process(COMMAND "C:/tools/msys64/ucrt64/bin/strip.exe" "$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/bin/glslang.exe")
endif()
endif() The Despite the fact that https://cmake.org/cmake/help/latest/envvar/DESTDIR.html says not to use The other changes of this PR, i.e. also backslash escaping the |
It's a bit of a minefield getting everything to work right in all situations including MinGW and proper integration with what package builders use, so any change should be tested across the broadest possible set of configurations. |
After the last commit, this PR can't have made anything worse. Putting quotes around a path is at worst harmless, and On the other hand, I've confirmed that without the quotes, the install script will fail if the install prefix contains spaces on any platform, and that the |
The generated
cmake_install.cmake
contains lines likeNote that it uses the install-time value of
CMAKE_INSTALL_PREFIX
, not the configure-time one.The line which copies glslang/creates a symlink should use the same path - if it uses a different one, then glslang won't be there to copy, or will be the wrong instance.
#3283 attempted to fix the same problem, but only did so sometimes. My understanding is that on some people's setups, the combined install-time value of
DESTDIR
and the configure-time install prefix was equivalent to the install-time install prefix, but it's better to just use the install-time install prefix. https://cmake.org/cmake/help/latest/envvar/DESTDIR.html says you can't useDESTDIR
on Windows in the first place.Having the backslash before the dollar sign means
CMAKE_INSTALL_PREFIX
will not be evaluated at configure-time, so it's not exactly the same as undoing #3283.Putting the whole expression in quotes solves a second problem where the install would fail if
CMAKE_INSTALL_PREFIX
contained spaces, as it does by default on Windows. Before the other changes in this MR, it was spaces in the configure-time variable that would cause problems, and after it was ones in the install-time variable.