Rewrite CMake for robustness as both standalone library and part of Trelis plugin #68

ljacobson64 · 2019-12-20T22:27:11Z

This is a companion PR to svalinn/Cubit-plugin#56.

… the Trelis plugin and also as a standalone program has become more robust.

gonuke

I haven't tested this, but it looks like there are some missing pieces in the new version?

gonuke · 2019-12-23T20:25:36Z

CMakeLists.txt

-    add_subdirectory(${CMAKE_SOURCE_DIR}/cli)
-endif()
-
-find_package(Eigen3 3.3 REQUIRED NO_MODULE)


Don't we still need to find Eigen3

Not as far as I can tell. It finds it automatically for me. I could put it back in just in case though.

I looked into this some more. As far as I can tell, this line wasn't doing anything in the first place. You would need to add the line include_directories(${EIGEN3_INCLUDE_DIRS}) for it to do anything.

The reason it worked despite this is that Eigen3 is located in /usr/include, and you don't need to specify /usr/include as a included directory because it's already included by default.

Should we always assume that Eigen is installed in the include path?

I missed this -- I'm going to re-introduce this line

Adding this line back in gives an error saying that it can't be used until CMake 3.0, so this can't be added without removing support for CMake 2.8. I'm going to add the line anyway and change the required version since 3.0 is still relatively ancient

@gonuke if you find_package eigen3 on a system with system eigen3, it sets EIGEN3_INCLUDE_DIR to /usr/include/eigen3. However, this directory is not useful because when we include eigen in the c++ files, we use the line #include <eigen3/Eigen/Eigen>. Should these lines be changed to #include <Eigen/Eigen>?

That change is probably the right thing to do

Just pushed a commit that made this change. It added these lines:

find_package(Eigen3 3.3 REQUIRED) message(STATUS "EIGEN3_INCLUDE_DIR: ${EIGEN3_INCLUDE_DIR}") include_directories(${EIGEN3_INCLUDE_DIR})

edit: and it changed the #include lines in the cpp files

gonuke · 2019-12-23T20:26:38Z

CMakeLists.txt

+# IGEOM_DIR must be specified if mcnp2cad is being built as a standalone
+# library. If it is not specified, then CMake will assume that mcnp2cad is being
+# built as a part of a larger project which already knows the location of iGeom.
+if (IGEOM_DIR)


Should building the CLI be a trigger for standalone? Don't we need to find iGeom headers somewhere even when building in the plugin?

That's a good idea. We could have a -DSTANDALONE flag or something, and if it's standalone then it requires you to tell it where iGeom is and it builds the CLI automatically

I added an option for -DSTANDALONE_MCNP2CAD. It defaults to on, but it is turned off in the Trelis-plugin config. If it is on, then it expects -DIGEOM_DIR to be present and it builds the CLI.

gonuke · 2019-12-23T20:27:11Z

CMakeLists.txt

+                         volumes.hpp)
+
+# mcnp2cad library
+add_library(mcnp2cad SHARED ${MCNP2CAD_SRC_FILES})


How does this build find the iGeom libraries when part of the plugin?

Since this CMakeFile will be included via the add_subdirectory command, CMake should already know what is meant when the iGeom library is specified. It's no different than in DAGMC, when we build the dagmc library, and everywhere else in the project we only need to specify dagmc to link to that library.

I know this was one of the complications of the circular system... perhaps it's only the iGeom headers that need to be found when building as part of the plugin. This is were there is a gap between

the add_subdirectory implementation in which it is difficult to pass in the location of headerse

the build directly from the parent directory in which you can take full control but may diverge from this package's build configuration

It would be great if there was a clean CMake-approved way to pass information into this build using the add_subdirectory method

I'm not sure if I'm following what the problem is

I'll have to try it to articulate more clearly, but using the add_subdirectory method from the plugin will drop into this directory and run it's CMake. Building this library requires access to iGeom headers, but those headers are not installed anywhere and I am not sure if there is a way to alter the list of include directories for this project from the CMake instance that calls it.

When we build mcnp2cad as a part of the plugin, it knows where the iGeom headers are because we're also building iGeom as a part of the plugin. The top-level Trelis-plugin CMakeFile includes the line

include_directories(iGeom)

which means that all CMakeFiles in subdirectories already have the iGeom headers available.

OK - I'll just have to try it. I know there was something not working correctly at some point...

ljacobson64 · 2019-12-23T21:44:08Z

I just realized that I made this branch in in the svalinn repo instead of my own fork... didn't intend to do that. I don't think it can be changed though.

bam241

comments from Software review

CMakeLists.txt

cli/CMakeLists.txt

CMakeLists.txt

ljacobson64 · 2020-09-28T19:16:29Z

I just confirmed that it is possible to build mcnp2cad as a standalone library (along with the CLI executable) on top of CGM which is built on top of OpenCascade. No guarantees that it works though.

CMakeLists.txt

…ed iGeom lib

…arget_link_directories to specify locations of iGeom directories

pshriwise

I'm good with these updates now. Thanks for your diligence, @ljacobson64!

ljacobson64 · 2020-09-30T15:28:55Z

I just realized that the mcnp2cad readme now contains wrong information. I could take some time to update it, or we could just leave it and update it later.

pshriwise · 2020-09-30T15:30:21Z

I'm fine w/ making that a separate PR right after this one.

pshriwise · 2020-09-30T16:32:09Z

@gonuke when you have time can you take another look? I think we're close to getting these Trelis-Plugin/mcnp2cad issues resolved.

gonuke · 2020-10-03T02:10:04Z

I left a couple of final comments. The most pressing one is about ensuring we can find Eigen

gonuke

Forgot to submit review

gonuke · 2020-10-03T01:56:08Z

CMakeLists.txt

+add_library(mcnp2cad SHARED ${MCNP2CAD_SRC_FILES})
+target_link_libraries(mcnp2cad iGeom)
+if (STANDALONE_MCNP2CAD)
+  # Only install public headers if building standalone


What's the use case for installing headers if building standalone?. Or is this just in case?

There used to be some reason why I thought this was needed, but I don't think it's needed anymore. I can remove it if you want, although I don't think it's hurting anything.

Maybe they are necessary? In a world where a dev writes their own cli they'll need these.

gonuke

Thanks @ljacobson64

Rewrite CMakeFile. The behavior for building mcnp2cad both as part of…

08d783b

… the Trelis plugin and also as a standalone program has become more robust.

gonuke requested changes Dec 23, 2019

View reviewed changes

ljacobson64 added 3 commits December 23, 2019 16:28

Add STANDALONE_MCNP2CAD option

63b5335

Move CLI to bottom of CMake file so iGeom is found

9b7fc2c

bam241 reviewed Mar 6, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

cli/CMakeLists.txt Outdated Show resolved Hide resolved

bam241 reviewed Sep 4, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

pshriwise reviewed Sep 4, 2020

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

pshriwise mentioned this pull request Sep 4, 2020

Check to see if IGEOM_LIB target exists before searching. #69

Closed

ljacobson64 added 2 commits September 4, 2020 14:20

Address BAM comments

2950877

Add IGEOM_LIB variable

7949b7c

ljacobson64 mentioned this pull request Sep 5, 2020

Rewrite CMake and README, Add test problem svalinn/Cubit-plugin#56

Merged

ljacobson64 added 3 commits September 7, 2020 12:12

Remove IGEOM_LIB variable

e58bddc

Install convertMCNP to bin directory

5996f30

Create imported iGeom target for standalone mode

1226fef

pshriwise reviewed Sep 29, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

ljacobson64 added 4 commits September 29, 2020 11:29

Use target_include_directories and target_link_directories for import…

8dc1bc5

…ed iGeom lib

Remove unused line

23d3f78

Update authors

59cb56c

Use set_target_properties instead of target_include_directories and t…

977a0a7

…arget_link_directories to specify locations of iGeom directories

pshriwise approved these changes Sep 30, 2020

View reviewed changes

gonuke reviewed Oct 3, 2020

View reviewed changes

Use cmake to find eigen3

4f5908c

gonuke approved these changes Oct 3, 2020

View reviewed changes

gonuke merged commit 595874c into master Oct 3, 2020

ljacobson64 deleted the fix_build branch October 3, 2020 17:21

pshriwise mentioned this pull request Nov 30, 2020

Adding check for standalone project. #63

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite CMake for robustness as both standalone library and part of Trelis plugin #68

Rewrite CMake for robustness as both standalone library and part of Trelis plugin #68

ljacobson64 commented Dec 20, 2019

gonuke left a comment

gonuke Dec 23, 2019

ljacobson64 Dec 23, 2019

ljacobson64 Dec 23, 2019

gonuke Oct 3, 2020

ljacobson64 Oct 3, 2020

ljacobson64 Oct 3, 2020

ljacobson64 Oct 3, 2020

gonuke Oct 3, 2020

ljacobson64 Oct 3, 2020 •

edited

Loading

gonuke Dec 23, 2019

ljacobson64 Dec 23, 2019

ljacobson64 Dec 23, 2019

gonuke Dec 23, 2019

ljacobson64 Dec 23, 2019

gonuke Dec 23, 2019

ljacobson64 Dec 23, 2019

gonuke Dec 23, 2019

ljacobson64 Dec 23, 2019 •

edited

Loading

gonuke Dec 23, 2019

ljacobson64 commented Dec 23, 2019

bam241 left a comment

ljacobson64 commented Sep 28, 2020

pshriwise left a comment

ljacobson64 commented Sep 30, 2020

pshriwise commented Sep 30, 2020

pshriwise commented Sep 30, 2020

gonuke commented Oct 3, 2020

gonuke left a comment

gonuke Oct 3, 2020

ljacobson64 Oct 3, 2020

gonuke Oct 3, 2020

gonuke left a comment

Rewrite CMake for robustness as both standalone library and part of Trelis plugin #68

Rewrite CMake for robustness as both standalone library and part of Trelis plugin #68

Conversation

ljacobson64 commented Dec 20, 2019

gonuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljacobson64 Oct 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljacobson64 Dec 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljacobson64 commented Dec 23, 2019

bam241 left a comment

Choose a reason for hiding this comment

ljacobson64 commented Sep 28, 2020

pshriwise left a comment

Choose a reason for hiding this comment

ljacobson64 commented Sep 30, 2020

pshriwise commented Sep 30, 2020

pshriwise commented Sep 30, 2020

gonuke commented Oct 3, 2020

gonuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

ljacobson64 Oct 3, 2020 •

edited

Loading

ljacobson64 Dec 23, 2019 •

edited

Loading