-
Notifications
You must be signed in to change notification settings - Fork 12
Flatten Aurora code hierarchy #34
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
base: main
Are you sure you want to change the base?
Conversation
|
I decided to also add most of the effort needed to flatten the code hierarchy, i.e. #32. A few items I could use some help with:
|
|
I'm not especially keen on the idea of adding MOOSE as a submodule. Not only will this potentially bloat the repository, but how does it scale when we consider the wider ecosystem? This application is designed to be interoperable with many other fusion-related MOOSE applications, and apply the same logic to each I would end up with multiple copies of MOOSE which is extremely undesirable. At best I then I have redundant copies of what is already a very large repository, and at worse the linker gets confused because I might have conflicting versions installed. The same reasoning applies to other co-dependencies. Alternatively AURORA gets treated differently to the other fusion apps - but to me this seems rather strange, I'd rather have e.g. AURORA, Achlys, Apollo, be on equal footing. I can't see a compelling argument to include MOOSE as submodule. If it is a more streamlined user experience you are looking for, I've been developing installation scripts in #30 that should be highly configurable. |
This one-liner is much simpler than
|
|
But ultimately it's your call of course! :) One other advantage with submodules is that Aurora users would not have to be responsible for setting |
|
Sounds like a win to me ... |
|
I do understand how submodules work, by bloating I was imagining having many users with many applications each with their own copy of MOOSE and thus many different redundant installations (and all this blowing up a bit). Our sysadmins already don't like the fact we each individually install moose, and would prefer it if we just had single system-wide install). OK, but if we are arbitrarily deciding to sometimes not use submodules, it will still continue to be necessary to support having the build externally, so the user will still have to set MOOSE_DIR and so on. New users won't need to write their own build scripts, the new build script I've written is completely configurable with command lines arguments. I still don't quite get the argument about what is to be gained, surely I will still need curate an understanding of compatible versions, whether that happens to be via a git submodule file or via versioned build scripts. |
|
I can remove the MOOSE submodule commit if you like. Just to address your points:
With submodules, you don't need the user to set MOOSE_DIR, but they still have the option for more advanced setups. I think right now, the user must set MOOSE_DIR, so it's an extra step (but please correct my understanding if this is not the case).
Yes, totally agree. IMO, the advantage of submodules is that it's very clear which MOOSE hash each App is compatible with. As long as the build scripts did something like then the scripts would have the same robustness as the submodule approach. So yes, the same effect could be achieved with the build scripts, but in terms of conformity with the rest of the MOOSE ecosystem, all other Apps use the submodule approach (to my knowledge). |
I think that might be best, at least until I've merged the new build scripts and updated docker files. If we change the build recipe we'll have to consolidate those anyway to pass CI. |
|
Sounds good! I have removed it. |
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.
Thanks for separating off the submodule discussion. I'm generally happy with the idea of flattening the structure and agree it'll make things less confusing. A few small changes on the makefiles. Otherwise, the main thing is to get the tests to pass: assuming everything still builds OK, I expect it's a question of just updating the dockerfiles.
- docker/aurora-fedora/Dockerfile
- docker/aurora-ubuntu/Dockerfile
- README.md (user instructions for build)
- We'll need to regenerate the html docs as well (though I'd be happy to make this merge seperately).
| CHEMICAL_REACTIONS := no | ||
| CONTACT := no | ||
| EXTERNAL_PETSC_SOLVER := no | ||
| FLUID_PROPERTIES := no | ||
| FUNCTIONAL_EXPANSION_TOOLS := no | ||
| HEAT_CONDUCTION := yes | ||
| LEVEL_SET := no | ||
| MISC := yes | ||
| NAVIER_STOKES := no | ||
| PHASE_FIELD := no | ||
| POROUS_FLOW := no | ||
| RDG := no | ||
| RICHARDS := no | ||
| SOLID_MECHANICS := no | ||
| STOCHASTIC_TOOLS := no | ||
| TENSOR_MECHANICS := yes | ||
| XFEM := no |
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 we could leave the extra modules in the makefile? Someone might want to enable them for their own purposes.
| AURORA_DIR := $(abspath $(dir $(word $(words $(MAKEFILE_LIST)),$(MAKEFILE_LIST)))) | ||
| CONTRIB_DIR := $(AURORA_DIR)/contrib | ||
| MOOSE_SUBMODULE ?= $(CONTRIB_DIR)/moose |
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.
If we're not doing the submodule thing I don't think we need these lines?
| CHEMICAL_REACTIONS := no | ||
| CONTACT := no | ||
| FLUID_PROPERTIES := no | ||
| HEAT_CONDUCTION := yes | ||
| MISC := no | ||
| NAVIER_STOKES := no | ||
| PHASE_FIELD := no | ||
| RDG := no | ||
| RICHARDS := no | ||
| SOLID_MECHANICS := no | ||
| STOCHASTIC_TOOLS := no | ||
| MISC := yes | ||
| TENSOR_MECHANICS := yes | ||
| XFEM := no | ||
| POROUS_FLOW := no | ||
| LEVEL_SET := no |
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.
Prefer to keep these options for now.
|
Ok a few more comments on these things
Probably not(?) I think it could probably go. But I like the name AuroraApp better than OpenMCApp, so maybe the latter is the one to get rid of?
I wrote every test to fulfil a function, so yes they need to stay. No doubt some will now fail after these changes, we'll need to fix them.
I think we have:
|

Adds MOOSE as a submodule in the
contribdirectory.@helen-brooks if you like this idea, I can then add all the other dependencies as submodules as well.