Fix cmake handling of paths with spaces #164
Conversation
| include(FreeRTOS_Kernel_import.cmake) | ||
|
|
||
| project(debugprobe) | ||
|
|
||
| pico_sdk_init() | ||
|
|
||
| if (${PICO_SDK_VERSION_MAJOR} LESS 2) | ||
| if ("${PICO_SDK_VERSION_MAJOR}" LESS 2) |
There was a problem hiding this comment.
Presumably there'll never be a space in PICO_SDK_VERSION_MAJOR ?
There was a problem hiding this comment.
Most likely. It was just quicker and easier to replace everything with quotations, and shouldn't (as far as I know) make an impact either way. Quotations should help with some special characters too, though this probably isn't a problem here.
| if (PICO_SDK_FETCH_FROM_GIT_PATH) | ||
| get_filename_component(FETCHCONTENT_BASE_DIR "${PICO_SDK_FETCH_FROM_GIT_PATH}" REALPATH BASE_DIR "${CMAKE_SOURCE_DIR}") | ||
| endif () | ||
| # GIT_SUBMODULES_RECURSE was added in 3.17 | ||
| if (${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.17.0") | ||
| if ("${CMAKE_VERSION}" VERSION_GREATER_EQUAL "3.17.0") |
There was a problem hiding this comment.
Similarly, I suspect there'll never be a space in CMAKE_VERSION ?
There was a problem hiding this comment.
As above, and given cmake's version numbering, I doubt it.
| message("Using PICO_SDK_PATH from environment ('${PICO_SDK_PATH}')") | ||
| endif () | ||
|
|
||
| if (DEFINED ENV{PICO_SDK_FETCH_FROM_GIT} AND (NOT PICO_SDK_FETCH_FROM_GIT)) | ||
| set(PICO_SDK_FETCH_FROM_GIT $ENV{PICO_SDK_FETCH_FROM_GIT}) | ||
| set(PICO_SDK_FETCH_FROM_GIT "$ENV{PICO_SDK_FETCH_FROM_GIT}") |
There was a problem hiding this comment.
PICO_SDK_FETCH_FROM_GIT is a BOOL value, so will never contain a space (AFAIK)
There was a problem hiding this comment.
As above, and as far as I can think, no, a boolean won't ever contain a space.
FreeRTOS_Kernel_import.cmake
Outdated
| endif() | ||
| if (_ACTUAL_PATH STREQUAL _POSSIBLE_PATH) | ||
| get_filename_component(FREERTOS_KERNEL_PATH ${CMAKE_CURRENT_LIST_DIR}/${FREERTOS_KERNEL_RP2040_BACK_PATH} REALPATH) | ||
| get_filename_component(FREERTOS_KERNEL_PATH "${CMAKE_CURRENT_LIST_DIR}/${FREERTOS_KERNEL_RP2040_BACK_PATH}" REALPATH) | ||
| message("Setting FREERTOS_KERNEL_PATH to ${FREERTOS_KERNEL_PATH} based on location of FreeRTOS-Kernel-import.cmake") |
There was a problem hiding this comment.
Probably worth adding single-quotes around the path in the message? (e.g. see line 60)
message("Setting FREERTOS_KERNEL_PATH to '${FREERTOS_KERNEL_PATH}' based on location of FreeRTOS-Kernel-import.cmake")
There was a problem hiding this comment.
I considered it but I'm not sure how the expansion would work (and I didn't touch the other messages because of it). Single quotes should produce a literal output of the variable, whereas double quotes will interpret it.
For example, C:\Program Files would end up as C:\Program\ Files in single quotes (or C:/Program/ Files under *nix systems).
I can test it if necessary. I'll come back to this later when I have more time.
There was a problem hiding this comment.
Oh, I'd kinda assumed that CMake-quoting was a bit like shell-quoting, where only the outer-most quote characters matter? So I was assuming that:
message("Setting FREERTOS_KERNEL_PATH to '${FREERTOS_KERNEL_PATH}' based on location of FreeRTOS-Kernel-import.cmake")
would result in e.g. Setting FREERTOS_KERNEL_PATH to 'C:\Program Files' based on location of FreeRTOS-Kernel-import.cmake, but perhaps CMake is more complicated than that? 🤷 (I'm not very familiar with CMake myself)
There was a problem hiding this comment.
Sorry for the wait. Yep, confirmed that it's fine, so I've updated the file accordingly.
|
@kilograham I guess when the dust has settled on this PR, we'll also want to "upstream" these changes to https://github.com/raspberrypi/pico-sdk/blob/develop/external/pico_sdk_import.cmake and https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/portable/ThirdParty/GCC/RP2040/FreeRTOS_Kernel_import.cmake ? |
|
Okay, latest commit should mean this is ready to merge. Only question left is whether we should keep double quotes for |
My Pico SDK filepath uses spaces, which caused problems with the cmake files. I know it's not ideal to have pathname spaces, but putting double quotes around all the variables in the cmake files should fix this.
I'm not aware of any downsides to this change but I'm no expert on the subject. I did test compiling after the changes and it seems to work fine.
There are two commits because I forgot to include the other two cmake files initially.