-
Notifications
You must be signed in to change notification settings - Fork 338
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
Cmake system for MPAS #386
Cmake system for MPAS #386
Conversation
This will need to be merged before: |
@jgfouca, maybe you've already discussed this with the MPAS team and I'm just not in the loop. My understanding is that we don't make pull requests directly into @mark-petersen, @jonbob and others, please correct me if I'm wrong. Also, this is something we may need to discuss at our MPAS developers' meeting on Monday. |
@xylar , there's only one mpas submodule now, right?
How would I have different branches for the various cores from E3SM's point of view? |
Nevermind, I see what you mean. |
@jgfouca - there are machinations about how we pull that single submodule together that require this to be three MPAS PR's under-the-covers, and then a single one into E3SM |
@jonbob, unfortunately, I think it will be 4 PRs, since we need one into develop as well (probably before the ones in the individual cores). @jgfouca, yep, we'll take care of this on our side so you don't have to worry about the details. I just wanted to warn you that it'll be a little bit of a process. |
Whoops, looks like there's an issue with per-component flags with MPAS. I'll let you know when it's fixed. |
@jgfouca Would you be available this coming Monday, 4 November at 11:00 MT? We'll be having a developers's telecon, and one of the items to discuss is an interest in a CMake build for MPAS-Atmosphere as well. It would be good to see where our efforts might complement or conflict with each other. |
@mgduda , yes, I'm available. Can you email me an invite? |
1deefd4
to
8271b0b
Compare
@jgfouca thanks for your work on this. I changed the base branch for this PR from @xylar, in this case I think it's OK to have a single PR for all brand-new files into each core's directory. After this is merged we will merge develop back into each core, which is simpler than 4 separate PRs. |
@xylar ,
Yeah, I definitely don't want to conflict with existing CMake efforts. My understanding is that no such effort/plan for MPAS exists; maybe that's changed since summer. If MPAS does want a CMake build system, then it should be pretty easy to leverage the work I've done to do the builds of the cores and framework, so you guys would only need to set up the root CMakeLists.txt file. |
Notes from telecon today:
|
@mark-petersen , I've added a couple commits addressing the topics in the telecon. In my opinion, this is ready to be merged. One thing I should point out is that this branch stopped working after you rebased it. The rebase point differs significantly from what the submodule was set to and it no longer works with E3SM. I had to continue development from the original branch and I cherry-picked the additional commits onto this branch. I'm totally unfamiliar with MPAS' development workflow, so maybe this expected. |
@jgfouca thanks for your work on this. Yes, the MPAS workflow is confusing. This PR will be merged into MPAS-Dev/develop, and that into MPAS-Dev/e3sm/develop, which ends up in mpas-source in e3sm. Your cherry-picking is fine. If you want to further test this PR in e3sm you can cd into components/mpas-source, add your fork, merge in this PR in locally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on visual inspection, @jgfouca fulfilled our request of keeping file information in each subdirectory within src
. When we get approval from a few others on the list, I'll go ahead and merge.
Thanks, @mark-petersen ! I hope the other reviewers can look at it soon because the all-hands is coming up fast! |
@mgduda @matthewhoffman @akturner @jonbob please review as soon as you can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also approving by visual inspection. The landice core-specific stuff has been moved to the landice dir, so that's good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve by visual inspection
@mgduda could you approve, or tell us of any outstanding concerns? We are working towards a deadline on this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this PR have no impact on builds of MPAS-Atmosphere cores using the stand-alone build system. However, as discussed on the 4 November telecon, we will need to update the CMakeLists.txt
files to accommodate stand-alone builds of MPAS-Atmosphere cores in future.
This PR switches E3SM's default build-system to be cmake-based. It does not alter the current standard MPAS make system.
@mark-petersen , many thanks for pushing this along. Is there another PR I can follow for the updating of the MPAS submodule in E3SM? |
I can make a single E3SM PR right now with the mpas-source update, or we can update mpas-source in E3SM-Project/E3SM#3043. What is your preference? |
If the later option takes longer, the first might be faster and more direct. |
@mark-petersen , I think the best option is to update via a fresh E3SM PR. I'm still not entirely certain how quickly 3043 will be able to make it into master, and based on git history, MPAS submodule is updated frequently, so updating it only in 3043 could be a source of conflicts. |
This PR switches E3SM's default build-system to be cmake-based. It does not alter the current standard MPAS make system.
This PR switches E3SM's default build-system to be cmake-based. It does not alter the current standard MPAS make system.