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

Add AMReX as a git submodule #762

Merged
merged 11 commits into from
Feb 12, 2020
Merged

Add AMReX as a git submodule #762

merged 11 commits into from
Feb 12, 2020

Conversation

maxpkatz
Copy link
Member

PR summary

With #760, this completes #651 by adding AMReX as a git submodule of Castro.

PR checklist

  • test suite needs to be run on this PR
  • this PR will change answers in the test suite
  • all functions have docstrings as per the coding conventions
  • the CHANGES file has been updated
  • if appropriate, this change is described in the docs

@adam-m-jcbs adam-m-jcbs self-assigned this Feb 10, 2020
@adam-m-jcbs
Copy link
Member

Need any help with this? I've used submodules (and subtrees) pretty extensively in my repos.

@maxpkatz
Copy link
Member Author

Fortunately this change is fairly simple, though if you want to test it out to make sure I didn't miss anything, that's appreciated.

A question though -- in the interests of long term stability, should I create a subdirectory where we start putting dependencies (for now, just AMReX and Microphysics, but later potentially other things like pynucastro, Hypre, etc.)? If so what should it be called? "external", perhaps?

@zingale
Copy link
Member

zingale commented Feb 10, 2020

I like external/

@maxpkatz
Copy link
Member Author

Also, is it correct that once you have downloaded the submodule(s), git pull --recurse-submodules is all that is needed to keep the full repo (and submodules) up to date? Or is there some additional step needed to actually update the submodules to the latest commit after the pull?

@adam-m-jcbs
Copy link
Member

external/ is not bad. My preference would be submodules/, but that may be less clear to new users. It may become inaccurate, too, if we root things there other than submodules eventually.

@maxpkatz maxpkatz marked this pull request as ready for review February 12, 2020 14:32
@zingale zingale merged commit d14bf5a into development Feb 12, 2020
@zingale zingale deleted the amrex branch February 12, 2020 19:04
@adam-m-jcbs
Copy link
Member

adam-m-jcbs commented Feb 13, 2020

@maxpkatz apologies for the delay in answering, but re: git pull --recurse-submodules

Both git clone --recurse-submodules and git pull --recurse-submodules I believe can achieve what you want. Ultimately, what we care about is what happens when you do git submodule update. The behavior for this should be configured by us in .gitmodules in the superproject (Castro).

The main relevant documentation is man git-submodules and man gitmodules (for the .gitmodules config file). Changing .gitmodules will impact what happens when we do git pull --recurse-submodules. By default, a checkout is performed. This can be nice for stability, because it checks out the specific commit registered in the superproject. However, this means it will not pull the current head of development or similar. Instead, submodules using the checkout update strategy will ALWAYS point to the specific commit referenced in the superproject (when you cd into the submodule, you'll find it in a detached head state). Until you update the superproject, it will keep going back to that commit.

We may want to instead use the rebase strategy. In this case, the current branch of the submodule will be rebased onto the commit recorded in the superproject. The nice part of this is that it keeps AMReX even with the current version of development or whichever branch the submodule is on. The bad part is it may rebase things that break within the superproject but didn't break when they ran tests on AMReX. This is a theoretical possibility, but maybe our development and merging practices are good enough this isn't a worry?

Ultimately, we should encourage people to use a "pull" model with submodules. It is WAY easier to manage only pulling in the latest tested release of submodules into a superproject than trying to let people push changes to submodules from within the superproject. Technically, yes, you could modify AMReX within Castro and push those changes to AMReX. In practice, this is dangerous and error-prone. It shouldn't be done by people that aren't at least intermediate experts in git and probably even in git submodules.

I hope that helps.

@adam-m-jcbs
Copy link
Member

I did a quick check of our .gitmodules. To the best of my knowledge, it implies we're currently using the checkout submodule update strategy, as I think this is the default and we do not specify an update strategy for our submodules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants