AIDASoft / podio

PODIO
GNU General Public License v3.0
24 stars 60 forks source link

Set the right path for `PODIO_SIOBLOCK_PATH` and add an error message #588

Closed jmcarcell closed 6 months ago

jmcarcell commented 6 months ago

BEGINRELEASENOTES

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.

jmcarcell commented 6 months ago

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 commented 6 months ago

So having a look I think the options are:

tmadlener commented 6 months ago

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:

https://github.com/AIDASoft/podio/blob/82814a8800e246af71077982e270672b71ed6531/src/SIOBlock.cc#L141-L168

jmcarcell commented 6 months ago

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 commented 6 months ago

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