Change finding of EDM4hep, ROOT and podio to use generator expressions

[jmcarcell]

BEGINRELEASENOTES

• Use generator expressions to find EDM4hep, ROOT and podio in the test environment, in case the targets don't exist at configure time

ENDRELEASENOTES

Old text before the edits:

There are a couple of reasons for this change: in some builds, some targets may not exist so this will cause an error. Also in my opinion paths for EDM4hep, ROOT and podio should be provided by the environment and failure to do so should be a test failure. When modifications are done to the test environment the run environment will be different so that reduces how useful the tests are as the same thing may fail at runtime because of the environment settings.

[tmadlener]

These are probably there to avoid issue in root dictionary loading during the tests, because just using LD_LIBRARY_PATH can be extremely messy. We have run into several issues with LD_LIBRARY_PATH in the past, especially if things changed in dictionaries. I started introducing these target dependencies to have as small of a test environment as possible in other repos.

It is not necessarily more readable, but it is possible to use generator expressions here (even to guard against non-existant targets).

$<$<TARGET_EXISTS:podio>:$<TARGET_FILE_DIR:podio>>

Also I think in this case, all of the targets that would be removed are dependencies of k4FWCore, so I don't see how they would not be available.

[jmcarcell]

But even if LD_LIBRARY_PATH is messy, by having the tests modify the environment then it's impossible to know at install time (considering the tests part of the installation) whether at runtime it will work or not. I don't understand exactly what you mean by "if things changed in dictionaries".

And I meant not that they don't exist but that they location can't be retrieved if they haven't been installed, which is what happens now. Alternatively the generator expressions work for me (but I think not modifying the environment and letting tests fail as it would happen at runtime is a better design):

  set_property(TEST ${_testname} APPEND PROPERTY ENVIRONMENT "ROOT_INCLUDE_PATH=$<TARGET_FILE_DIR:podio::podio>/../include/podio:$<TARGET_FILE_DIR:EDM4HEP::edm4hepDict>/../include:$ENV{ROOT_INCLUDE_PATH}")
  set_property(TEST ${_testname} APPEND PROPERTY ENVIRONMENT "LD_LIBRARY_PATH=${PROJECT_BINARY_DIR}:${PROJECT_BINARY_DIR}/${CMAKE_PROJECT_NAME}:${PROJECT_BINARY_DIR}/test/k4FWCoreTest:$<TARGET_FILE_DIR:ROOT::Core>:$<TARGET_FILE_DIR:EDM4HEP::edm4hepDict>:$<TARGET_FILE_DIR:podio::podio>:$ENV{LD_LIBRARY_PATH}")
  set_property(TEST ${_testname} APPEND PROPERTY ENVIRONMENT "PYTHONPATH=${PROJECT_BINARY_DIR}/${CMAKE_PROJECT_NAME}/${GAUDI_GENCONF_DIR}:${PROJECT_BINARY_DIR}/test/k4FWCoreTest/${GAUDI_GENCONF_DIR}:$ENV{PYTHONPATH}")

[tmadlener]

considering the tests part of the installation

I personally always consider (most of) the tests as part of the build, rather than the installation. After all for most of the development I want to see whether my current changes work without having to reinstall things inbetween.

I don't understand exactly what you mean by "if things changed in dictionaries".

Whenever a header of something that goes into a root dictionary changes it can be hard to make sure that the current dictionary is read first. This will make tests fail at runtime. This is probably more of an issue with podio and EDM4hep than it is for k4FWCore, as k4FWCore doesn't build a new dictionary for either of those.

And I meant not that they don't exist but that they location can't be retrieved if they haven't been installed, which is what happens now.

But if they haven't been installed how would you be able to run the tests in the first place (even if LD_LIBRARY_PATH points at the right direction)? In that case I would like the failure to be as early as possible during configuration, rather than at test time, no?

but I think not modifying the environment and letting tests fail as it would happen at runtime is a better design

I am not sure I agree with that (fully). During development (and also CI) I basically want to know whether things work if I have a consistent environment. The problem of building such an environment for use at runtime is a somewhat orthogonal concern for me and the individual packages shouldn't really have to care.

[andresailer]

I get it now, I think?
You get this error when doing add_subdirectory with podio, edm4hep, k4FWCore?

CMake Error at k4FWCore/test/k4FWCoreTest/CMakeLists.txt:45 (get_target_property):
  The LOCATION property may not be read from target "edm4hepDict".  Use the                                                                                                                                                                   
  target name directly with add_custom_command, or use the generator                                                                                                                                                                          
  expression $<TARGET_FILE>, as appropriate.                                                                                                                                                                                                  



CMake Error at k4FWCore/test/k4FWCoreTest/CMakeLists.txt:51 (get_target_property):
  The LOCATION property may not be read from target "podio".  Use the target                                                                                                                                                                  
  name directly with add_custom_command, or use the generator expression                                                                                                                                                                      
  $<TARGET_FILE>, as appropriate.       

?

But then just replace things with the generator expression (TARGET_FILE_DIR), and problem solved, because that moves things from the configuration step to the build step??

[jmcarcell]

Yes it's about the error. The point is also, do we need that? If yes then it should be in every repository that depends on podio or edm4hep but k4FWCore is the only one therefore we don't need it and it can be removed. Otherwise we should add it to every repository if we think this is needed. This is why I went for deleting instead of using generator expressions; I couldn't find something similar in any of the other repos.

[andresailer]

This is needed when testing newer versions of podio, edm4hep, etc. on top of stacks that have existing podio, edm4hep etc.

[jmcarcell]

But in that situation typically you add the newer versions to your environment variables so in that case with the environment variables are enough. And if you don't then it won't find the newer versions of course and in that case this PR makes no difference. And then we could conclude finding podio, edm4hep is not necessary, or am I missing something?

[andresailer]

But this package depends on podio, so of course it has to find podio?

To find podio you only need CMAKE_PREFIX_PATH, nothing else in the environment, this code makes sure you get the rest of the environment correct for your test.

[jmcarcell]

To find podio you only need CMAKE_PREFIX_PATH, nothing else in the environment, this code makes sure you get the rest of the environment correct for your test.

But how often does anyone have CMAKE_PREFIX_PATH set and not the others? Even in the case when there is a stack version and a local one.
I updated the PR to use generator expressions although I'm still in favor of just getting rid of it, let people / other tools set the proper environment for when there are multiple versions

[jmcarcell]

Any more comments? If not I merge this today