Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests build with installed version of DAGMC if present #877

Open
pshriwise opened this issue Apr 11, 2023 · 8 comments
Open

Tests build with installed version of DAGMC if present #877

pshriwise opened this issue Apr 11, 2023 · 8 comments

Comments

@pshriwise
Copy link
Member

Describe the Bug

With the merge of #876 I noticed after pulling those changes that I couldn't rebuild DagMC successfully due to missing symbols at the linking stage of building our test programs. The error was a missing symbol to the DagMC constructor with signature

DagMC::DagMC(std::shared_ptr<Interface>, double, double)

We of course now have an extra int argument at the end of our constructors. The problem turned out to be that the test program is using the installed DagMC.hpp header instead of the one in the build directory. The installed header came from before I pulled the recent changes in that PR and contains the old constructor signature. When that test program tries to link to the DAGMC library compiled in the build directory, it then throws an error for a missing symbol.

To Reproduce

For an existing build of DAGMC (w/ PATH env var set to include the current DAGMC installation), pull the changes from #876 and attempt to rebuild DAGMC.

@gonuke
Copy link
Member

gonuke commented Apr 13, 2023

To make sure I understand: the solution would be to force all tests to use the headers from the build directory, right?

@pshriwise
Copy link
Member Author

To make sure I understand: the solution would be to force all tests to use the headers from the build directory, right?

That would be my proposal, yes. I haven't looked at the CMake files, but I don't expect it would be a big change.

@ahnaf-tahmid-chowdhury
Copy link
Member

One way to do this is by adding the build directory to the include path before the installation directory.

include_directories(${CMAKE_BINARY_DIR}/src/dagmc ${CMAKE_INSTALL_INCLUDEDIR})

Or, we can modify the tests themselves.

#include "${CMAKE_BINARY_DIR}/src/dagmc/DagMC.hpp"

@gonuke
Copy link
Member

gonuke commented May 9, 2023

What if we add SET(CMAKE_INCLUDE_DIRECTORES_BEFORE ON)?

Seems like a big hammer, and surprised we haven't encountered it before, but should work?

@ahnaf-tahmid-chowdhury
Copy link
Member

What if we add SET(CMAKE_INCLUDE_DIRECTORES_BEFORE ON)?

Seems like a big hammer, and surprised we haven't encountered it before, but should work?

According to my knowledge, DAGMC depends on third-party libraries installed in the system include directories. So, if these libraries have conflicting versions or headers with DAGMC, this could lead to unexpected behavior or compilation errors. However, we can resolve that by testing DAGMC in different build environments (Geant4, OpenMC, PyNE etc) to ensure that it works as expected.

@gonuke
Copy link
Member

gonuke commented May 9, 2023

But for the purpose of testing we probably always want the most local headers, right?

@ahnaf-tahmid-chowdhury
Copy link
Member

But for the purpose of testing we probably always want the most local headers, right?

Yes, it is usually best to use the most local headers to ensure that the tests are using the correct version of the code.

Setting CMAKE_INCLUDE_DIRECTORIES_BEFORE to ON can be one way to achieve this, but as I mentioned earlier, it can have unintended consequences if not used carefully.

@ahnaf-tahmid-chowdhury
Copy link
Member

I have tried all the methods above, but nothing has worked out. I think we need to reconfigure the dagmc_install_exe or dagmc_get_link_libs macros?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants