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 external control of whether to test or install #471

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

dneto0
Copy link
Contributor

@dneto0 dneto0 commented Feb 13, 2024

This makes the project more composable: It can be built and tested as part of a larger set of projects, from source.

Copy link
Contributor

@charles-lunarg charles-lunarg left a comment

Choose a reason for hiding this comment

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

Because this changes the name of BUILD_TESTS, it breaks anyone's local build but also breaks the github actions CI job.

- run: cmake -S . -B build -D BUILD_TESTS=ON -G Ninja

While I'm not sure there is a policy around the naming, the BUILD_TESTS was consistent across other Vulkan repositories. Having a per-repo variable allows tighter control of which repo's are doing what when vendored, but it is not common for vendored repo's to want to test their dependencies.

Put another way, if you want to change the variable name, at least change all occurrences of it in the codebase.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@charles-lunarg charles-lunarg left a comment

Choose a reason for hiding this comment

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

I went ahead and applied the name change into ci.yml. VULKAN_HEADERS_ENABLE_TESTS is a mouthful but it is very explicit and wont be enabled by accident.

@charles-lunarg charles-lunarg requested a review from a user June 14, 2024 20:38
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I'd rebase and squash to make this into 1 commit. Other than that LGTM

This makes the project more composable: It can be built and
tested as part of a larger set of projects, from source.
@charles-lunarg charles-lunarg merged commit 8f034f6 into KhronosGroup:main Jun 14, 2024
8 checks passed
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.

None yet

2 participants