MPAS-Dev / MPAS-Model

Repository for MPAS models and shared framework releases.
231 stars 308 forks source link

Implement new Makefile logic for sharing framework builds across cores #1179

Closed mgduda closed 1 month ago

mgduda commented 1 month ago

This PR updates the top-level Makefile (and also the Makefile in src/) to enable reuse of existing builds of either the shared infrastructure or a core when all build options in the current build are compatible with those used to previously compile the infrastructure or core.

Previously, after compiling one core but before compiling another, an intermediate clean step was required (or the AUTOCLEAN=true option could be specified, which automatically executed the clean step). However, the infrastructure code in src/framework, src/operators, and src/external is core-independent and can be reused by multiple MPAS cores, provided build options are the same.

Now when compiling, build options are saved to the files .build_opts.$(CORE) and .build_opts.framework. If either of these files already exist, the build options saved in them are compared with the current build options to determine whether the core or the shared infrastructure needs to be recompiled.

The AUTOCLEAN=true option now cleans the core, shared infrastructure, or both as needed.

Also in this PR, old logic in the top-level Makefile for determining whether cleaning was necessary has been removed and the .gitignore file has been updated to ignore the new .build_opts.* files.

gdicker1 commented 1 month ago

user error

Example of user error:

  1. Build default init_atmsphere core with SMIOL
  2. Attempt a build with PIO and AUTOCLEAN=true, but abort before the clean occurs.
  3. Try another build with the same options as Step 2. This will result in a statement from a confused user like "well of course you can't find SMIOL, I want to use PIO now."
  4. (Hopefully,) the confused user issues a make clean ... command and moves on.

Because the build_options.* are overridden when AUTOCLEAN=true is provided, this might even result if (in a parallel build) the rebuild_check occurs as pio_test fails.

mgduda commented 1 month ago

@gdicker1 There are definitely ways to inadvertently trick the build system, and I think it's a matter of balancing code complexity with expectations that users have some idea of what is reasonable to do and what is not.

gdicker1 commented 1 month ago

Agreed and good point.

(and in case I was unclear, approved)

mgduda commented 1 month ago

@gdicker1 Actually, I think I've found a problem! Although src/framework, src/operators, and src/external are all core-independent, it turns out that the namelist filename (and a couple of other parameters) are build into the parse executable in the src/tools/registry directory. Why we did this, I have no recollection. Anyway, I think we'll need to either force the parse program to be rebuilt when switching cores, or make all of the core-specific parameters be command-line arguments.

mgduda commented 1 month ago

I'm not sure if I can convert this PR back to a draft now, so I've just added the "Don't merge" label. I'll see if I can get a suitable fix in place before the v8.2.0 release branch cutoff. Otherwise, I'll aim for the next release.

mgduda commented 1 month ago

@gdicker1 I'll re-request a review after PR #1180 has been reviewed and merged.

mgduda commented 1 month ago

@gdicker1 I've rebased this PR branch onto the HEAD of develop (including the merge of PR #1180), and I think we're good for another review.