Set the right path for PODIO_SIOBLOCK_PATH and add an error message

[jmcarcell]

BEGINRELEASENOTES

• Set the right path for PODIO_SIOBLOCK_PATH, fixing unit tests that read and write SIO files (there aren't any at the moment)
• Add an error message when PODIO_SIOBLOCK_PATH is set but none of the SioBlock libraries are found.

ENDRELEASENOTES

Adding a simple test that writes a frame to a SIO file won't work when ran with ctest. It looks like the environment is not correctly set because when running the test manually it will work fine. I don't know why but using LD_LIBRARY_PATH instead of DL_PATHS fixes this. This is the test

+#if PODIO_ENABLE_SIO
+
+TEST_CASE("SIO writing a frame", "[relations]") {
+  auto frame = podio::Frame();
+  auto emptyColl = ExampleClusterCollection();
+  frame.put(std::move(emptyColl), "emptyClusters");
+  auto writer = podio::SIOWriter("unittests_frame_sio.sio");
+  writer.writeFrame(frame, podio::Category::Event);
+  writer.finish();
+}
+
+#endif

Here I don't think we need a unittest to simply test the environment, but for running with sanitizers, since all the ROOT ones fail then it's useful to have the SIO ones run from the unit tests.

[tmadlener]

I think I originally added the DL_PATHS like this because catch_discover_tests actually runs the executable during the build and it didn't find all the datamodel libraries without injecting the DL_PATHS like this.

This is the original commit that introduced it, it also contains a link to the original issue on Catch2.

At least for now the sanitizer builds all skip the test discovery because they run in an LCG environment where the CMake version is too old to have DL_PATHS. However, also on dev3 with a newer environment things seem to work as expected with test discovery.

Did you check that the tests run in the correct environment with this change? I.e. PODIO_SIOBLOCK_PATH is also set? IIRC, there was also an issue with trying to set multiple environment variables this way with catch_discover_tests)

[jmcarcell]

I noticed but since it works fine without PODIO_SIOBLOCK_PATH didn't give it much thought. Actually, based on https://github.com/catchorg/Catch2/issues/2424, it's possible to set several variables at the same time. But when doing that and setting both LD_LIBRARY_PATH and PODIO_SIOBLOCK_PATH the unittest crashes. So actually that tells us that the problem is not with LD_LIBRARY_PATH but with PODIO_SIOBLOCK_PATH, and indeed removing both also makes this new unittest pass (and all the others). Why do we need PODIO_SIOBLOCK_PATH? What do we want to do here? Since all the test pass without PODIO_SIOBLOCK_PATH I would just remove it.

[jmcarcell]

So having a look I think the options are:

• Remove PODIO_SIOBLOCK_PATH and just use LD_LIBRARY_PATH. From the release notes it seems it was introduced when having multiple installations but then LD_LIBRARY_PATH also needs to be changed anyway.
• If PODIO_SIOBLOCK_PATH is not removed, a check should be added to not crash when the library is not found. Then the path can be modified since the current one is wrong and both PODIO_SIOBLOCK_PATH and LD_LIBRARY_PATH are set.

[tmadlener]

PODIO_SIOBLOCK_PATH was introduced to have the possibility to choose which SioBlocks libraries to actually. If we just have them on LD_LIBRARY_PATH that is much harder to do and in the past test regularly broke simply because we couldn't get a clean test environment to run the SIO backend tests, because we could not clean up the LD_LIBRARY_PATH (e.g. LCG stacks, where this usually pulls in a complete <stack-prefix>/lib). It's far eaiser (also for users) to hand-pick things into a PODIO_SIOBLOCK_PATH and adjust that to only load what they want than it is to try and play with the LD_LIBRARY_PATH.

I am not sure I understand how the failure appears with one but not the other. We first check if PODIO_SIOBLOCK_PATH is set and if not we see if we can find LD_LIBRARY_PATH. In case neither is set, we don't try to load any libraries and otherwise the library loading just continues exactly the same for both:

podio/src/SIOBlock.cc

Lines 141 to 168 in 82814a8

	std::vector<std::tuple<std::string, std::string>> libs;
	const auto ldLibPath = []() {
	// Check PODIO_SIOBLOCK_PATH first and fall back to LD_LIBRARY_PATH
	auto pathVar = std::getenv("PODIO_SIOBLOCK_PATH");
	if (!pathVar) {
	pathVar = std::getenv("LD_LIBRARY_PATH");
	}
	return pathVar;
	}();
	if (!ldLibPath) {
	return libs;
	}
	std::string dir;
	std::istringstream stream(ldLibPath);
	while (std::getline(stream, dir, ':')) {
	if (not fs::exists(dir)) {
	continue;
	}
	for (auto& lib : fs::directory_iterator(dir)) {
	const auto filename = lib.path().filename().string();
	if (filename.find("SioBlocks") != std::string::npos) {
	libs.emplace_back(std::move(filename), dir);
	}
	}
	}

[jmcarcell]

The issue is that PODIO_SIOBLOCK_PATH is set but not to the correct path for unittests, and this isn't a problem because there isn't yet a test that triggers it. Then the library isn't found but it doesn't fall back to LD_LIBRARY_PATH as this only happens when PODIO_SIOBLOCK_PATH isn't set. I added an error message for this case, which would be triggered with the current set up. Then, the changes in CMakeLists.txt make it point to the right directory.

[tmadlener]

Thanks for fixing this. It looks like I messed this up a bit in the whole test reorganization.

If we remove the DL_PATHS, I think we also need to adapt this comment here to not create too much confusion:

podio/tests/unittests/CMakeLists.txt

Lines 66 to 72 in 82814a8

	# To work around https://github.com/catchorg/Catch2/issues/2424 we need the
	# DL_PATH argument for catch_discoer_tests which requires CMake 3.22 at least
	# The whole issue can be avoided if we skip the catch test discovery and set the
	# environment on our own
	if (CMAKE_VERSION VERSION_LESS 3.22)
	set(SKIP_CATCH_DISCOVERY ON)
	endif()

[tmadlener]

Is this second ENVIRONMENT necessary?

[jmcarcell]

Having a look at the comment, I think the SKIP_CATCH_DISCOVERY option can be removed; the point of that part is to check if the cmake version is less than 3.22 because in principle using DL_PATH is needed and this is supported only for 3.22 and above. But, it is actually not needed, because duplicating the ENVIRONMENT keyword works (it's described in the Catch2 issue).

[tmadlener]

We even get the fine grained list for unittests in the sanitizer builds with this. Very nice. Thanks :)