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

CSM_share: no cime/Tools/Makefile #6025

Merged
merged 7 commits into from
Oct 27, 2023
Merged

Conversation

jgfouca
Copy link
Member

@jgfouca jgfouca commented Oct 26, 2023

The CSM_share build is very similar to how we build our main components, except it was being driven by the CIME/Tools/Makefile. This PR changes it to use our CMake build_model function. A few minor tweaks were needed, the main problem was with the Depends files. The CSM_share build cannot use our standard Depends system due to the likelihood that the Depends file will modify flags for files that CSM doesn't know about, which is currently an error. Instead, the CSM depends files are moved to E3SM/share, which is a bit unfortunate since it is not consistent and also makes them non-customizable within a case. On the plus side, this allows us to get rid of our last remaining Makefile-style Depends files.

[BFB]

@jgfouca jgfouca requested review from rljacob and bartgol October 26, 2023 19:21
@jgfouca jgfouca self-assigned this Oct 26, 2023
@github-actions
Copy link

github-actions bot commented Oct 26, 2023

PR Preview Action v1.4.4
🚀 Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6025/
on branch gh-pages at 2023-10-26 21:43 UTC

@rljacob
Copy link
Member

rljacob commented Oct 26, 2023

Looks like there's also a CIME submodule change. Intended?

@jgfouca
Copy link
Member Author

jgfouca commented Oct 26, 2023

@rljacob , good catch. That was not intended.

jgfouca added a commit that referenced this pull request Oct 26, 2023
CSM_share: no cime/Tools/Makefile

The CSM_share build is very similar to how we build our main
components, except it was being driven by the
CIME/Tools/Makefile. This PR changes it to use our CMake build_model
function. A few minor tweaks were needed, the main problem was with
the Depends files. The CSM_share build cannot use our standard Depends
system due to the likelihood that the Depends file will modify flags
for files that CSM doesn't know about, which is currently an
error. Instead, the CSM depends files are moved to E3SM/share, which
is a bit unfortunate since it is not consistent and also makes them
non-customizable within a case. On the plus side, this allows us to
get rid of our last remaining Makefile-style Depends files.

[BFB]
@jgfouca jgfouca added the BFB PR leaves answers BFB label Oct 26, 2023
@bartgol
Copy link
Contributor

bartgol commented Oct 26, 2023

Wouldn't it be easier to make the CMakeLists.txt in <root>/share do its own thing, rather than bend build_model.cmake? I don't think there are lots of src files in the csm_share lib, so we could prob do things a bit more tidy? E.g., we could list the files instead of globbing, and do away with buildconf dir and filepath; we could set flags with generator expressions (taking care of compiler and language, no need of Depends files), do away with the sourcemods folder, and maybe more. The "generic" build_model.cmake has to accommodate so many use cases and scenarios, that is hard to navigate for new comers. I think a slim, include-what-you-use kind of CMakeLists.txt may be easier to maintain?

It seems to me that by using build_model we're trying to use a cannon to kill a fly.

@jgfouca jgfouca mentioned this pull request Oct 27, 2023
49 tasks
@jgfouca
Copy link
Member Author

jgfouca commented Oct 27, 2023

@bartgol , that's an interesting thought. I think the current approach is probably the path of least resistance in that it gets us the exact behavior we had before (build_model.cmake was originally designed to mimic CIME/Tools/Makefile) with minimal additional lines of CMake. build_model is definitely overkill for csm_share (I'm not 100% sure no one is using sourcemods for these files though) but it has the advantage of code reuse.

Your ideas are something that could be interesting for V3 of the build system. As a side note, V3 will probably be the hardest upgrade yet because it will involve diving in to those massive perl files to pull out things that CMake should be handling.

Copy link
Contributor

@bartgol bartgol left a comment

Choose a reason for hiding this comment

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

I agree this is the path of least resistance, so I'm going to approve. For what is worth though, I am in favor of doing the V3 effort. My reasoning is that yes, using CMake over Makefiles has the advantage of being more readable for new devs/users (less ppl fiddle with Makefile's these days), but if the underlying logic and (cmake) code flow is still the same (the Filepath trick to retrieve the src files is a great example), it remains hard to digest/read for those not accustomed to it.

@jgfouca
Copy link
Member Author

jgfouca commented Oct 27, 2023

@bartgol , thanks. V3 will come someday, hopefully a long time from now :)

Those perl eam configure files are the stuff of nightmares.

@rljacob
Copy link
Member

rljacob commented Oct 27, 2023

We still want to see those perl configs completely replaced with python (minus any cmake parts). No one seems motivated to do it.

@jgfouca
Copy link
Member Author

jgfouca commented Oct 27, 2023

@rljacob , that would be a nice thing for E3SM. It will be a huge job:

% wc -l build-namelist configure perl5lib/Build/*
  5369 build-namelist
  3282 configure
   174 perl5lib/Build/ChemNamelist.pm
   590 perl5lib/Build/ChemPreprocess.pm
   437 perl5lib/Build/Config.pm
   337 perl5lib/Build/NamelistDefaults.pm
   665 perl5lib/Build/NamelistDefinition.pm
   953 perl5lib/Build/Namelist.pm
 11807 total

So that's nearly 12,000 lines of perl just for eam. My hunch is that there's a huge amount of copy/paste code design going on, so a well-implemented python port should be far smaller, but that's still a lot of perl to sift through.

Maybe I will get around to it some day 🤷‍♂️

@jgfouca jgfouca merged commit b2ea1c3 into master Oct 27, 2023
2 checks passed
@jgfouca jgfouca deleted the jgfouca/csm_share_no_makefile branch October 27, 2023 16:06
@bartgol
Copy link
Contributor

bartgol commented Oct 27, 2023

@rljacob , that would be a nice thing for E3SM. It will be a huge job:

% wc -l build-namelist configure perl5lib/Build/*
  5369 build-namelist
  3282 configure
   174 perl5lib/Build/ChemNamelist.pm
   590 perl5lib/Build/ChemPreprocess.pm
   437 perl5lib/Build/Config.pm
   337 perl5lib/Build/NamelistDefaults.pm
   665 perl5lib/Build/NamelistDefinition.pm
   953 perl5lib/Build/Namelist.pm
 11807 total

So that's nearly 12,000 lines of perl just for eam. My hunch is that there's a huge amount of copy/paste code design going on, so a well-implemented python port should be far smaller, but that's still a lot of perl to sift through.

Maybe I will get around to it some day 🤷‍♂️

Is the plan for v4 to replace eam with eamxx? Or are both of them still going to be there. If eam is going away, then we can allow eam to maintain its logic, and just replace perl with cmake/python only for long-term components...

@rljacob
Copy link
Member

rljacob commented Oct 27, 2023

ELM also has a long perl configure.

@bartgol
Copy link
Contributor

bartgol commented Oct 27, 2023

Ok, but I would take 1 long perl over 2 long perls :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFB PR leaves answers BFB CMake build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants