modular makefiles

[ortegagingrich]

This pull request contains some modifications to the common makefile intended to allow for a library-like system of Fortran source file dependencies while still retaining the benefits of using makefiles and maintaining backwards compatibility with existing makefiles. I demonstrated this to @rjleveque and he seemed to think that this might be a useful system to adopt. I describe it below and invite any ideas or comments on my proposed changes.

The basic idea is that in a makefile for a particular run, instead of specifying each Fortran source file required individually, the user might instead choose to "include" a makefile located in the relevant repository (e.g. $CLAW/classic/src/2d/Makefile.classic_2d) which contains a list of "common" files (COMMON_SOURCES and COMMON_MODULES) needed for such a run. If the user wishes to use some custom source files instead of the ones referenced in the common makefile, they would then specify those files in a separate list (SOURCES, MODULES). If the user wishes to replace a source file with one of a different name for their run, they can add the name of the "common" file to be excluded to a third list (EXCLUDE_SOURCES, EXCLUDE_MODULES) and add its replacement to the SOURCES or MODULES list. A python script in the clawutil repository is called from Makefile.common to combine these three lists into a single source list. It should be noted that EXCLUDE_SOURCES and COMMON_SOURCES need not be defined; if they are not, the lists contained in SOURCES and MODULES are used by to compile the executable. Thus, all existing makefiles will continue to work without necessary modification.

This system combines the advantages of makefiles with the modularity provided by a more standard package-based organization. Custom files can easily be specified in the run makefile without the need to list all files need for the run. Changes to the source code such as the refactoring of source files or the addition of new required source files no longer require changing all existing makefiles to reflect the changed source dependencies; it is sufficient simply to update the makefile in the repository. This system also can handle dependencies between repositories. For a geoclaw run, for example, which requires sources from both the geoclaw and 2D amrclaw repositories, it is sufficient to include Makefile.geoclaw located in the geoclaw source directory. This makefile contains a reference to another makefile located in the 2D amrclaw repository.

Note that I have only submitted pull requests for the classic and clawutil repositories at this point; however, I have implemented the system with amrclaw and geoclaw and have been using it for my personal runs for the last month or so without issue. All of the examples in the classic repository have been modified to use the proposed pseudo-package makefile structure. Below is an example from the makefile for the 3D acoustics example.

# Makefile for Clawpack code in this directory.
# This version only sets the local files and frequently changed
# options, and then includes the standard makefile pointed to by CLAWMAKE.
CLAWMAKE = $(CLAW)/clawutil/src/Makefile.common

# See the above file for details and a list of make options, or type
#   make .help
# at the unix prompt.


# Adjust these variables if desired:
# ----------------------------------

CLAW_PKG = classic                  # Clawpack package to use
EXE = xclaw                         # Executable to create
SETRUN_FILE = setrun.py             # File containing function to make data
OUTDIR = _output                    # Directory for output
SETPLOT_FILE = setplot.py           # File containing function to set plots
PLOTDIR = _plots                    # Directory for plots

OVERWRITE ?= True                   # False ==> make a copy of OUTDIR first
RESTART ?= False                    # Should = clawdata.restart in setrun

# Environment variable FC should be set to fortran compiler, e.g. gfortran

# Compiler flags can be specified here or set as an environment variable
FFLAGS ?=  

# ---------------------------------
# package sources for this program:
# ---------------------------------

include $(CLAW)/classic/src/3d/Makefile.classic_3d

# ---------------------------------------
# package sources specifically to exclude
# (i.e. if a custom replacement source 
#  under a different name is provided)
# ---------------------------------------

EXCLUDE_MODULES = \

EXCLUDE_SOURCES = \
  inlinelimiter.f90


# ---------------------------------
# List of custom sources for this program:
# ---------------------------------

MODULES = \

SOURCES = \
  qinit.f \
  setprob.f \
  bc3.f \
  setaux.f \
  $(CLAW)/riemann/src/rpn3_vc_acoustics.f90 \
  $(CLAW)/riemann/src/rpt3_vc_acoustics.f90 \
  $(CLAW)/riemann/src/rptt3_vc_acoustics.f90 \
  $(CLAW)/classic/src/3d/limiter.f90 \
  $(CLAW)/classic/src/3d/philim.f90

#-------------------------------------------------------------------
# Include Makefile containing standard definitions and make options:
include $(CLAWMAKE)

This example demonstrates the use of the standard set of source files for classic 3D runs, with some custom files and replacing a "common" source file inlinelimiter.f90 with limiter.f90 and philim.f90. Note that when specifying sources to exclude, it is not necessary to give the full path to the source file, but only its name. Also note that when replacing files with custom sources with the same file name (like qinit.f and setprob.f), it is not necessary to specify it as a source to be excluded; the python script automatically excludes the eponymous common source.

Finally, note that Riemann solvers must still be specified manually in the Makefile.

I have also modified the tests in the classic repository to use the new system, but was unsure if it would be appropriate to include modifications to these in this pull request, so I did these changes on a separate branch from the one that I am submitting this pull request for.

Hopefully this explanation was clear . . . Please let me know if there is any ambiguity.

[mjberger]

Organizing the files in the makefile is fine. But to me this adds complexity, more things to remember, and more things that could go wrong. We already have modules that don't also work, and a Makefile.common error.

Am I wrong in thinking this adds another level of complexity to an already complicated make system?

[rjleveque]

A big advantage is that it avoids having to change all the application Makefiles if we add a new source code file in the library. This is already a problem and will get worse if we succeed in growing the apps repository. Keeping the long list of standard library routines in one file rather than replicating everywhere seems like a best practice to minimize errors.

A Makefile of the current form would still work if one wants to explicitly list all the files, but with this system the user can examine $(CLAW)/classic/src/3d/Makefile.classic_3d to see what the standard files are if they want, and also see more explicitly what has been changed (rather than having to search through the long local Makefile looking for places a filename or location was changed, as often done when debugging).

So I see this as simplifying and making it less prone to common errors rather than complicating, but I would be interested to hear more about concerns or further suggestions.

[mandli]

I like this and upon further thinking this may actually allow us to better eliminate the occasional module problem (although that will still take some thinking).

[mjberger]

To avoid this problem I’ve been adding small routines into existing ones of the same general type.

As I said, to me this look like it will make the build system more complicated and less transparent, and will be another layer of indirection when trying to figure things out. But if I’m outvoted so be it.

On Sep 22, 2015, at 11:26 AM, Randall J. LeVeque notifications@github.com wrote:

A big advantage is that it avoids having to change all the application Makefiles if we add a new source code file in the library. This is already a problem and will get worse if we succeed in growing the apps repository. Keeping the long list of standard library routines in one file rather than replicating everywhere seems like a best practice to minimize errors.

A Makefile of the current form would still work if one wants to explicitly list all the files, but with this system the user can examine $(CLAW)/classic/src/3d/Makefile.classic_3d to see what the standard files are if they want, and also see more explicitly what has been changed (rather than having to search through the long local Makefile looking for places a filename or location was changed, as often done when debugging).

So I see this as simplifying and making it less prone to common errors rather than complicating, but I would be interested to hear more about concerns or further suggestions.

—
Reply to this email directly or view it on GitHub.

[ortegagingrich]

Simply choosing not to add new routines or modules does not seem like a viable long term solution and seems to go against good software development practices. Changes of this sort (for example, the recent additions to the geoclaw repository for storm surges) are inevitable and trying to get around it by forcing new functionality into existing files/routines/modules that are only marginally related will very quickly increase complexity.

I do not really think that transparency is an issue at play here either, as it is not very difficult for a user to navigate to the referenced library makefile and see the full list of files used should they choose to do so.

[mandli]

I tend to agree with @ortegagingrich although one thing did occur to me, previously we did not pursue a library approach before was that there was a desire to keep a record of what source files were used for any compilation. This could actually be accomplished using this approach by recording in a text file what the full source was at the time of compilation. Note that this is only a problem for the Makefile lists in other repositories (i.e. for a GeoClaw run the AMRClaw sources should be recorded).

[donnaaboise]

I think it will definitely simplify the makefile structure to have common source files linked via a single common make file, rather than requiring that each application makefile list every common file. This will also make maintenance easier.

But the need for "EXCLUDE_SOURCE", and "EXCLUDE_MODULE" points to the weakness of a system that uses the Makefile/compile system as a way to customizing codes. Ideally, the user would provide their custom versions of a library function via function pointers or an inheritance tree. Then multiple copies of, say, a limiter routine could be compiled (assuming different subroutine names) without the compiler complaining. Early on, the Clawpack design allowed for Riemann solvers to be passed into claw2/claw3 as function pointers. But instead of exploiting this feature and allowing the user to pass in "rpn2_acoustics", or "rpn2_euler" (which would have allowed for different Riemann solvers to be compiled into the the same executable), the build system took over and is now used instead as the mechanism by which users customize their code.

I suggest extending that early idea by first deciding which library routines the user is likely to want to customize (Riemann solvers, flag2refine, limiters, src2, setaux, bc2, and so on), and then defining a mechanism (function pointers, or inheritance) by which the user could dynamically choose among different versions of one of these routines. Even a simple virtual function table (possibly implemented as a module) might be easy enough to implement, and would avoid the need for relying on Makefiles for basic code customization.

[mandli]

I definitely agree with your approach. The one downside is that this would break a lot of backwards compatibility so we would have to put this off to 6.x I would guess. The classic prototype code makes heavy use of function pointers in this way and is a good place to start from for taking that approach. In the mean time I am still in favor of pursuing this as an intermediate step.

[mjberger]

when we break backward compatibility, I just put in an issue that I would like to automatically expand, or at least allow for restarting with an expanded number of nodes. This would be easier if we wrote out the old number of number for restarting, though it could also be done just when running out, instead of preemptively.

Marsha

On Oct 1, 2015, at 5:44 PM, Kyle Mandli notifications@github.com wrote:

I definitely agree with your approach. The one downside is that this would break a lot of backwards compatibility so we would have to put this off to 6.x I would guess. The classic prototype code makes heavy use of function pointers in this way and is a good place to start from for taking that approach. In the mean time I am still in favor of pursuing this as an intermediate step.

—
Reply to this email directly or view it on GitHub.

[rjleveque]

I've been experimenting with this using clawpack/classic#70 and also in amrclaw and I like this approach. In amrclaw I also tested redefining a module locally and it seems to work fine.

But see also the note I just added to that PR.

[rjleveque]

Merging this in after more discussion with @mjberger and @mandli. Old-style Makefiles still work, but I plan to update all the examples and tests to this new form in order to simplify future changes to code.