Skip to content

build-system: tests do not require the static library#95

Merged
eao197 merged 1 commit intoStiffstream:masterfrom
bengsparks:cmakelists-tweak
May 5, 2025
Merged

build-system: tests do not require the static library#95
eao197 merged 1 commit intoStiffstream:masterfrom
bengsparks:cmakelists-tweak

Conversation

@bengsparks
Copy link
Copy Markdown

Tests only link against the shared library, so the check in the CMakeLists for building the static library only prevents building tests.

add_executable(${UNITTEST} ${UNITTEST_SRCFILES})

@eao197
Copy link
Copy Markdown
Member

eao197 commented May 5, 2025

Hi!

I've read the discussion in NixOS/nixpkgs and don't understand why this is a problem :(
Suppose we'll add a test that requires a static form of SObjectizer library and this will revert your changes. What would we do in that case?

@bengsparks
Copy link
Copy Markdown
Author

bengsparks commented May 5, 2025

What would such a test hope to accomplish? Unittests concern themselves with functionality in isolation, building the library shared or static should not impact this.

@eao197
Copy link
Copy Markdown
Member

eao197 commented May 5, 2025

building the library shared or static should not impact this.

We had several cases in the past when an executable was linked fine with dynamic library, but there were error when linking with static library. Those cases where produced by errors in our code (missing of inline keyword on some functions in .hpp-files), but tests that linked with dynamic library didn't catch such errors.

Anyway, I just want to understand what is the reason. Just avoid to build static version of a library because NixOS always uses dynamic libraries?

@bengsparks
Copy link
Copy Markdown
Author

Ah I see 😄 that makes sense.
Nix packages allow configuration of builds based upon parameters the package receives. Building the shared library is the norm, but there are cases where the static version is desired instead.

It is also custom to run tests to detect if a package is healthy.
As this project only runs its tests when both builds are enabled, this initially left the package testless.
To remedy this, I went through the build system files, spotted that the tests only need the shared library, which culminated in this PR.

Please don't feel pressured into accepting this PR btw 😄 It is custom to submit such patches to upstream, but if you don't want to include it, we can simply include the patch in nixpkgs.

Alternatively, if you wish to add tests to determine if static linkage will break, you could merge this PR, then add a separate test suite that is run only when at least the static library is built. Sort of like the examples, but enabled when tests are on.

@eao197 eao197 merged commit 10eb34c into Stiffstream:master May 5, 2025
@eao197
Copy link
Copy Markdown
Member

eao197 commented May 5, 2025

@bengsparks thanks for explanation.

I think we can accept your PR. In a case when we'll need a test that links against the static library we'll do something. But because we have no such plains now it's better to have more simpler CMake files.

@bengsparks bengsparks deleted the cmakelists-tweak branch May 5, 2025 16:40
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.

2 participants