[execute_process] Don't strip embedded semicolons #12926
Conversation
The existing implementation totally screwed up commands if the command's arguments contained semicolons (this is the case, e.g., in the FindPython modules of the cmake distribution).
…tually are use cases for this
|
Added support for execute_process with multiple COMMANDs as there actually are use cases in both vcpkg itself and some ports. |
incorporate changes from microsoft:master
JackBoosY
added
category:vcpkg-bug
Revert "incorporate changes from microsoft:master"
| @@ -13,8 +13,75 @@ if (NOT DEFINED OVERRIDEN_EXECUTE_PROCESS) | |||
| message(FATAL_ERROR "This command cannot be executed in Download Mode.\nHalting portfile execution.\n") | |||
| endmacro() | |||
| else() | |||
There was a problem hiding this comment.
In this else case could we just do nothing (and leave the default execute_process) instead? We are concerned about changes to execute_process in differing versions of cmake and emulating all such behaviors correctly.
There was a problem hiding this comment.
I must admit I couldn't imagine that just leaving away the else case could work. But as I understand your concerns, I just verified it, to make sure we didn't miss such an easy solution. The result is: no. vcpkg then fails already at a very early stage when detecting the compiler hash:
[DEBUG] CreateProcessW("C:\Program Files\CMake\bin\cmake.exe" -DCURRENT_PORT_DIR=C:/work/tools/vcpkg/scripts/detect_compiler -DCURRENT_BUILDTREES_DIR=C:/work/tools/vcpkg/buildtrees/detect_compiler -DCURRENT_PACKAGES_DIR=C:/work/tools/vcpkg/packages/detect_compiler_x64-windows-static -DCMD=BUILD -DTARGET_TRIPLET=x64-windows-static "-DTARGET_TRIPLET_FILE=C:\work\tools\vcpkg\triplets\x64-windows-static.cmake" -DVCPKG_PLATFORM_TOOLSET=v142 -DDOWNLOADS=C:/work/tools/vcpkg/downloads -DVCPKG_CONCURRENCY=9 "-DGIT=C:/Program Files/Git/cmd/git.exe" -DVCPKG_ROOT_DIR=C:/work/tools/vcpkg -DPACKAGES_DIR=C:/work/tools/vcpkg/packages -DBUILDTREES_DIR=C:/work/tools/vcpkg/buildtrees -D_VCPKG_INSTALLED_DIR=C:/work/tools/vcpkg/installed -DDOWNLOADS=C:/work/tools/vcpkg/downloads -DVCPKG_MANIFEST_INSTALL=OFF -P "C:\work\tools\vcpkg\scripts\ports.cmake")
[DEBUG] -- Configuring x64-windows-static
[DEBUG] CMake Error at scripts/cmake/vcpkg_execute_required_process.cmake:50 (_execute_process):
[DEBUG] Unknown CMake command "_execute_process".
[DEBUG] Call Stack (most recent call first):
[DEBUG] scripts/cmake/vcpkg_configure_cmake.cmake:300 (vcpkg_execute_required_process)
[DEBUG] scripts/detect_compiler/portfile.cmake:18 (vcpkg_configure_cmake)
[DEBUG] scripts/ports.cmake:79 (include)
There was a problem hiding this comment.
After hitting escaping issues myself, I think I found the third way:
if (NOT DEFINED OVERRIDEN_EXECUTE_PROCESS)
set(OVERRIDEN_EXECUTE_PROCESS ON)
if (DEFINED VCPKG_DOWNLOAD_MODE)
macro(execute_process)
message(FATAL_ERROR "This command cannot be executed in Download Mode.\nHalting portfile execution.\n")
endmacro()
else()
macro(_execute_process)
execute_process(${ARGV})
endmacro()
endif()
endif()There was a problem hiding this comment.
Which possible problems concerning different cmake versions do you see? I only see the problem that more options could get added to execute_process() in the future. This wouldn't break anything, because nobody could be using such options as they didn't exist before. Only someone starting to use such options in new or updated ports would run into an error, and would need to add the new option to the list in my implementation.
There was a problem hiding this comment.
@ras0219 yeah, your suggestion actually works. I breaks _execute_process() instead of execute_process(), which would be fine with me as _execute_process() is only used a couple of times in: vcpkg_acquire_msys, vcpkg_apply_patches, vcpkg_download_distfile, vcpkg_execute_required_process, vcpkg_find_acquire_program, vcpkg_from_github, and vcpkg_from_gitlab. So, your solution would fix all execute_process() usages within the cmake distribution.
There was a problem hiding this comment.
The issue is just that we're duplicating the "signature" of execute process and maintaining that moving forward is a burden; less code is best code after all :)
In the future, we might move away from this approach for handling DOWNLOAD_MODE entirely if we can more reliably get every port to call vcpkg_execute_required_process().
BillyONeal
changed the title
|
@jgehw Can you make it work like ras0219@b76640e (as discussed above)? |
@BillyONeal yes, as I said [ras0219/vcpkg@b76640e] just leaves |
|
Why did you close this PR? #12977 is no fix for this case. We either need this fix or [ras0219/vcpkg@b76640e]. |
|
@jgehw I didn't actively try to close this, GitHub does that itself if a PR is merged that mentions another issue or PR with a hot phrase like "fixes 1234" in the discussion / history. |
|
(Specifically that PR had "fixed #12926" in the description which causes GitHub to link them together and close one when the other is merged) |
|
@BillyONeal Ahh, I see, I'll pay attention to avoid such phrasing next time. |
|
arm_uwp and x64_uwp CI failing on json5-parser port downloading https://bitbucket.com/wlandry/json5_parser/get/1.0.0.tar.gz. When viewing in a browser, I get "Repository wlandry/json5_parser not found". So, not related to this PR. |
|
Yeah, upstream is gone. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 12926 in repo microsoft/vcpkg |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
upstream is gone again... please re-run once upstream is fixed |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…te_required_process_repeat
The existing implementation totally screwed up commands if the command's arguments contained semicolons (this is the case, e.g., in the FindPython modules of the cmake distribution).
Fixes #12922.