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

Define libiGeom as a dependency of libmcnp2cad if part of a larger pr… #50

Merged
merged 2 commits into from
Nov 19, 2018

Conversation

pshriwise
Copy link
Member

This fixes a bug I introduced in #43.

The if block here doesn't set IGEOM_LIB at all if mcnp2cad is part of a bigger project, so the iGeom library won't be linked (unless you were testing the previous change on in a directory with cached CMake variables like a fool...).

This PR addresses that problem by adding iGeom as a dependency of the mcnp2cad library by default. If the project is being configured as a standalone, an external iGeom library must be found.

@gonuke
Copy link
Member

gonuke commented Nov 7, 2018

This change doesn't make sense to me...

@gonuke gonuke closed this Nov 7, 2018
@gonuke gonuke reopened this Nov 7, 2018
@gonuke
Copy link
Member

gonuke commented Nov 7, 2018

closed by accident!

@gonuke
Copy link
Member

gonuke commented Nov 7, 2018

Doesn't this just hardcode something we IF'ed out of the flow?

@gonuke
Copy link
Member

gonuke commented Nov 7, 2018

Don't we just need to make this a hard dependency with FIND_PACKAGE and fix the way we use it from the plugin, etc?

@pshriwise
Copy link
Member Author

The problem with using only FIND_PACKAGE is that it will require en existing iGeom library during configuration of the plugin, which, given that the build command hasn'e been run yet, won't exist. This will cause the configuration to fail and lead us back to a situation where we require several make/cmake commands to successfully build the project.

My thought here is that we now cover the standalone case by using FIND_PACKAGE which would require that an iGeom library already exists somewhere on the system. If it is part of a larger project, that project is required to build an iGeom interface to link to, which is why I decided to hardcode this "library" as iGeom but really in this case it is target dependency that should exist as part of the plugin when the mcnp2cad library is being linked.

There may be better ways to do this, however. I know CMake has an ExternalProject module that can handle this as well, but I kind of liked the way we were doing it.

@gonuke
Copy link
Member

gonuke commented Nov 7, 2018

Perhaps it would be better if we used some other trigger. For example, is it possible to have IGEOM_LIB set (or unset) before we enter this file. If it is already set, we can skip the FIND_PACKAGE? Or something similar?

@pshriwise
Copy link
Member Author

I like that better, yeah. Leaves more flexibility for the library name. Updating..

I'll submit another PR to dagmc-trelis setting the IGEOM_LIB variable before configuring the mcnp2cad subdirectory.

@gonuke
Copy link
Member

gonuke commented Nov 7, 2018

This definitely seems better - makes me wonder, now, whether we should treat IGEOM_INCLUDE_DIR within this, as well?

@pshriwise
Copy link
Member Author

pshriwise commented Nov 7, 2018

Good point. I'm not sure about that... are there clearly defined rules on how those will be found for an externally built iGeom library? Should we be looking at CGM to understand that?

@gonuke gonuke self-requested a review November 19, 2018 02:31
@gonuke
Copy link
Member

gonuke commented Nov 19, 2018

I'm happy with this for now, we can improve other parts more in the future.
Thanks @pshriwise

@gonuke gonuke merged commit 13fd5e0 into svalinn:modular Nov 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants