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

[magnum] Removing quotes around tools list where vcpkg_copy_tools is involked #14383

Merged
merged 5 commits into from
Nov 5, 2020

Conversation

PhoebeHui
Copy link
Contributor

@PhoebeHui PhoebeHui commented Nov 4, 2020

Fixes #14329

emoving quotes around tools list where vcpkg_copy_tools is involked, see details in #14329 (comment)

@PhoebeHui PhoebeHui added category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) info:internal This PR or Issue was filed by the vcpkg team. labels Nov 4, 2020
@jgehw
Copy link
Contributor

jgehw commented Nov 4, 2020

Sorry for not replying earlier (I didn't get notified that I was mentioned in the issue).

This is not the way we should fix it, because removing the backslashes is essentially a work-around to remove work-arounds in the portfiles, which aren't needed any more. Moreover, this could breaks things.

To explain the background, as it's really anything else than obvious:

  • cmake uses semicolons as list separators
  • => semicolons inside strings that a elements of a list must be escaped
  • cmake has two bugs: correctly escaped semicolons get broken, if
    • cmake_parse_arguments is used not the PARSE_ARGV-way
    • lists are modified other than by appending, i.e. inserting
  • That's why I fixed all cmake_parse_arguments in [vcpkg] Fix more cases of semicolon mishandling in "scripts" - follow-up to PR #12926 #13968
  • various portfiles used work-arounds in the past to get around this, in particular, by passing a list as string, for example:
set(_TOOL_EXEC_NAMES "")
list(APPEND _TOOL_EXEC_NAMES "some_tool")
list(APPEND _TOOL_EXEC_NAMES "another_tool")
# passing ${_TOOL_EXEC_NAMES} would be passing the list
# passing "${_TOOL_EXEC_NAMES}" converts the list into a string
# so this way to invoke it is a work-around for the buggy cmake API
vcpkg_copy_tools(TOOL_NAMES "${_TOOL_EXEC_NAMES}" AUTO_CLEAN)
# now that the non-buggy version of cmake_parse_arguments is used, it must be invoked without string-conversion work-around
vcpkg_copy_tools(TOOL_NAMES ${_TOOL_EXEC_NAMES} AUTO_CLEAN)

Long story short: please fix by removing quotes in magnum portfile.

@PhoebeHui PhoebeHui changed the title [vcpkg_copy_tools] Remove backslash in args [magnum] Removing quotes around tools list where vcpkg_copy_tools is involked Nov 5, 2020
@PhoebeHui PhoebeHui added category:port-bug The issue is with a library, which is something the port should already support and removed category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) labels Nov 5, 2020
@PhoebeHui
Copy link
Contributor Author

@jgehw, thanks for your review! I have updated the magnum port.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Nov 5, 2020
@BillyONeal BillyONeal merged commit 9032255 into microsoft:master Nov 5, 2020
@BillyONeal
Copy link
Member

Thanks :D

@PhoebeHui PhoebeHui deleted the dev/Phoebe/vcpkg_copy_tools branch December 23, 2020 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[magnum] build failure - actual issue is vcpkg_copy_tools.cmake
4 participants