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

Remove manage externals #2443

Closed
wants to merge 8 commits into from

Conversation

jedwards4b
Copy link
Contributor

@jedwards4b jedwards4b commented Mar 28, 2024

Description of changes

This PR changes the source branch and continues discussion from #2303

Specific notes

Contributors other than yourself, if any:

CTSM Issues Fixed (include github issue #):

Are answers expected to change (and if so in what way)? No

Any User Interface Changes (namelist or namelist defaults changes)? No

Testing performed, if any:

@wwieder
Copy link
Contributor

wwieder commented Apr 3, 2024

Thanks for this, @jedwards4b. Is your PR ready for review now, or is this still a work in progress?

@jedwards4b
Copy link
Contributor Author

I've just created a new branch [email protected]:jedwards4b/ctsm.git rme/ctsm5.1.dev175
please give it a try and send me feedback

@wwieder
Copy link
Contributor

wwieder commented Apr 3, 2024

Thanks, I'm in meetings most of the rest of today. @samsrabin are you able to give this a test drive for Jim as some point?

@samsrabin
Copy link
Collaborator

Sure, either today or tomorrow.

@samsrabin samsrabin self-assigned this Apr 3, 2024
@wwieder wwieder added the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Apr 4, 2024
@samsrabin
Copy link
Collaborator

Jim, this works great—thanks! The one thing I noticed is that it automatically downloads doc-builder, whereas in Externals.cfg we had that as optional. Is this an intended behavior change?

@jedwards4b
Copy link
Contributor Author

Yes we can make the doc-builder optional.

@samsrabin
Copy link
Collaborator

I also tried adding this:

[submodule "representative-hillslopes"]
path = tools/external/representative-hillslopes
url = https://github.com/swensosc/Representative_Hillslopes
fxDONOTUSEurl = https://github.com/swensosc/Representative_Hillslopes
fxrequired = AlwaysOptional
fxtag = ee75e0a1cf0b582ab27f8b0aa3f0c5b475bdb0ca

And ran into an error:

$ ./bin/git-fleximod update representative-hillslopes
e representative-hillslopes not checked out, out of sync at tag None, expected tag is ee75e0a1cf0b582ab27f8b0aa3f0c5b475bdb0ca
root - ERROR - Failed to checkout representative-hillslopes False None /glade/u/home/samrabin/ctsm_fleximods_20240403/tools/external/representative-hillslopes tools/external/representative-hillslopes
Traceback (most recent call last):
  File "/glade/u/home/samrabin/ctsm_fleximods_20240403/./bin/git-fleximod", line 8, in <module>
    sys.exit(main())
  File "/glade/u/home/samrabin/ctsm_fleximods_20240403/.lib/git-fleximod/git_fleximod/git_fleximod.py", line 576, in main
    submodules_update(gitmodules, root_dir, fxrequired, force)
  File "/glade/u/home/samrabin/ctsm_fleximods_20240403/.lib/git-fleximod/git_fleximod/git_fleximod.py", line 382, in submodules_update
    single_submodule_checkout(
  File "/glade/u/home/samrabin/ctsm_fleximods_20240403/.lib/git-fleximod/git_fleximod/git_fleximod.py", line 251, in single_submodule_checkout
    utils.fatal_error(
  File "/glade/u/home/samrabin/ctsm_fleximods_20240403/.lib/git-fleximod/git_fleximod/utils.py", line 130, in fatal_error
    raise RuntimeError("{0}ERROR: {1}".format(os.linesep, message))
RuntimeError:
ERROR: Failed to checkout representative-hillslopes False None /glade/u/home/samrabin/ctsm_fleximods_20240403/tools/external/representative-hillslopes tools/external/representative-hillslopes

From looking at the Python for that error message, it looks like repodir tools/external/representative-hillslopes isn't getting created (i.e., repo_exists is false), but I don't know why.

@jedwards4b
Copy link
Contributor Author

Yes there is an extra step required to add a new repository. Let me think about it a bit, maybe there doesn't need to be.

@wwieder wwieder removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Apr 11, 2024
@samsrabin
Copy link
Collaborator

Yes there is an extra step required to add a new repository. Let me think about it a bit, maybe there doesn't need to be.

@jedwards4b, the discussion in the CSEG meeting reminded me about this. Any thoughts? I think if it's too much effort to rework things to skip the extra step, then it'd be fine to just improve the error message (and also any instructions?) to explain what one needs to do.

@jedwards4b
Copy link
Contributor Author

@samsrabin thank you for the reminder, i have opened a PR to git-fleximod that solves this problem. ESMCI/git-fleximod#36

eb932b0 Bump to 0.7.4
472e2fe Merge pull request ESCOMP#36 from ESMCI/fix/update_add
46a74ac create parent if it doesnt exist
5398ef4 if submodule does not exist add it

git-subtree-dir: .lib/git-fleximod
git-subtree-split: eb932b0
@jedwards4b
Copy link
Contributor Author

@samsrabin I've updated this branch with the latest git-fleximod which solves the issue.

@ekluzek ekluzek added enhancement new capability or improved behavior of existing capability code health improving internal code structure to make easier to maintain (sustainability) bfb bit-for-bit labels Apr 25, 2024
@samsrabin
Copy link
Collaborator

Thanks Jim! I just re-tested and it works great. One last thought: I'm not sure what the difference is between Toplevel and Always. Is there any documentation about this?

@jedwards4b
Copy link
Contributor Author

Yes this is documented here: https://github.com/ESMCI/git-fleximod?tab=readme-ov-file#supported-gitmodules-variables Toplevel means that this submodule is only considered if this module is at the toplevel of the clone, so files in ctsm's .gitmodules file labeled toplevel would be considered when you have cloned ctsm but not when you have cloned cesm.

@ekluzek
Copy link
Collaborator

ekluzek commented Apr 26, 2024

@jedwards4b there are some changes that we need to bring in associated with this. Would you prefer:

  • We open a PR to this one so you review?
  • We add changes directly to this PR?
  • We merge this into a different branch where we put those updates?

The main thing I'm thinking of is documentation in README type files, as well as running testing. Which you wouldn't need to review. If we do anything that changes any important details of your work, we'd do a PR to this one, to get your feedback.

@jedwards4b
Copy link
Contributor Author

My plan is to close this PR unmerged and open a new one based on the final tag you put in beta18. Does that meet your needs? I think that you can prepare updates to documentation now to be ready to merge to that PR when it's opened.

@ekluzek
Copy link
Collaborator

ekluzek commented Apr 26, 2024

@jedwards4b thanks for letting us know your plans. I think the only thing that needs to be done here is to to update .gitmodules based on our CTSM tag for cesm2_3_beta18 right?

If so I suggest keeping this one open, and we'll do that update. We need to get comfortable with updating that file anyway. Having us do that update would actually be good for us to be more confident in working with git fleximod. How does that sound?

@jedwards4b
Copy link
Contributor Author

Sure, that sounds fine.

@samsrabin
Copy link
Collaborator

Blocked while we wait for cesm2_3_beta18 to be done.

@samsrabin samsrabin added the blocked: dependency Wait to work on this until dependency is resolved label May 6, 2024
@samsrabin samsrabin removed their assignment May 9, 2024
@wwieder
Copy link
Contributor

wwieder commented May 9, 2024

This should also include #2536 for documentation.

@slevis-lmwg
Copy link
Contributor

@jedwards4b
close this PR as won't fix or similar?

@jedwards4b
Copy link
Contributor Author

This PR is superceeded by #2559

@jedwards4b jedwards4b closed this May 31, 2024
@ekluzek
Copy link
Collaborator

ekluzek commented Jun 6, 2024

FYI. As an exercise in learning about git-fleximod I took this branch and updated it to ctsm5.2.007 to make sure I could manage the process of handling submodule updates in the git-fleximod framework. I was able to do that and show that I ended up with the same thing as ctsm5.2.007 in the end. So that was a good way of familiarizing myself with the new process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bfb bit-for-bit blocked: dependency Wait to work on this until dependency is resolved code health improving internal code structure to make easier to maintain (sustainability) enhancement new capability or improved behavior of existing capability
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants