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.

Sign up for GitHub

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

Jump to bottom

Cleaning up build system #662

Open
VincentVanlaer opened this issue Jun 20, 2024 · 13 comments
Open

Cleaning up build system #662

VincentVanlaer opened this issue Jun 20, 2024 · 13 comments
Assignees
@pmocz

Comments

@VincentVanlaer
Copy link
Collaborator

Context

I am experimenting with building and packaging MESA for nixos using nix. I did the same with GYRE recently. In order to do that properly, I wanted to understand how MESA actually gets build. I found the existing combination of bash scripts and makefiles difficult to understand and adapt, so I ended up re-implementing the build system mostly from scratch. This issue is for me to check if what I'm proposing below is considered a meaningful improvement.

What I found to be not ideal in the current build system

  • The combination of bash scripts and multiple makefiles in each module was confusing to look at at the start
  • Quite some things were duplicated between different modules
  • The make subfolders end up containing both the makefiles and build artifacts

What I ended up implementing

The implementation, with a few modules (const, utils, math, mtx, chem) ported can be found on https://github.com/VincentVanlaer/mesa/tree/new-build. The main flow is as follows:

Makefile (in repository root)  -> single Makefile in each module -> makefiles in the make subfolder doing the heavy lifting

The system has the following benefits:

  • All files produced by the build process end up in the build subfolder, making cleaning just deleting this folder
  • Each module only has a single short Makefile as configuration
  • Modules that do not depend on each other are built in parallel

In terms of lines of code, the changes are 162 files changed, 563 insertions(+), 2888 deletions(-) for the five modules ported.

Caveats & remarks

  • Some files got updated to be buildable with Fortran 2008
  • Minimal GNU make version required is 4.3 (released 2020) and would probably need to be packaged in the mesasdk
  • Dependencies (including external dependencies such as lapack) are implemented using pkg-config (which would mean some small changes to the mesasdk). This is useful if you want to build without the sdk.
  • I implemented and tested this only on my Linux installation
  • Some aspects such as a updating the install script (i.e. as a user friendly wrapper) and openmp support are not yet done

Conclusions

I have two questions after all of this (which are difficult to answer for me given that I have written them):

  • Are the new makefiles and scripts actually cleaner?
  • If so, is it worth the further effort of development and testing to swap things over?

It might also be worth considering alternatives such as only applying some of the concepts, or porting to another build system such as Meson.

If you want to play with the Makefiles I can also upload a patched version of the mesasdk that contains the necessary changes and a gnumake install with at least version 4.3.
@fxt44
Copy link
Member

fxt44 commented Jun 20, 2024

wow! impressive that rather than just complain about the current build system, an experiment with an alternative build system was pursued and implemented. i have no comments on your two questions at this point ; still absorbing the newness and scope of this development.

@mjoyceGR
Copy link
Member

This is brilliant Vincent! Was especially amused/impressed to see this post after midnight in the middle of the summer school :)

@warrickball
Copy link
Contributor

warrickball commented Jun 21, 2024 •
edited
Loading

First, thanks for taking this on. I think your observations are sound and having a demonstration of what things could look like will hopefully kickstart the process of fixing things.

I think it's worth tagging @rhdtownsend as the resident build system expert.

I'm going to boldly chime in, having only looked around for about 20 min, because I maintain the ifort-based tests we run, so I've fought with utils/makefile_header a bit over the years and at a glance I think the breakup of different scripts in $MESA_DIR/make will make non-standard builds (e.g. using HPC modules) easier.

I have two questions after all of this (which are difficult to answer for me given that I have written them):

  • Are the new makefiles and scripts actually cleaner?

Yes, very yes. The module makefile_bases duplicate a lot of code so simply refactoring that, as you have done, reduces those files. I never saw much value in cluttering the module directories with wafer-thin wrappers around make (e.g. clean, mk) and its appealing that, for the chem module,

the old structure 
$ tree chem/
chem/
├── build_and_test
├── build_and_test_parallel
├── build_data_and_export
├── clean
├── data
│   ├── isotopes.data
│   ├── lodders03.data
│   ├── lodders09.data
│   └── README
├── export
├── i1
├── i1p
├── make
│   ├── makefile
│   └── makefile_base
├── mk
├── preprocessor
│   ├── build_for_export
│   ├── chem_input_data.tar.xz
│   ├── clean
│   ├── inlist
│   ├── make
│   │   └── makefile
│   ├── mk
│   ├── README
│   ├── rebuild_all
│   ├── rn
│   └── src
│       ├── chem_support.f90
│       └── create_table.f90
├── private
│   ├── chem_isos_io.f90
│   ├── lodders_mod.f90
│   └── nuclide_set_mod.f90
├── public
│   ├── chem_def.f90
│   └── chem_lib.f90
└── test
    ├── chem_ids.list
    ├── ck
    ├── clean
    ├── cleanup
    ├── export
    ├── make
    │   ├── makefile
    │   └── makefile_base
    ├── mk
    ├── mkx
    ├── rn
    ├── src
    │   └── test_chem.f
    └── test_output

is at first glance reduced to

the new one. 
$ tree chem/
chem/
├── Makefile
├── preprocessor
│   ├── chem_input_data.tar.xz
│   ├── inlist
│   ├── Makefile
│   ├── README
│   └── src
│       ├── chem_support.f90
│       └── create_table.f90
├── private
│   ├── chem_isos_io.f90
│   ├── lodders_mod.f90
│   └── nuclide_set_mod.f90
├── public
│   ├── chem_def.f90
│   └── chem_lib.f90
└── test
    ├── chem_ids.list
    ├── src
    │   └── test_chem.f
    └── test_output

I'll note that I see less obvious gain over the structure of the old utils/makefile_header. The old one is ~340 lines, compared with the total contents of make now being ~272 lines. Still, just breaking it up makes it easier to navigate.

  • If so, is it worth the further effort of development and testing to swap things over?

I personally think yes, with the caveat that I might be underestimating how much effort remains.

It might also be worth considering alternatives such as only applying some of the concepts, ...

When I first start looking at this, I also wondered about cherry-picking some ideas, but I think everything you've implemented has clear benefits. If in each module, we consolidate makefile and makefile_base, remove the wafer-thin wrapper scripts and refactor what's left, I think we end up with more or less what you've got.

... or porting to another build system such as Meson.

I'm personally a big fan of trying to accomplish as much as possible with the simplest tools that are available. More advanced build tools have a place but I don't think our requirements are complicated enough to warrant the nuisance that might be caused for someone trying to install somewhere like an HPC system, with an old OS and little room to install base packages.

Regarding potential improvements, this stands out:

  • Modules that do not depend on each other are built in parallel

I like the sound of that! MESA's current build time is ~15-20 minutes. Just cutting that in half would feel like a big improvement on the CI turnaround.

I have three points:

  1. What feature of GNU make means you need at least version 4.3? I just had a look on our HPC and found 4.2.1, though 4.3 is available as a module. If it isn't too hard to workaround, I'd prefer relaxing that requirement by one or two minor versions, though I'm happy to keep the minimum requirement if it prevents things getting messy.
  2. We should preserve as much of the user-friendliness of the top-level install scripts as possible, including e.g.
  • warning that the SDK hasn't been initialised,
  • a single command to produce output to attach to troubleshooting queries and
  • detecting and stopping users from trying to run as sudo.
    I think e.g. ./help can be replaced with make help (or any other subcommand name) but the functionality must be somewhere.
  1. How much more work do you need to do to finish converting the build system, and how might the development team or community potentially make it easier for you? I'm personally happy to spend some very scarce time on seeing how to build with ifort/other non-standard builds under a new system. (I occasionally also built with the Fedora gfortran and libraries if gfortran got ahead of the SDK.)

@rhdtownsend
Copy link
Contributor

rhdtownsend commented Jun 21, 2024 •
edited
Loading

Jumping in here with a few remarks. I've recently done quite a bit of work in revamping the build system for GYRE (and other projects I maintain). The changes include

  • migrating from fpx3 to fypp as the pre-processor/templating engine. fpx3 required Perl, and hasn't been maintained for years; fypp is a maintained project (but does require Python 3).
  • replacing makedepf90 with a combination of fypp_deps (a wrapper around fypp that handles .fypp file dependency generation) and makedepf08.awk (an awk script that handles .f90 file dependency generation).
  • reworking the makefiles to avoid re-compilation cascades and increase parallel building. My approach was inspired by the fantastic notes at https://aoterodelaroza.github.io/devnotes/modern-fortran-makefiles/ (this was also the basis for the awk script).

The most recent release of GYRE (7.2) has many of these improvements; I'm going to be making another release (7.2.1) very soon, which will complete the update. It's this newer release that will make it into MESA as the GYRE refresh. Within the GYRE distribution directory, the top-level Makefile defines user-settable flags and various targets (all, install, clean, test, etc). The building all happens in the build/ subdirectory. The system is pretty clean, and robust against crazy stuff happening. And, it parallelizes very well, thanks to separate passes for module (.mod/.smod) file generation and code compilation.

ETA: oh, and not a single shell script apart from the SDK version checker!

@rhdtownsend
Copy link
Contributor

rhdtownsend commented Jun 21, 2024 •
edited
Loading

On the subject of GNU make 4.3, it would be possible to bundle make into the SDK (I've done that in the past with 'custom' SDKs). One of the big changes in 4.3 was the introduction of grouped targets, which can cut down on makefile verbosity (see https://www.gnu.org/software/make/manual/html_node/Multiple-Targets.html). But the new GYRE build system doesn't need this, as the automatic dependency generation obviates the need for grouped targets.

@rhdtownsend
Copy link
Contributor

A further note on the GYRE build system: all of the heavy lifting is done in the file build/Make.inc. This file handles all of the dependency generation, compilation rules, etc. build/Makefile just sets up the list of source files, libraries, executables. It would be possible to adapt this to MESA, with a single Make.inc for everything, and then a build/Makefile for each MESA module.

@VincentVanlaer
Copy link
Collaborator Author

Thanks for the positive feedback! I left some replies to the comments below

  1. What feature of GNU make means you need at least version 4.3? I just had a look on our HPC and found 4.2.1, though 4.3 is available as a module. If it isn't too hard to workaround, I'd prefer relaxing that requirement by one or two minor versions, though I'm happy to keep the minimum requirement if it prevents things getting messy.

It is indeed the grouped targets, which I introduced at some point to make parallel building work. Also because I currently don't understand how make deals with non-grouped multiple outputs.

  1. How much more work do you need to do to finish converting the build system, and how might the development team or community potentially make it easier for you? I'm personally happy to spend some very scarce time on seeing how to build with ifort/other non-standard builds under a new system. (I occasionally also built with the Fedora gfortran and libraries if gfortran got ahead of the SDK.)

It depends on how custom the other modules are. If they fit within the parameters of the new system as it is now, then it is not too much work. Maybe it would be a good idea for one or more developers to try to port some of the modules to the new system to become more familiar with it. It would also be great to see some tests with other compilers. In principle this should only require swapping out make/compile-settings-gnu.mk.

  • migrating from fpx3 to fypp as the pre-processor/templating engine. fpx3 required Perl, and hasn't been maintained for years; fypp is a maintained project (but does require Python 3).

  • replacing makedepf90 with a combination of fypp_deps (a wrapper around fypp that handles .fypp file dependency generation) and makedepf08.awk (an awk script that handles .f90 file dependency generation).

Since MESA doesn't use a templating engine like fpx3 (as far as I could tell at least), do you think it still make senses to replace makedepf90?

I'll definitely read through that.

And, it parallelizes very well, thanks to separate passes for module (.mod/.smod) file generation and code compilation.

That would indeed simplify things and could indeed remove the need for grouped targets

@VincentVanlaer
Copy link
Collaborator Author

I made the following two changes in the meantime

  • The module files are now outputted in a separate step, which increase parallelism. The build time on my laptop dropped from roughly 6 seconds to 5 seconds for these modules.
  • I used an anchor file instead of the grouped targets (similar to what is described in https://aoterodelaroza.github.io/devnotes/modern-fortran-makefiles/). This means that the minimum gnumake version is no longer 4.3

@warrickball
Copy link
Contributor

@VincentVanlaer, I just made a first attempt to build with the new system. I cloned your fork, switched to the new-build branch, loaded MESA SDK 23.7.3 and ran make in the top-level MESA directory (which I've exported as MESA_DIR). This immediately failed with:

$ make
make[1]: Entering directory '/home/wball/mesa/vv-new-build/const'
makedepf90 -free -m modules/%m.mod -B ../build/const  -I public:private public/const_def.f90 public/const_lib.f90 test/src/test_const.f | \
INSTALL_INCLUDES='' \
MODULES='../build/const/modules/const_def.mod ../build/const/modules/const_lib.mod' \
BUILD_DIR='../build/const' \
../make//gen-compile-tree > ../build/const/depend

makedepf90: ERROR: Unknown Option '-B'
compile.mk:33: ../build/const/depend: No such file or directory
make[1]: *** [compile.mk:27: ../build/const/depend] Error 1
make[1]: *** Deleting file '../build/const/depend'
make[1]: Leaving directory '/home/wball/mesa/vv-new-build/const'
make: *** [build/depend:2: const] Error 2

Indeed, -B isn't an option for makedepf90, though -b is. Is that the intention? If I change -B to -b in make/compile.mk, I instead get

$ make
mkdir -p build
make/gen-folder-deps "make" const utils math mtx chem > build/depend
make[1]: Entering directory '/home/wball/mesa/vv-new-build/const'
mkdir -p ../build/const
makedepf90 -free -m modules/%m.mod -b ../build/const  -I public:private public/const_def.f90 public/const_lib.f90 test/src/test_const.f | \
INSTALL_INCLUDES='' \
MODULES='../build/const/modules/const_def.mod ../build/const/modules/const_lib.mod' \
BUILD_DIR='../build/const' \
../make//gen-compile-tree > ../build/const/depend
mkdir -p ../build/const/lib/pkgconfig/
make[1]: *** No rule to make target '../build/const/public/const_def.o', needed by '../build/const/lib/libconst.a'.  Stop.
make[1]: Leaving directory '/home/wball/mesa/vv-new-build/const'
make: *** [build/depend:2: const] Error 2

but that might not be surprising.

@VincentVanlaer
Copy link
Collaborator Author

I guess that the version of makedepf90 is older than the one I have. I also had to patch makedepf90 to make include files cleaner to support, but that shoudn't cause the error. In any case, I have been cleaning up and documenting the new build system, and I have replaced makedepf90 by a simple script in the meantime, similar to what Rich has done for GYRE. I'll make at least a draft PR in the next few days (depending on how fast I go over the remaining modules I had ported already), which should work with the SDK as it currently is.

@rhdtownsend
Copy link
Contributor

rhdtownsend commented Aug 1, 2024 via email

@fxt44
Copy link
Member

fxt44 commented Aug 1, 2024

i don't see a patch file.

@rhdtownsend
Copy link
Contributor

Must have been stripped from the mail. Here it is:

makedepf90-sdk2.patch

@pmocz pmocz self-assigned this Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pmocz pmocz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@pmocz @fxt44 @rhdtownsend @VincentVanlaer @mjoyceGR @warrickball