search

amanzi / amanzi

Amanzi primary repository
Other
48 stars 37 forks source link

bootstrap gets the wrong ATS branch #686

Open ecoon opened 2 years ago

ecoon commented 2 years ago

There is a bug in bootstrap.sh where we call:

git submodule update --init --recursive --remote src/physics/ats

to get the ATS.

This is incorrect -- there should be no "--remote" flag here I think. That goes to the origin and is supposed to do something with the "remotely tracked branch" but it doesn't seem to work with our current workflow, and now somehow means that bootstrap ALWAYS checks out the ATS commit that the origin (github)'s MASTER branch is pointing to.

What SHOULD happen is that each Amanzi branch carries its own commit ID, and when you check out the submodule, you should get that commit. You shouldn't ALSO have to specify an ATS branch, or an ats-regression-tests branch, or anything else.

We've worked around this bug in a few ways:

1) the bootstrap argument "--ats_branch" is used to checkout the HEAD of a different branch. If you use this argument, you won't see the bug. 2) the docker scripts have all sorts of extra steps to pass ats branches and ats-regression-test branches around to manually check those branches out. I suspect this was David working around the fact that the wrong branch is checked out by default. 3) the build_ATS_generic.sh forces the user to set an ATS_VERSION that will be passed to ats_branch, thereby working around the bug.

I'm going to remove this flag in 1.3 and master. Hopefully this means that we can simplify some of the (ATS) dockerfile logic as well. I'm not sure why we haven't seen more users struggle with this in installing older (release) branches of the code, other than possibly if everyone is using the build_ATS_generic.sh script.

ecoon commented 2 years ago

I'm a bit uncertain on whether to change the docker script.

There are two possible arguments:

  1. The current file will always check out and pull, getting the tip of the {ats,ats-regression-tests} branches. That's good if your goal is to support developers, but it hides the fact that you may not have updated the ATS pointer -- this would break a user's experience.
  2. Alternatively we could not check out and pull, and rely on the ATS/ats-regression-tests submodule pointers. This would be good to confirm that the user will get the right thing when they try and build the branch.

For now, I think it is probably better to support 1?

ecoon commented 2 years ago

@jd-moulton What is the usage difference between Docker/Dockerfile-ATS and Docker/Dockerfile-ATS-build?

jd-moulton commented 2 years ago

@ecoon Dockerfile-ATS was what we started with and was intended to a little better (a little smaller file) for building docker containers we deploy, and it runs the tests. As it stands the removal of the build directory is commented out. The Dockerfile-ATS-build, just builds, and it does not remove the build before pushing up to dockerhub. So this means that the docker image is available for developers even when the tests fail. As docker has improved there maybe a cleaner way to do this, I agree it's pretty crufty right now.

jd-moulton commented 2 years ago

I think the docker scripts are ok for now, as you note the intent is to try to support developers managing multiple repos and they are currently able to support whatever combination they want. When it comes to releases though, we definitely want to test the pointers, and so it could be that we need to get a bit smarter about this part, but is tricky. When a push arrives to ATS, all we know for sure is what ATS branch that is on, and the action has to figure out what Amanzi branch to pass to the Dockerfile, as well as whether or not to pass an ATS branch (or any of the other branches).

ecoon commented 2 years ago

At some point I wonder if it would be worth taking a crack at a post-commit action that updated the corresponding Amanzi branch automatically.

That said, better yet would be coming up with a way to reverse the dependency and let ATS point to Amanzi and yet still build as one unit. I don't want to go back to thinking of Amanzi as a TPL, but allowing ATS to point to a slightly older Amanzi commit would make ATS testing more robust.

jd-moulton commented 1 year ago

@ecoon The --remote option is still present on master. Did you want to remove this before we release.

There's good discussion here, and so we could keep this open as it's not fully resolved how best to robustly manage the hierarchy of repositories.