Closed pshriwise closed 3 years ago
Maybe this was a better place to wonder about a meeting to discuss the build process improvement. @ljacobson64 already contributed #56 with a lot of changes (and a related one in mcnp2cad, I think)
Yeah after looking at those PR's I think we should have a meeting. I'm good with the majority of the changes in #56 and the related mcnp2cad
PR (@ljacobson64 is our CMake wizard after all).
My main issue is that handling of the current iGeom
-mcnp2cad
circular dependency that are more complicated than they need to be. If we rely on target information for iGeom
we don't need to re-discover the library and headers in mcnp2cad
when building the plugin. We can also establish the dependency needed to resolve this "chicken and egg" problem between iGeom
and mcnp2cad
.
Also some of the documentation in #56 should be added to for new discoveries about current (17.1+) SDKs.
I'm going to ask @bam241 to try and schedule a meeting on this to focus on the vision of how we think this should work. We discussed briefly at a software meeting, but didn't really have the right voices there.
This is now waiting on #56 to merge.
@pshriwise could you rebase now that #56 is in ?
I don't want to speak for Pat, but I'm pretty sure that everything that was in this PR was addressed by #56
The only thing here that isn't in #56 is the automation of the submodule checkout and update. It's there for convenience. Is it something we want to include?
Good call, we might want that. Or at least something that checks to see if mcnp2cad is there, and if it's not, to give an error message explaining how to fix it rather than just exiting
Maybe we shall update L129 to L136 from he README ? as mcnp2cad
is now pull from cmake...
this is working super smoothly :)
@bam241 are you suggesting we simply remove those lines?
yes I think they are not required (maybe add a mention that state that we are pulling it automatically...)
What happens if you're using a custom mcnp2cad for development purposes? Will the CMake complain that mcnp2cad's commit ID is wrong?
If there are any uncommitted modifications in the mcnp2cad repo, it will complain b/c git will refuse to discard those changes. Otherwise, it will automatically revert the mcnp2cad
repo to the commit stored as part of the trelis-plugin
repo.
Custom versions of the mcnp2cad
repo can still be used, however. It just needs to be updated using standard git submodule commands.
I'm honestly kind of hoping we don't have to update the mcnp2cad
repo very often. If we feel like it's going to be too big a hassle to do development with this in place, we don't have to add it.
thx @pshriwise !
Merging
Some updates to the CMake here:
Adding the
mcnp2cad
project as a submoduleThis is a pretty clean way to make sure we're getting the version of
mcnp2cad
we need when building this tool. I've added a CMake block that will update this submodule if the MCNP importer is enabled, meaning no additional git commands are needed when cloning the plugin.Place build if
iGeom
beforemcnp2cad
To successfully build the plugin with the mcnp importer enabled, a build with
BUILD_IGEOM=ON
andBUILD_MCNP_IMPORTER=OFF
must first be performed. Building theiGeom
target first (now also aliased asIGEOM_LIB
) allows us to provide this target tomcnp2cad
and perform the build in one step without modifications to the default CMake settings.Note: This PR is dependent on https://github.com/svalinn/mcnp2cad/pull/69