eagles-project / haero

A toolbox for constructing performance portable aerosol packages
Other
3 stars 3 forks source link

EkatCreateUnitTest vs CreateUnitTest #435

Closed jaelynlitz closed 1 year ago

jaelynlitz commented 1 year ago

Even when not using MPI, calling EkatCreateUnitTest vs CreateUnitTest causes different behavior when you have additional libraries to link against.

For example in mam4xx, tests that include and use atmosphere_utils.hpp will throw an undefined reference error when set with CreateUnitTest in the cmake as below, but work when used with EkatCreateUnitTest

CreateUnitTest(mam4_ndrop_unit_tests ndrop_unit_tests.cpp)
vs
EkatCreateUnitTest(mam4_ndrop_unit_tests ndrop_unit_tests.cpp LIBS mam4xx_tests ${HAERO_LIBRARIES})

Should just one of these macros be used as default? Does not using mpi in mam4xx change that choice? Issue open for discussion...

cc @jeff-cohere

jeff-cohere commented 1 year ago

Personally, I think we should just use EkatCreateUnitTest. We created CreateUnitTest when Haero was going to be the aerosol package, and now we have Haero and mam4xx, so we don't have a single context with all the information about which libraries to link in.

@overfelt , @pbosler what do you think about this?

overfelt commented 1 year ago

It seems CreateUnitTest time has passed.

pbosler commented 1 year ago

I like having just one; EKAT's version is good for me.

jeff-cohere commented 1 year ago

Okay, it sounds like we should decommission CreateUnitTest. @jaelynlitz , if you proceed to use EkatCreateUnitTest for your work, I'll set up a PR that gets rid of CreateUnitTest and expunges it from Haero. I think we'll probably need a related PR to switch off of CreateUnitTest in mam4xx first, though.

@pressel @odiazib @mjs271 @CameronRutherford

cameronrutherford commented 1 year ago

I approve of ditching CreateUnitTest for a standardized EkatCreateUnitTest, but I want to make sure the change in behaviour is clear:

# 1 ---
# This does not link against mam4xx_tests or ${HAERO_LIBRARIES}
CreateUnitTest(mam4_ndrop_unit_tests ndrop_unit_tests.cpp)
target_link_libraries(mam4_ndrop_unit_tests PRIVATE mam4xx_tests ${HAERO_LIBRARIES})
# 2 ---
# This links against additional libraries
EkatCreateUnitTest(mam4_ndrop_unit_tests ndrop_unit_tests.cpp LIBS mam4xx_tests ${HAERO_LIBRARIES})

1 links against the additional libraries manually, whereas 2 links against the extra libraries through the single CMake macro call. With the additional call to target_link_libraries, I think the behaviour should be identical.

I am sure we could also make our own mam4xx_add_test macro, but I think the EKAT one serves us just fine here.

jeff-cohere commented 1 year ago

That's correct, Cameron. CreateUnitTest is just a thin wrapper around EkatCreateUnitTest anyway, and we don't have the information we need in Haero to make decisions about which libraries to link against testing executables within mam4xx.