KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
2.9k stars 816 forks source link

Fix installation of glslangValidator link/copy #3515

Closed AnyOldName3 closed 3 days ago

AnyOldName3 commented 4 months ago

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.

https://github.com/KhronosGroup/glslang/pull/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, 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.

CLAassistant commented 4 months ago

CLA assistant check
All committers have signed the CLA.

AnyOldName3 commented 4 months ago

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 cmake_install.cmake doesn't just contain the line I referenced above, and is instead

    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 $ENV{DESTDIR} is coming from CMake rather than glslang's CMake scripts, but then so's the path in the earlier file(INSTALL command that doesn't have $ENV{DESTDIR}.

Despite the fact that https://cmake.org/cmake/help/latest/envvar/DESTDIR.html says not to use $ENV{DESTDIR} on Windows, other parts of CMake's documentation suggest doing so without fencing it off behind platform checks (e.g. https://cmake.org/cmake/help/latest/variable/CPACK_CUSTOM_INSTALL_VARIABLES.html) and so does its source code (e.g https://gitlab.kitware.com/cmake/cmake/-/blob/master/Source/cmInstallGenerator.cxx#L245-255) so I think it's actually better to keep it.

The other changes of this PR, i.e. also backslash escaping the $ for CMAKE_INSTALL_PREFIX so it's evaluated at install time, and putting the path in quotes so spaces don't kill it, are still necessary, though.

arcady-lunarg commented 4 months ago

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.

AnyOldName3 commented 4 months ago

After the last commit, this PR can't have made anything worse. Putting quotes around a path is at worst harmless, and CMAKE_INSTALL_PREFIX is intended to be evaluated at install time. I backed out of the removal of $ENV{DESTDIR} as I found evidence that doing so might be sketchy.

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 glslangValidator placeholder wasn't getting created if a different install prefix was provided at install time, so that's two problems it definitely fixes.

arcady-lunarg commented 3 days ago

This is superseded by #3642.