Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AnyOldName3
Copy link
Contributor

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.

#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.

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.
@CLAassistant
Copy link

CLAassistant commented Feb 14, 2024

CLA assistant check
All committers have signed the CLA.

@AnyOldName3
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants