Skip to content

Check submodule hash consistency#229

Merged
alperaltuntas merged 7 commits intomainfrom
add_git_diff_test
Feb 10, 2025
Merged

Check submodule hash consistency#229
alperaltuntas merged 7 commits intomainfrom
add_git_diff_test

Conversation

@alperaltuntas
Copy link
Copy Markdown
Member

@alperaltuntas alperaltuntas commented Feb 8, 2025

This PR add a new step in CI test to check if gitmodules and externals are inconsistent by simply running git diff.

run: |
echo "Checking if .gitmodules and external hashes are consistent"
cd $GITHUB_WORKSPACE/CESM/components/mom/
../../bin/git-fleximod update
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alper - you can also do ../../bin/git-fleximod test at this point. Instead of update and then git diff.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran the following in a clean checkout of the MOM_interface tag used in beta05:

$ git submodule update --init
Submodule 'MOM6' (https://github.com/NCAR/MOM6.git) registered for path 'MOM6'
Submodule 'MARBL' (https://github.com/marbl-ecosys/MARBL.git) registered for path 'externals/MARBL'
Submodule 'stochastic_physics' (https://github.com/ESCOMP/stochastic_physics.git) registered for path 'externals/stochastic_physics'
Cloning into '/glade/work/mlevy/codes/mi_tmp/MOM6'...
Cloning into '/glade/work/mlevy/codes/mi_tmp/externals/MARBL'...
Cloning into '/glade/work/mlevy/codes/mi_tmp/externals/stochastic_physics'...
Submodule path 'MOM6': checked out '8feb16278d3dbeb22e4902257d2dcf6560a49e06'
Submodule path 'externals/MARBL': checked out '2c04fb23d0ee9ceef6d61f1021652ccab62e8324'
Submodule path 'externals/stochastic_physics': checked out 'ba1dc9d73da5ede3927fd3cdcc38ab2db0a212e5'
$ /path/to/git-fleximod test
s                 MOM6 dev/ncar_250128 8feb16278 is out of sync with .gitmodules dev/ncar_241122b
          e        pkg/CVMix-src has no fxtag defined in .gitmodules
          e      pkg/GSW-Fortran has no fxtag defined in .gitmodules
    stochastic_physics at tag ocn_skeb_240807
                 MARBL at tag marbl0.48.2

and that returns with a non-zero exit code. So there seem to be two options for testing:

  1. use git-fleximod update to checkout the submodules, and then do a git diff to make sure the submodules are consistent with what git is expecting
  2. use git to checkout submodules, and then use git-fleximod test to check if they match what fleximod expects.

I don't have a preference for one method over the other

echo "Checking if .gitmodules and external hashes are consistent"
cd $GITHUB_WORKSPACE/CESM/components/mom/
../../bin/git-fleximod update
git diff --exit-code
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talking with @jedwards4b, it sounds like ../../bin/git-fleximod test would be better than my proposed git diff --exit-code here. Though I'm not sure how it works, because running it in my sandbox I see

$ ../../bin/git-fleximod test
                  MOM6 at tag dev/ncar_241122b
          e        pkg/CVMix-src has no fxtag defined in .gitmodules, module at 87c3c0c
          e      pkg/GSW-Fortran has no fxtag defined in .gitmodules, module at 29e64d6
    stochastic_physics at tag ocn_skeb_240807
                 MARBL at tag marbl0.48.2

dev/ncar_241122b is the right tag from git-fleximod's perspective, but it's a problem because it's not what git submodules expects

@alperaltuntas
Copy link
Copy Markdown
Member Author

@jedwards4b, @mnlevy1981 Does the current version look good?

@jedwards4b
Copy link
Copy Markdown
Contributor

Yes, thanks!

Copy link
Copy Markdown
Collaborator

@mnlevy1981 mnlevy1981 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I do want to talk about this type of test at tomorrow's CSEG meeting, and if that meeting leads to a different solution for testing then we might need a follow-up PR... but I think we should merge this in now and deal with updates in the future if needed

@alperaltuntas alperaltuntas merged commit 84cfd0b into main Feb 10, 2025
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