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

Skip building tests by default when included in other projects #931

Merged
merged 1 commit into from
Jan 18, 2025

Conversation

ankane
Copy link
Contributor

@ankane ankane commented Jan 16, 2025

Hi, I think it'd be nice to skip building tests by default when libpqxx is included in other projects. For instance, with FetchContent:

include(FetchContent)

FetchContent_Declare(libpqxx GIT_REPOSITORY https://github.com/jtv/libpqxx.git GIT_TAG 7.10.0)
FetchContent_MakeAvailable(libpqxx)

target_link_libraries(app PRIVATE libpqxx::pqxx)

Currently, projects need to set:

set(SKIP_BUILD_TEST ON)

nlohmann/json is one project that does this.

@tt4g
Copy link
Contributor

tt4g commented Jan 16, 2025

When the changes are merged, the test code that was previously built in the subproject will no longer be built.
It would be better to add to CHANGELOG that “Users who want to continue to build libpqxx tests in subprojects must add set(BUILD_TEST ON) to their CMake project”.

@jtv
Copy link
Owner

jtv commented Jan 17, 2025

It's not clear to me why the tests break on this PR, but at any rate I'm not confident yet that I get this idea.

Also, it relies on a string comparison and I'd like to be sure that that check is robust in the face of soft links, unicode normalisation, etc. if possible.

@tt4g
Copy link
Contributor

tt4g commented Jan 17, 2025

@jtv

CMAKE_SOURCE_DIR is the path to CMakeLists.txt at the top level of the project and CMAKE_CURRENT_SOURCE_DIR is the path to the current CMakeLists.txt.
The two paths are the same for CMakeLists.txt at the top level of the project.
When importing another CMakeLists.txt from CMakeLists.txt at the top level of the project, the two paths will be different in the imported CMakeLists.txt.

See also:

In libpqxx, CMakeLists.txt is the top of the project.
In this, CMAKE_SOURCE_DIR and CMAKE_CURRENT_SOURCE_DIR are the same path.

 CMAKE_SOURCE_DIR:         path/to/libpqxx/CMakeLists.txt
 CMAKE_CURRENT_SOURCE_DIR: path/to/libpqxx/CMakeLists.txt

From CMakeLists.txt, test/CMakeLists.txt is imported by add_subdirectory(test).

add_subdirectory(test)

At this time test/CMakeLists.txt is evaluated, but at this time CMAKE_SOURCE_DIR and CMAKE_CURRENT_SOURCE_DIR are different paths.

-CMAKE_SOURCE_DIR:         path/to/libpqxx/CMakeLists.txt
+CMAKE_CURRENT_SOURCE_DIR: path/to/libpqxx/test/CMakeLists.txt

Also, it relies on a string comparison and I'd like to be sure that that check is robust in the face of soft links, unicode normalisation, etc. if possible.

These two variables are generated by CMake, so there is no need to worry about them.

@jtv
Copy link
Owner

jtv commented Jan 18, 2025

Trying to figure out why the CircleCI test is failing... It doesn't look related but it's entirely unclear. :-(

Thanks @tt4g for the explanation. I'm warming to the idea. On the one hand the tests even when they don't actually run may expose some compile problems on other systems out there before they actually hit any users. On the other hand, the tests take up most of the compile time and most portability problems and such will show up soon enough as people build applications.

@jtv
Copy link
Owner

jtv commented Jan 18, 2025

@ankane it looks like the build is failing because you're not logged into CircleCI.

@jtv jtv merged commit 58379ef into jtv:master Jan 18, 2025
5 of 6 checks passed
@jtv
Copy link
Owner

jtv commented Jan 18, 2025

@ankane I've merged it anyway — bit silly that the test pipeline gets run under your identity but does not check that this is in fact possible. A local build still works. Thanks for the contribution, it'll save a lot of people some time.

@ankane
Copy link
Contributor Author

ankane commented Jan 20, 2025

Thanks @jtv! Another method I've seen is CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME (like CLI11), but seems like there could be collisions.

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.

3 participants