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

Excise supporting software #220

Closed
wants to merge 6 commits into from
Closed

Conversation

tmbgreaves
Copy link
Contributor

@tmbgreaves tmbgreaves commented Jan 31, 2019

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.

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.
@tmbgreaves tmbgreaves force-pushed the excise_supporting_software branch from 5a69594 to d5e3550 Compare January 31, 2019 16:51
@tmbgreaves tmbgreaves requested review from jrper and stephankramer and removed request for jrper January 31, 2019 16:52
@tmbgreaves tmbgreaves self-assigned this Jan 31, 2019
Copy link
Contributor

@jrper jrper left a 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.

configure.in Outdated Show resolved Hide resolved
configure.in Show resolved Hide resolved
else
LIBS="$LIBS $ac_cv_search_spud_get_option"
fi

Copy link
Contributor

@jrper jrper Feb 1, 2019

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.

Copy link
Contributor Author

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!

@tmbgreaves
Copy link
Contributor Author

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?'.

@tmbgreaves
Copy link
Contributor Author

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.

@tmbgreaves tmbgreaves closed this Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants