Rethink inclusion of the WorldBuilder via a git submodule

[bangerth]

It is not lost on me that the reason WorldBuilder was added to ASPECT via a git submodule (in #2571) is because I took (or had to take) a break from ASPECT development and didn't get to reviewing @MFraters's patches. That's my fault.

Be that as it may, we may want to rethink this structure. Submodules have the distinct disadvantage that (i) we need to update build instructions to include flags people don't typically use (--recursive), see #2587 , and (ii) that a git pull upstream master does not get the newest version of the WorldBuilder into a working tree. Both are annoying and confusing. The latter is also going to present a source of awkward cases where the WorldBuilder changes its interface, ASPECT is correspondingly adjusted, but someone just pulls from upstream and can't compile anymore because they forgot to also update the submodule.

For this reason, we've generally treated external projects as external to the tree. My preference would therefore be if, in the long run (before the next release), we could undo the current structure and either treat WorldBuilder as entirely external (like PETSc, Trilinos, ... in deal.II) or if we could import a fixed version that is updated periodically (like we do with BOOST or the TBB, for example).

What do others think?

[tjhei]

What do others think?

Let's see:

submodules:

• better separation (including history) (+)
• making it a separate project at some point is more natural (+)
• cloning/updating etc. is less obvious for users (-)
• potential dependency problems when not updating the submodule (-)
• other changes to release tasks/testing/etc. needed (-)

external project:

• no dependency problems (+)
• users might not install or able to use it (-)
• extra effort when installing (-)
• no testing unless we put in extra effort to set it up (-)

bundled:

• easy setup/usage/testing (+)
• no separation of history, copying over files often is not great (-)

I have to admit that I don't see a clear winner here. I think it depends a bit on the development speed of the WorldBuilder. If we don't have to update very often, the "bundled" approach looks best. On the other hand, if a lot of development happens, the other two options look better.

So, I am fine with either approach but I prefer a solution that allows for a solid testing infrastructure.

[gassmoeller]

I agree with Timo that I see not one solution that is uniformly better than the others. Long-term I am fine with all options, but would add that we were not very good at reviewing the large chunks of code that the world builder pull requests included in the past. @MFraters needs to be able to make fast progress on this since he is on a tight schedule, so the bundled version is at the moment out of the question (at least until he has finished his thesis). If we want to allow for easy testing we need to stick with the submodule for the moment.

[bangerth]

Yes, I agree that the solution was expedient in the short term, and apologize for not getting around to reviewing these pieces.

I was more thinking of how to deal with this in the long run.

[MFraters]

I do not have a strong preference between the submodule and bundled, but I do think that it is important that at least the coupling between the world builder and aspect can be tested inside aspect. So that is the reason why I think that an external project would not be a good idea.

For now I would be very grateful if it would remain a submodule, so that it is easy for me to test new versions of the world builder with aspect.

[MFraters]

We we want to resolve this issue before or after the release? It has my preference to do this after, so that we can discuss and fix this during the hackaton.

[tjhei]

I have it included in https://github.com/geodynamics/aspect/releases/tag/v2.1.0-rc1 at the moment. What I don't like is that we don't compile cleanly without warnings. :-( It doesn't matter if we go with submodule vs bundled, we have the same problem with the warnings (https://github.com/GeodynamicWorldBuilder/WorldBuilder/issues/118 ). One option would be to not include the world builder in the tarball, if we don't get things fixed?

[MFraters]

I was not aware that it generated warnings in aspect. I will see whether I can reproduce it and hopefully fix it.