Skip to content

cmake: Expose PYBIND11_TEST_OVERRIDE #2218

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

Merged

Conversation

EricCousineau-TRI
Copy link
Collaborator

Primarily for the ccmake curses GUI

I've found myself using this quite a bit, and would love to have it a bit more accessible.

@EricCousineau-TRI EricCousineau-TRI changed the title tests: Expose PYBIND11_TEST_OVERRIDE cmake: Expose PYBIND11_TEST_OVERRIDE May 18, 2020
@EricCousineau-TRI EricCousineau-TRI force-pushed the feature-cmake-test-override-cache branch from 90cfa65 to b4d03e7 Compare May 18, 2020 20:26
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Would it make sense to add a comment to your change in CMakeList.txt, to explain what it is good for?
(I'm not a regular cmake users myself, but I imagine a comment targeted at cmake users could be very helpful.)

@EricCousineau-TRI
Copy link
Collaborator Author

@rwgk I'm not sure if it would make sense, as set(... CACHE ...) / option(...) cache vars are pretty standard for CMake configuration.
https://cmake.org/cmake/help/latest/command/set.html#set-cache-entry
https://cmake.org/cmake/help/latest/command/option.html

@jamiesnape If you have time, can I ask your opinion (on the CMake side)?

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@jamiesnape
Copy link
Contributor

I do not think the CMake constructs need documenting, but the docstring in the set(... CACHE) could be improved, I think. Override tests is a little vague. Something like "tests from ;-separated list of *.cpp files will be built instead of all tests"?

Primarily for the ccmake curses GUI
@EricCousineau-TRI EricCousineau-TRI force-pushed the feature-cmake-test-override-cache branch from b4d03e7 to 45381fe Compare May 26, 2020 20:31
@EricCousineau-TRI
Copy link
Collaborator Author

Done - thanks!

@wjakob
Copy link
Member

wjakob commented May 31, 2020

LGTM, thanks!

@wjakob wjakob merged commit 2c30e0a into pybind:master May 31, 2020
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