Repository navigation
build_tsmp2.sh housekeeping #65
Description
Activity
As far as I understand the objectives of
build_tsmp2.shis 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.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.shlightweight yet robust at the same time is worth the effort :)My comments on each of your ideas:
- The default environment file should be loaded based on the
hostnamecommand, which is universally available on*Nixsystems unlike$SYSTEMNAME.
This is not true. The
hostnamecommand 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.shshould avoid setting machine-specific environment variables (e.g.,$SYSTEMNAMEand$STAGE). Environment variables should only be set in the environment file.
Most convenient is to set
SYSTEMNAMEandSTAGEs.t. environment file can be picked based onSYSTEMNAME.
LikeSYSTEMNAMEalso theSTAGEis 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.shdoes 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 eCLMor 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
GPUandCPUbuild 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.shis 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?- The default environment file should be loaded based on the
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-2025tomaster). 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.envand associated logic. Especially the introduction ofMACHINEraises my eyebrows. Besides my earlier arguments about hostname(1) being the wrong command, at JSC the whole purpose ofSYSTEMNAMEis 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 OpenBSDmachinereturns the same asarch.)I think Unix did not envision the rise of clusters, so there is no command for the clustername.
I propose to revert 74b68a4.
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.
On to the issues:
1. Introduction of
default.2025.envDoing 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 inbuild_tsmp2.sh.I'm fine reverting
74b68a4if there's a stronger argument to what I've just discussed here and my previous comment.2. Where and how to set
$SYSTEMNAMEand$STAGE?build_tsmp2.shshould 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
SYSTEMNAMEandSTAGE.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
- Canonical way of constructing
I really wish we had already merged
stages-2025intomasterlast 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 forstages-2025-pdafto be merged intostages-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.
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 😜
Reacted by Marco van Hulten- added a commit that references this issue
on Apr 17, 2025 - pinned this issue
on May 16, 2025 - moved this from New features to Housekeeping in TSMP2: Towards Stages/2026 support
on Jan 13, 2026 - removed a sub-issue
on May 22, 2026
#53 introduced a new set of options for supporting multiple build configurations. This also introduced a lot of issues in
build_tsmp2.sh:hostnamecommand, which is universally available on*Nixsystems unlike$SYSTEMNAME.build_tsmp2.shshould avoid setting machine-specific environment variables (e.g.,$SYSTEMNAMEand$STAGE). Environment variables should only be set in the environment file.BUILD_ID. This should encode information about the machine, compiler, and model combination used.build_tsmp2.shdoes not use program arguments (i.e. input strings not prefixed with dashes) . Everything is either a short or long option.GPUandCPUbuild options.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.