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 cmake handling of paths with spaces (RP2040) #1255

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

Conversation

TankedThomas
Copy link

Fix cmake handling of paths with spaces

Description

Double quotes are added to cmake variables in order to handle pathname spaces (or other special characters).

Test Steps

When building with the related cmake files over on the raspberrypi/debugprobe repo, I ran into issues with my pathnames having spaces in them.
The same thing might happen here, so the maintainers of debugprobe suggested pushing this fix upstream.

Checklist:

  • I have tested my changes. No regression in existing tests.
    I'm not sure how to build FreeRTOS to test, but this should work fine according to my tests with raspberrypi/debugprobe
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.
    I don't believe unit test changes should be necessary here.

Related Issue

Related raspberrypi/debugprobe pull request

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Signed-off-by: TankedThomas <[email protected]>
@TankedThomas TankedThomas requested a review from a team as a code owner March 11, 2025 10:45
@AniruddhaKanhere
Copy link
Member

Thank you @TankedThomas for your contribution. The changes look correct to me. I have approved the CI check run - let us see if that passes successfully.

@AhmedIsmail02
Copy link
Member

@TankedThomas Thank you for your contribution, I've checked the RP2040 port part in portable/CMakeLists.txt file where I can see ${CMAKE_CURRENT_LIST_DIR} which produces an absolute path doesn't have the double quotes, how is it working without your changes?

@kar-rahul-aws
Copy link
Member

kar-rahul-aws commented Mar 12, 2025

@AhmedIsmail02 I think this is because CMake looks for whitespace to separate arguments before it evaluates any variables.
For example:-

add_custom_target(PrintMessage
    COMMAND ${CMAKE_COMMAND} -E echo
                "$<IF:$<CONFIG:Debug>,This is debug,Must be something else>"
)

Here double quotes are needed , To prevent CMake from treating the spaces in the expression as argument separators.

add_custom_target(PrintMessage
    COMMAND ${CMAKE_COMMAND} -E echo
                $<IF:${is_debug},${msg_debug},${msg_other}>
)

No quotes are needed around the final $IF:... generator expression in the above, due to lack of white spaces.

In the similar way, in RP2040 port part in portable/CMakeLists.txt
there are no white spaces, so when ${CMAKE_CURRENT_LIST_DIR} or ${FREERTS_PORT} is substituted, it retains spaces but does not get split further.

@AhmedIsmail02
Copy link
Member

AhmedIsmail02 commented Mar 12, 2025

@kar-rahul-aws Thanks for your reply, As far as I understand and according to your reply I think some of the changes are not needed since CMake looks for white spaces separator before evaluating variables. Hence, paths containing white spaces shouldn't be an issue in that case. I believe that wrapping strings between double quotes is generally the safer option, but it might be a bit confusing for users/developers if it's not consistent.

@kilograham
Copy link
Contributor

@kar-rahul-aws Thanks for your reply, As far as I understand and according to your reply I think some of the changes are not needed since CMake looks for white spaces separator before evaluating variables. Hence, paths containing white spaces shouldn't be an issue in that case. I believe that wrapping strings between double quotes is generally the safer option, but it might be a bit confusing for users/developers if it's not consistent.

I agree; the issue doesn't identify which unquoted paths are a problem; i think it would be much preferable to only modify those if the number is limited

@kilograham
Copy link
Contributor

I assume it is just the get_filename_component calls

@aggarg
Copy link
Member

aggarg commented Mar 18, 2025

@TankedThomas Can you please check @kilograham's suggestion and limit the changes to only the required ones?

@TankedThomas
Copy link
Author

Sorry everyone, really busy this week. I'll get back to you ASAP but it probably won't be until the end of next week.

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.

7 participants