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

Split build production vs testing #29

Merged

Conversation

neatudarius
Copy link
Member

@neatudarius neatudarius commented Jun 28, 2024

Split build production vs testing: #28

@neatudarius neatudarius force-pushed the tweaks-in-cmakelists-for-packaging branch 6 times, most recently from ed29cdf to 2488662 Compare June 29, 2024 00:06
@neatudarius neatudarius changed the title Create a new CMakeLists.txt for tests Split build production vs testing Jun 29, 2024
@neatudarius neatudarius force-pushed the tweaks-in-cmakelists-for-packaging branch 4 times, most recently from 52f43b2 to 70ed031 Compare June 29, 2024 00:33
@neatudarius neatudarius mentioned this pull request Jun 29, 2024
2 tasks
@neatudarius neatudarius requested a review from bretbrownjr June 29, 2024 00:38
@neatudarius neatudarius marked this pull request as ready for review June 29, 2024 00:38
@neatudarius neatudarius force-pushed the tweaks-in-cmakelists-for-packaging branch from 70ed031 to 828cb17 Compare June 29, 2024 16:41
@steve-downey
Copy link
Member

With optional being a template, I'm concerned that not building tests will even more easily lead to silent breaks in building the library that only show up in users of the library. Although optional is pretty simple, I wouldn't want to encourage packagers to skip tests, even by accident. It's more important that a Conan or Vcpkg build self-test because it's even more likely that something is outside of our test matrix.

I do have some code from an earlier version of my scratch project that will check if the gtest project exists before enabling the code, though.

Might also switch, temporarily, to pulling googletest in as a git subtree rather than as a git submodule, which would mean we have a copy, but can also pull in updates, at least while we sort out a package reuse mechanism. Or switch to something lighter weight to vendor in.

@neatudarius
Copy link
Member Author

neatudarius commented Jul 3, 2024

With optional being a template, I'm concerned that not building tests will even more easily lead to silent breaks in building the library that only show up in users of the library. Although optional is pretty simple, I wouldn't want to encourage packagers to skip tests, even by accident. It's more important that a Conan or Vcpkg build self-test because it's even more likely that something is outside of our test matrix.

I'm OK with both approaches, but I would like to use an uniform approach for all libraries in Beman - CC: @camio , maybe we can add this topic to our sync agenda. Also, I propose to move this discussion in https://discourse.bemanproject.org/t/build-tests-always-implied-or-optional-task/144.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakePresets.json Show resolved Hide resolved
src/Beman/Optional26/tests/CMakeLists.txt Show resolved Hide resolved
src/Beman/Optional26/tests/CMakeLists.txt Show resolved Hide resolved
src/Beman/Optional26/tests/CMakeLists.txt Outdated Show resolved Hide resolved
src/Beman/Optional26/tests/CMakeLists.txt Outdated Show resolved Hide resolved
src/Beman/Optional26/tests/CMakeLists.txt Outdated Show resolved Hide resolved
src/Beman/Optional26/tests/CMakeLists.txt Outdated Show resolved Hide resolved
src/Beman/Optional26/tests/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@camio camio left a comment

Choose a reason for hiding this comment

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

I mentioned a couple minor things, but it otherwise looks good. Note that I didn't review the docker file at all.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Beman/Optional26/tests/CMakeLists.txt Outdated Show resolved Hide resolved
@neatudarius
Copy link
Member Author

I mentioned a couple minor things, but it otherwise looks good. Note that I didn't review the docker file at all.

Sorry, I added the Dockerfile by mistake. I don't want to added right now. Currently I'm using it to replicate the environment from CI, but it's not ready. TBD

@neatudarius neatudarius force-pushed the tweaks-in-cmakelists-for-packaging branch 4 times, most recently from fc86a36 to 310212f Compare July 17, 2024 09:54
@neatudarius neatudarius force-pushed the tweaks-in-cmakelists-for-packaging branch 3 times, most recently from 3dd2f8b to c1d1892 Compare July 17, 2024 10:19
@neatudarius neatudarius force-pushed the tweaks-in-cmakelists-for-packaging branch from c1d1892 to 8d0a514 Compare July 17, 2024 10:20
@neatudarius neatudarius force-pushed the tweaks-in-cmakelists-for-packaging branch 2 times, most recently from 20ab0df to e72636e Compare July 17, 2024 11:19
@neatudarius neatudarius force-pushed the tweaks-in-cmakelists-for-packaging branch from e72636e to c57d250 Compare July 17, 2024 11:26
@neatudarius neatudarius force-pushed the tweaks-in-cmakelists-for-packaging branch 3 times, most recently from 952a3ba to a735b3c Compare July 17, 2024 15:52
@neatudarius neatudarius force-pushed the tweaks-in-cmakelists-for-packaging branch from a735b3c to e167523 Compare July 17, 2024 15:56
@neatudarius neatudarius requested a review from dietmarkuehl July 17, 2024 15:57
@neatudarius neatudarius merged commit bfeb908 into bemanproject:main Jul 17, 2024
5 checks passed
@steve-downey steve-downey mentioned this pull request Jul 22, 2024
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