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

CMake: allow skipping include of settings.cmake #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

axel-h
Copy link
Member

@axel-h axel-h commented Jun 23, 2021

My build system currently sets up everything and then uses add_subdirectory() to include sel4test. This patch allow building sel4test then. Maybe there is a smarter solution here by impersonating settings.cmake somehow to this does nothing?

@axel-h axel-h requested a review from kent-mcleod June 23, 2021 15:01
@axel-h axel-h force-pushed the patch-axel-1 branch 2 times, most recently from 415cc22 to beca6a3 Compare June 23, 2021 15:46
@kent-mcleod
Copy link
Member

What is your build system setting up before including sel4test? Could you instead include add_subdirectory(apps/sel4test-driver) instead of the toplevel CMakeLists.txt that currently expects to setup the elfloader and kernel.elf targets?

@axel-h
Copy link
Member Author

axel-h commented Dec 1, 2021

Our build system basically does what settings.cmake does and that conflicts. The goal is being able to use existing seL4 projects like this without modification, so we don't have to create another wrapper project. Having the switch SEL4TEST_DONT_USE_SEL4_DEFAULT_BUILD_SYSTEM nicely solves this.

@kent-mcleod
Copy link
Member

The goal is being able to use existing seL4 projects like this without modification, so we don't have to create another wrapper project.

But isn't this already a modification? The coupling between settings.cmake and CMakeLists.txt isn't a stable public interface. settings.cmake contains many sel4test specific options and options can be moved between CMakeLists.txt and settings.cmake without causing external breakages.

add_subdirectory(apps/sel4test-driver) or even using https://cmake.org/cmake/help/latest/variable/CMAKE_PROJECT_PROJECT-NAME_INCLUDE_BEFORE.html#variable:CMAKE_PROJECT_%3CPROJECT-NAME%3E_INCLUDE_BEFORE will let you include the sel4test applications into another project without requiring special modifications upstream.

@axel-h
Copy link
Member Author

axel-h commented Dec 8, 2021

Thanks, I will give this a try.

@kent-mcleod
Copy link
Member

@axel-h, feel free to merge this if you still need the change

@axel-h axel-h marked this pull request as ready for review March 3, 2023 12:03
@axel-h
Copy link
Member Author

axel-h commented Mar 9, 2023

I have not found a better solution yet for our use case with a different/custom build system. I will check our internal CI one more tomorrow and merge this then. Thanks.

@Indanz
Copy link
Contributor

Indanz commented Jun 25, 2023

I think this can be merged.

@axel-h axel-h marked this pull request as draft July 5, 2023 04:47
CMakeLists.txt Show resolved Hide resolved
@axel-h axel-h marked this pull request as ready for review March 31, 2024 23:39
@axel-h axel-h marked this pull request as draft April 8, 2024 19:48
@axel-h axel-h added the hw-test set to run sel4test hardware test for this PR label Apr 10, 2024
@lsf37 lsf37 marked this pull request as ready for review June 18, 2024 09:00
@lsf37 lsf37 removed the hw-test set to run sel4test hardware test for this PR label Jun 18, 2024
@lsf37 lsf37 self-assigned this Jun 18, 2024
@lsf37
Copy link
Member

lsf37 commented Jun 18, 2024

@axel-h this PR should either be closed or merged — happy to merge it if it’s useful for you, otherwise I’d close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants