build: do not export valgrind with export() #2189
Conversation
|
@robinchrist hi Robin, could you help test this change? |
in Seastar, valgrind library is only used at the build time, and is
used internally for accessing its header files when buildings its
internal .cc files. so we don't expect to expose it via `export()`,
for exposing it in `SeastarTargets.cmake`.
but somehow when building Seastar as a static library, valgrind is
still exposed in Seastar's CMake configuration file. if a CMake
based project consume Seastar using its CMake configuration file,
it runs into following failure when CMake generates the building
system:
```
-- Configuring done (2.5s)
CMake Error at external/seastar/build/release/SeastarTargets.cmake:52 (set_target_properties):
The link interface of target "Seastar::seastar" contains:
Valgrind::valgrind
but the target was not found. Possible reasons include:
* There is a typo in the target name.
* A find_package call is missing for an IMPORTED target.
* An ALIAS target is missing.
Call Stack (most recent call first):
external/seastar/build/release/SeastarConfig.cmake:36 (include)
CMakeLists.txt:6 (find_package)
```
fortunately, in CMake 3.26, `BUILD_LOCAL_INTERFACE` was introduced
this problem in specific, so that the linked library is not included
by `export()` anymore.
in this change, we include `Valgrind::valgrind` using this generator
expression if the CMake's version is greater or equal to 3.26, so that
`Valgrind` is not required for building Seastar applications.
as this generator expression is not available on CMake versions lower
than 3.26, and the minimal required CMake version for building Seastar
is 3.13, and Seastar does support static build. The issue is still not
fully addressed even with this change with older CMake and if Seastar
is built as a static library.
previously, 372a16a intended to address this. but somehow Valgrind
is still exposed by SeastarTargets.cmake.
Refs scylladb#2180
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
tchaikov
force-pushed
the
build-sans-valgrind
branch
from
d0868e1
to
988ff26
Compare
|
@scylladb/seastar-maint hello maintainers, could you help review this change? |
|
We should consider dropping valgrind. Is there anything it does that asan/ubsan does not? |
|
Yep I can confirm that this fix is working! Thanks a lot @tchaikov |
Four years ago, after someone who was a valgrind fan decided that Seastar must depend on it, I wrote a patch in https://groups.google.com/g/seastar-dev/c/f0WfaK0KgZ0/m/qK2WlDMrCAAJ making this dependency optional - i.e., if you (a Seastar developer) already have valgrind installed, it will be compiled in a way that you can use valgrind on it. But if you don't have valgrind installed? That's fine, and you can still compile without it. But there was fierce opposition to my patch and it never got in. Maybe it's time to reconsider - my original patch or something else with the same effect of making valgrind optional. |
…needed anymore after upgrading seastar as scylladb/seastar#2189 was merged
if it's desirable to make valgrind optional, i can add a |
in Seastar, valgrind library is only used at the build time, and is used internally for accessing its header files when buildings its internal .cc files. so we don't expect to expose it via
export(), for exposing it inSeastarTargets.cmake.but somehow when building Seastar as a static library, valgrind is still exposed in Seastar's CMake configuration file. if a CMake based project consume Seastar using its CMake configuration file, it runs into following failure when CMake generates the building system:
fortunately, in CMake 3.26,
BUILD_LOCAL_INTERFACEwas introduced this problem in specific, so that the linked library is not included byexport()anymore.in this change, we include
Valgrind::valgrindusing this generator expression if the CMake's version is greater or equal to 3.26, so thatValgrindis not required for building Seastar applications.as this generator expression is not available on CMake versions lower than 3.26, and the minimal required CMake version for building Seastar is 3.13, and Seastar does support static build. The issue is still not fully addressed even with this change with older CMake and if Seastar is built as a static library.
previously, 372a16a intended to address this. but somehow Valgrind is still exposed by
SeastarTargets.cmake.Refs #2180
Signed-off-by: Kefu Chai kefu.chai@scylladb.com