-
Notifications
You must be signed in to change notification settings - Fork 116
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
Excise supporting software #220
Conversation
This commit adds bionic support, and adds system packages for Judy, spatialindex, and spud
This commit removes build system configuration that assumes local copies of supporting software and adds in support for system versions.
5a69594
to
d5e3550
Compare
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.
A couple of minor points regarding your changes to the autotools configure. There's still the wider question of how to ensure the supply of the supporting software. In a void, my preference would be something along the lines of the PETSc/ VTK super build approach of a second download, which is only built if required/requested.
else | ||
LIBS="$LIBS $ac_cv_search_spud_get_option" | ||
fi | ||
|
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.
Replacing this with
OLDLIBS=$LIBS
AC_SEARCH_LIBS([spud_get_option], [spud])
if test "$ac_cv_search_spud_get_option" = "none required" ; then
AC_MSG_ERROR("Unexpected error when checking for libspud")
elif test "$ac_cv_search_spud_get_option" = "no" ; then
AC_MSG_FAILURE("libspud not found")
else
LIBS="$OLDLIBS -Wl,-Bstatic $ac_cv_search_spud_get_option -Wl,-Bdynamic"
fi
will solve the failing test, although this is very specific to the gnu ld.
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 suspect we can get away with that, so will implement for now but with an openness to other options!
Regarding how we deal with ensuring the supply of supporting software, I'd suggest the train of thought that goes 'we already require quite a lot of external builds such as petsc and zoltan; should spatialindex, judy, and spud be treated differently because they have historically been bundled into Fluidity?'. |
This seems to be a shelved concept - it's not a major issue to keep packages periodically updated within Fluidity, and is useful to have them wrapped in for HPC installs. Closing for now but happy for others to reopen if there's interest. |
This PR removes libspud, spatialindex, and libjudy from master, picking up on half of PR #217.
Question for discussion, following on from PR #217 -- is this the route we want to go down for mashis ter? It could make life more difficult for people on esoteric systems where the removed software can't be picked up from packages, but has the benefit of making it simpler to add in CMake support and set up out-of-tree builds as per Issue #219.
This still has the issue with spud libraries causing a failure in python_diagnostic_field as @jrper discussed in PR #217. I've had a couple of goes at fixing this neatly without success so would also be glad of feedback on the best way forward there. I'd note this is failing from the autoconfigured build, not CMake.