Use DESTDIR when creating symlinks in CMake install (PR #22653)
VZ
Prepend $ENV{DESTDIR}, sufficiently quoted to delay its expansion until
the execution of "cmake -E create_symlink" command, to the command
path arguments.
Closes #22610.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/22653
Commit Summary
- b6d52f1 Use DESTDIR when creating symlinks in CMake install
File Changes
(2 files)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/22653.patch
- https://github.com/wxWidgets/wxWidgets/pull/22653.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent requested changes on this pull request.
In build/cmake/utils/CMakeLists.txt:
> @@ -39,8 +39,8 @@ if(wxUSE_XRC)
wx_install(CODE "execute_process( \
⬇️ Suggested change
- wx_install(CODE "execute_process( \ + install(CODE "execute_process( \
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
In build/cmake/utils/CMakeLists.txt:
> @@ -39,8 +39,8 @@ if(wxUSE_XRC)
wx_install(CODE "execute_process( \
It doesn't work with the wx_install macro, it resolves the escaped variables I think.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 1 commit.
- 9b9c24d Use DESTDIR when creating symlinks in CMake install
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz pushed 1 commit.
- 0e7ca02 Use DESTDIR when creating symlinks in CMake install
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
> if(IPHONE)
set(EXE_SUFFIX ".app")
else()
set(EXE_SUFFIX ${CMAKE_EXECUTABLE_SUFFIX})
endif()
- wx_install(CODE "execute_process( \
+ # Don't use wx_install() here to preserve quoting.
Just nitpicking, but it is to preserve escaping.
In build/cmake/utils/CMakeLists.txt:
> COMMAND ${CMAKE_COMMAND} -E create_symlink \
${CMAKE_INSTALL_PREFIX}/bin/${wxrc_output_name}${EXE_SUFFIX} \
- ${CMAKE_INSTALL_PREFIX}/bin/wxrc${EXE_SUFFIX} \
+ \"\$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/bin/wxrc${EXE_SUFFIX}\" \
I think for consistency we should use quotes around both, or none. Not just around one.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
In build/cmake/utils/CMakeLists.txt:
> if(IPHONE)
set(EXE_SUFFIX ".app")
else()
set(EXE_SUFFIX ${CMAKE_EXECUTABLE_SUFFIX})
endif()
- wx_install(CODE "execute_process( \
+ # Don't use wx_install() here to preserve quoting.
I thought quoting was removed when the argument was passed to wx_install(), resulting in escape not working any more? But I don't understand anything in CMake quoting/escaping rules (I honestly tried, but they seem hopelessly complicated).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
> COMMAND ${CMAKE_COMMAND} -E create_symlink \
${CMAKE_INSTALL_PREFIX}/bin/${wxrc_output_name}${EXE_SUFFIX} \
- ${CMAKE_INSTALL_PREFIX}/bin/wxrc${EXE_SUFFIX} \
+ \"\$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/bin/wxrc${EXE_SUFFIX}\" \
So do you mean that not quoting this line would still work? If so, let's not do this... I'd rather not add quotes around the otherwise unchanged line -- or, if we want to do it, do it everywhere and not just here. But see above about my understanding of CMake quoting.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
In build/cmake/utils/CMakeLists.txt:
> COMMAND ${CMAKE_COMMAND} -E create_symlink \
${CMAKE_INSTALL_PREFIX}/bin/${wxrc_output_name}${EXE_SUFFIX} \
- ${CMAKE_INSTALL_PREFIX}/bin/wxrc${EXE_SUFFIX} \
+ \"\$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/bin/wxrc${EXE_SUFFIX}\" \
The quotes are escaped because they are used within other quotes: install(CODE " ... \"quoted text\" ... "). This is maybe less obvious because the command is split over multiple lines.
I added the quotes so it is possible to have spaces in the install path. for example the resulting code might look like: cmake -E create_symlink "/test dir/bin/wxrc-3.2" "/test dir/bin/wxrc". Without quotes this will fail.
I've never tried spaces in the install path (or destdir) so there are probably other places that also should be quoted. So for now you could leave them out and it can be part of a PR that adds them everywhere where needed.
The variable $ENV{DESTDIR} is escaped so it will not be evaluated during CMake's configure step, and it will appear as-is in the cmake_install.cmake script that is executed when installing.
Using ${ARGN} in the wx_install macro seems to cause everything to be re-evaluated, removing the escape. Thats why I added the wxBUILD_INSTALL check and use install directly.
It is possible to keep using wx_install by escaping the escape: \\\$ENV{DESTDIR}
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
In build/cmake/utils/CMakeLists.txt:
> COMMAND ${CMAKE_COMMAND} -E create_symlink \
${CMAKE_INSTALL_PREFIX}/bin/${wxrc_output_name}${EXE_SUFFIX} \
- ${CMAKE_INSTALL_PREFIX}/bin/wxrc${EXE_SUFFIX} \
+ \"\$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/bin/wxrc${EXE_SUFFIX}\" \
OK, so for now I'll just remove the (escaped) quotes, right?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
In build/cmake/utils/CMakeLists.txt:
> COMMAND ${CMAKE_COMMAND} -E create_symlink \
${CMAKE_INSTALL_PREFIX}/bin/${wxrc_output_name}${EXE_SUFFIX} \
- ${CMAKE_INSTALL_PREFIX}/bin/wxrc${EXE_SUFFIX} \
+ \"\$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/bin/wxrc${EXE_SUFFIX}\" \
yes
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
In build/cmake/utils/CMakeLists.txt:
> COMMAND ${CMAKE_COMMAND} -E create_symlink \
${CMAKE_INSTALL_PREFIX}/bin/${wxrc_output_name}${EXE_SUFFIX} \
- ${CMAKE_INSTALL_PREFIX}/bin/wxrc${EXE_SUFFIX} \
+ \"\$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/bin/wxrc${EXE_SUFFIX}\" \
I added the quotes in #22660
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Sorry for the delay, I was taken by other things but will try to get back to wx and merge your PR a.s.a.p.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Closed #22653.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
We still need the \$ENV{DESTDIR} fix from this PR though.
Or shall I add it in a commit in my PR?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Reopened #22653.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Oh sorry I didn't even notice this. Let's apply this one and then yours on top of it then.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
Just remember to remove the escaped quotes in this one then. And maybe update the comment.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I hope I've done it correctly, finally, but please let me know if there is still anything wrong.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.