-
Notifications
You must be signed in to change notification settings - Fork 31
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
Mac build #73
Mac build #73
Conversation
when compiling on Mac OS with non-standalone mode I got:
which is solved by defining |
@gonuke @pshriwise @ljacobson64 this should be ready to be reviewed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of little questions/changes and then happy to merge
version.hpp.in
Outdated
#define MCNP2CAD_VERSION_MINOR @MCNP2CAD_VERSION_MINOR@ | ||
#define MCNP2CAD_VERSION_REV @MCNP2CAD_VERSION_PATCH@ | ||
|
||
#define STANDALONE_MCNP2CAD '@STANDALONE_MCNP2CAD@' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for quotes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite remember.... I'll check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure...
I'll try without.
@@ -90,8 +85,10 @@ set(MCNP2CAD_PUB_HEADERS dataref.hpp | |||
mcnp2cad.hpp | |||
MCNPInput.hpp | |||
options.hpp | |||
version.hpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we still want this to be installed even though it is autogenerated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do at line 91
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think line 91 makes it available for the build, but doesn't install it for use by downstream applications, although I'm not 100% sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes you are probably right...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll commit an update soon.
if (CMAKE_SYSTEM_NAME MATCHES Linux AND CMAKE_COMPILER_IS_GNUCXX) | ||
if (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER "5.0") | ||
if (NOT CMAKE_CXX_FLAGS MATCHES _GLIBCXX_USE_CXX11_ABI) | ||
set(CMAKE_CXX_FLAGS "-D_GLIBCXX_USE_CXX11_ABI=0 ${CMAKE_CXX_FLAGS}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I am out of the loop, but how come these lines were removed? I remember them being necessary when I was building on Linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is still needed for Trelis 17.1.
And we now add it manually on the reps we build (MOAB, and DAGMC).
but as this need to be removed for Cubit 2020.2, we decided that we remove them and one specify them manually in the make process.
(@pshriwise I think we need to update the doc)
@gonuke I think I addressed all your comments. (@pshriwise @gonuke this is the next PR to get reviewed/merged in the Trellis-plugin GitHub Action chain.) |
In the absence of CI, I'll merge this for testing downstream |
this allow to build.on Mac OS
it also an autogenerated version.hpp file for the version numbers.