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

Allow FetchContent usage in external projects #1166

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

nicovanduijn
Copy link
Contributor

Description

My project has an existing FreeRTOS Kernel and I attempted to include this project via CMake's FetchContent function as suggested by the Readme. In this process I stumbled over two minor details:

  • Copy-paste error in the readme, suggesting to set defines for the FAT build instead of the TCP library
  • No functionality to prevent the TCP library to pull in its own copy of the kernel, as other libraries have

Test Steps

  • Set up a project with bare-bones FreeRTOS
  • Consume this library via CMake's FetchContent as suggested by the readme

Checklist:

  • [~] I have tested my changes. No regression in existing tests.
    -> I have obviously tested that this solution fixes my problem, but I'm unsure how I can test it against regressions in the upstream CI other than just opening this PR.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.
    -> No real code changes outside the build system, so this does not apply

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

@nicovanduijn nicovanduijn requested a review from a team as a code owner July 2, 2024 14:31
@kstribrnAmzn
Copy link
Member

Thank you for submitting this PR! I took a look over it and saw nothing wrong. I'll send this to the TCP experts on the team for their review.

CMakeLists.txt Outdated
Comment on lines 177 to 179
if(FREERTOS_PLUS_TCP_FETCH_FREERTOS)
FetchContent_MakeAvailable(freertos_kernel cmock)
endif()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMake documentation (for the latest release) mentions

True if the constant is 1, ON, YES, TRUE, Y, or a non-zero number (including floating point numbers). False if the constant is 0, OFF, NO, FALSE, N, IGNORE, NOTFOUND, the empty string, or ends in the suffix -NOTFOUND.

We should keep default behavior to clone the FreeRTOS kernel. This default behavior is used in this now failing check. Maybe negate your variable to something like FREERTOS_PLUS_TCP_USE_LOCAL_FREERTOS. Then your condition could be...

if(NOT FREERTOS_PLUS_TCP_USE_LOCAL_FREERTOS)
    FetchContent_MakeAvailable(freertos_kernel cmock)
endif()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great input, indeed this breaks existing builds. Thanks for the pointer.

I flipped the logic, hopefully CI passes now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it makes sense to check if we are the top level CMake project and only clone FreeRTOS kernel then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know this was possible until I googled it just now. Is doing this considered best practice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@archigup I've changed the logic as you suggested

kstribrnAmzn
kstribrnAmzn previously approved these changes Jul 2, 2024
archigup
archigup previously approved these changes Jul 5, 2024
@n9wxu n9wxu merged commit 3979677 into FreeRTOS:main Jul 8, 2024
10 checks passed
@nicovanduijn nicovanduijn deleted the nico/bugfix/cmake-external branch July 9, 2024 06:36
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.

4 participants