build_tsmp2.sh housekeeping

[kvrigor]

#53 introduced a new set of options for supporting multiple build configurations. This also introduced a lot of issues in build_tsmp2.sh:

• The default environment file should be loaded based on the hostname command, which is universally available on *Nix systems unlike $SYSTEMNAME.
• build_tsmp2.sh should avoid setting machine-specific environment variables (e.g., $SYSTEMNAME and $STAGE). Environment variables should only be set in the environment file.
• Decide on the canonical way of constructing BUILD_ID. This should encode information about the machine, compiler, and model combination used.
• Address un-ergonomic and POSIX-violating option handling.
  • build_tsmp2.sh does not use program arguments (i.e. input strings not prefixed with dashes) . Everything is either a short or long option.
    • Investigate if CLI parsing libraries are worth it (e.g. argparse.sh, argbash)
  • Devise a much simpler grammar for specifying model combinations and compiler switches. The grammar should also account for GPU and CPU build options.
  • In general, following CLI design best practices is a good idea and will save us from future maintenance headaches.
• Add a bash linter in CI (e.g. shellcheck) to easily catch mistakes and enforce proper coding style.

This list is a set of TODOs and are not exhaustive; feel free to add more using the comments below. I suggest addressing these issues after #53 and before we move on to fully supporting GPU builds.

[DCaviedesV]

As far as I understand the objectives of build_tsmp2.sh is to be a minimum, ultra-thin wrapper around the more sophisticated CMake build interface. The expert user should be more or less happy to fall back on CMake for detailed control of the build.
Part of the consideration is that it should be lightweight enough to minimise the effort of maintaining it. Although I understand the concerns @kvrigor raises with it, we should not only investigate whether implementing more sophisticated and compliant approaches are worth it, we should very much ask whether we need/want to.

[kvrigor]

lightweight enough to minimise the effort of maintaining it

Unfortunately lightweight solutions are typically made through lightweight efforts, i.e. dirty hacks, which compromises long-term maintenance. This is not wrong per se since pragmatism is necessary in moving forward, but we cannot ignore tech debts either. I think the ROI of making build_tsmp2.sh lightweight yet robust at the same time is worth the effort :)

[mvhulten]

Sorry, I only saw this now. I do not agree with most of the ideas here. The parts about the hostname/SYSTEMNAME are in the comments to #53.

Apropos, @s-poll and I just found out that messages in a review are not visible until you submit them!

[mvhulten]

My comments on each of your ideas:

• The default environment file should be loaded based on the hostname command, which is universally available on *Nix systems unlike $SYSTEMNAME.

This is not true. The hostname command returns the node name, possibly with information identifying the cluster name, but it need not. I have introduced a more reliable method in e40615e.

• build_tsmp2.sh should avoid setting machine-specific environment variables (e.g., $SYSTEMNAME and $STAGE). Environment variables should only be set in the environment file.

Most convenient is to set SYSTEMNAME and STAGE s.t. environment file can be picked based on SYSTEMNAME.
Like SYSTEMNAME also the STAGE is encoded in the environment filename as done in my great commits e40615e and cbf9ccb.

• Decide on the canonical way of constructing BUILD_ID. This should encode information about the machine, compiler, and model combination used.

This *could encode.....:-) let's discuss and decide at a meeting.

• Address un-ergonomic and POSIX-violating option handling.

It does not group arguments. So it is not a full implementation of getotps. What I could think of someone types ./build_tsmp2.sh -hv (to get verbose help or something) and then it returns an obvious error, user going like "oh, slightly unexpected, let's try ./build_tsmp2.sh -h -v, that works, oh well, it didn't have verbose help anyhow." So if we stick with q, v and h as the only short options, we're fine.

• build_tsmp2.sh does not use program arguments (i.e. input strings not prefixed with dashes) . Everything is either a short or long option.

Except for help, quiet and verbose. All options have long versions. I think it's fine to keep as is.
edit: I misread (you did not meant to write "either"); program arguments like --comps eCLM or so could make sense. Possibly parser (below needed). But not sure if such arguments are needed to start with.

• Investigate if CLI parsing libraries are worth it (e.g. argparse.sh, argbash)

Don't fix what's not broken?

• Devise a much simpler grammar for specifying model combinations and compiler switches. The grammar should also account for GPU and CPU build options.

Sounds good.

• In general, following CLI design best practices is a good idea and will save us from future maintenance headaches.

I'll have a look; looks like a couch read for me :-).

• Add a bash linter in CI (e.g. shellcheck) to easily catch mistakes and enforce proper coding style.

I've installed it now and will try to use it now and then. It returns some useful hints, but it seems that build_tsmp2.sh is pretty okay already according to shellcheck (and some issues it caught I already saw but didn't care for so much because they fail in corner cases only—still good tips). So no need to integrate it in the CI. People are free to run shellcheck and push a commit! If you do integrate it in the CI, I'd suggest to stick with warnings, not blocking. Is it trivial to integrate it in CI?

[mvhulten]

You call it housekeeping but it feels like refactor. Two words for non-functional change (beyond occasional bug fix or introduction as a side effect), so it is indeed good as you say we'll address the above after #53 (request to merge stages-2025 to master). At the Z04 meeting of 3 April we decided to wrap this up and merge when possible. As I see it, this means no housekeeping/refactoring.

But you already introduced some things in stages-2025, namely you made headways on your first two points within this pull request!

Concretely, I have doubts about the introduction of env/default.2025.env and associated logic. Especially the introduction of MACHINE raises my eyebrows. Besides my earlier arguments about hostname(1) being the wrong command, at JSC the whole purpose of SYSTEMNAME is to get the... uh... what's the name, right the system name! Not the hostname, but the system name, or the machine name. (Last fun fact, on OpenBSD machine returns the same as arch.)

I think Unix did not envision the rise of clusters, so there is no command for the clustername.

I propose to revert 74b68a4.

[kvrigor]

You raise a lot of valid concerns @mvhulten. To ground the discussion, let me take a step back and describe how I envision the TSMP2 build system. I will address the issues on a separate comment.

The main goal of TSMP2 is to be able to support coupled model builds on any machine, with HPC systems being the main use case. Local machines and cloud runners, despite being underpowered, are also included since being able to build TSMP2 on such machines demonstrates the generality of TSMP2.

Making TSMP2 universal as much as possible has a practical reason: it needs to keep pace with frequent HPC software+hardware upgrades without straining our very limited developer resources. I believe this could be achieved by diligently separating slow-changing core build scripts from the fast-changing machine-specific scripts:

In the long run, I expect adding support for new machines or software stages will become as simple as creating a new environment file. Moreover, such new support should not require touching the core build scripts (i.e. build_tsmp2.sh + CMake scripts). Compared to TSMP1, the code structure of TSMP2 is much simpler that it becomes obvious which files have to be changed when adding or fixing stuff.

The housekeeping/refactoring tasks in my first post ultimately serve the ideal TSMP2 build system—lean, supports every machine, and highly maintainable.

[kvrigor]

On to the issues:

1. Introduction of default.2025.env

Doing so would simplify this piece of code to below:

# Use default env file if no env file was provided
if [[ -z "${tsmp2_env}" ]]; then
  tsmp2_env="${cmake_tsmp2_dir}/env/default.2025.env"
fi

# Source environment file
if [[ -n "${tsmp2_env}" ]]; then
  message "Sourcing environment..."
  source "$tsmp2_env" ${compiler_options}
fi

This eliminates associating an environment file to a particular machine in build_tsmp2.sh, which is a liability since this tends to get more complex as new machine+stage+compiler combinations are introduced. Better outsource this to the default environment file.

I didn't bother putting more effort on the naming and the local variable usage in default.2025.env. This can be improved in next iteration. What's more important is to start removing the hostname-to-environment mapping logic in build_tsmp2.sh.

I'm fine reverting 74b68a4 if there's a stronger argument to what I've just discussed here and my previous comment.

2. Where and how to set $SYSTEMNAME and $STAGE?

build_tsmp2.sh should not bother setting this in the first place. Maintainability improves when there's a clear separation between build logic and machine-specific logic.

The default environment file is the best place to set the default SYSTEMNAME and STAGE.

3. Other noteworthy issues

These are less important issues so I would postpone commenting on them to avoid fragmenting the discussion. Once I have the time I'll express my thoughts on a separate issue/PR.

• Canonical way of constructing BUILD_ID
• Option handling semantics
• Bash linter in CI

[mvhulten]

I really wish we had already merged stages-2025 into master last week (#53). Given that you thought out an improvement in the structure and that finalising this should take no more than a week(?) and that we also need to wait for stages-2025-pdaf to be merged into stages-2025, I am okay if we do this.

The idea to separate the core build scripts from the machine-specific scripts is fine. (I don't think they have much different paces per se.) The above clarification was really needed as the naming is confusing and you did not mean when writing this comment as it is logically impossible to do that. It is so important to be precise, especially in collaborative developments.

I could fix this up, but let's first discuss this approach and the PRs' status in tomorrow's meeting.

[kvrigor]

I really wish we had already merged stages-2025 last week.

I'm also eager to merge #53 ASAP, but this crucial eCLM fix is blocking the PR: HPSCTerrSys/eCLM#62

It is so important to be precise, especially in collaborative developments.

Your feedback made me flesh out my thoughts and express them more clearly. I really appreciate it!

Don't worry—I won't sneak more housekeeping fixes until the last TODO in #53 😜