Activity

1. gsell commented on Oct 21, 2026

   

   gsell

   on Oct 21, 2026

   ContributorAuthor

   More actions

   Code review: modbuild.tar (round 13, 20 Oct 2026)

   Files: libstd.bash (978 lines), libpmodules.bash (204), libpbuild.bash (1497), modbuild.in (1675). All four parse; no non-breaking spaces or other non-ASCII; no single-colon namespace calls. ShellCheck 0.11 reports only the known nameref false positives (SC2034).

   What changed: the cfg/config/module_config nameref parameters were replaced by a global nameref, ModuleConfig (declare -ng ModuleConfig=mod_config_… in build::modules_yaml_v1). The direction is good, but the conversion is incomplete in five places. Two of them stop every build.

   Method: I don't have the round-12 sources in this session, so this is a full read of all four files, not a diff. For the first time I ran modbuild end to end: a sandbox install, a stub modulecmd, a real recipe and config.yaml. I patched each blocker in the sandbox copy to reach the next one. Items marked † were reproduced this way. Bash 5.3 couldn't be obtained in this workspace, so runs used bash 5.2.21 with the version gates lowered, plus Go yq 4.47. Nothing found depends on 5.3 semantics.

   Suppressed by request and not listed: H1, H3, M4–M11, M13, L2–L5.

   New issues

   #	Sev	Location	Issue	Suggested fix
   B3 †	B	modbuild.in:1202–1204	cfg::expand_version_key dropped its config parameter, but the body still reads $2, $3, $4. The only caller (1593) passes three arguments, so the version key becomes the nameref target. Every build dies at version expansion with 1.{0..1}: invalid variable name for name reference, then $4: unbound variable.	local -n __evk_result="$1", __evk_key="$2", __evk_version="$3" (the header comment already documents it that way).
   B4 †	B	modbuild.in:968, 973	cfg::check_config reads ${_ModuleConfig[…]}, a name that doesn't exist. It runs for every variant (1603, 1617), so every build dies with 'overlay': syntax error: operand expected.	ModuleConfig in both lines.
   C8 †	C	modbuild.in:908, 926, 944, 953–956	cfg::get_config still writes to cfg[${key,,}]. With no cfg declared, the subscript is evaluated arithmetically and aborts with build_requires: unbound variable. Any config that uses build_requires, configure_args, docfiles, patch_files, runtime_deps, systems, use_overlays, variant, kernels, target_cpus or any …+ key dies. That is most real recipes.	ModuleConfig[…] at all six sites.
   C9 †	C	libpbuild.bash:947–948, 1443	PREFIX=$(pbcore::set_mod_dir_and_prefix) runs the function in a subshell. This causes two separate failures, both ending in exit 0. (a) The ModuleConfig['modulefile_dir']=… assignment is lost, so the value keeps its default, 0. The modulefile, .config-V, .modulerc and .deps-V are written to $BUILD_DIR/0/, which cleanup_build then deletes. No module ever becomes visible (reproduced: nothing under modulefiles/). (b) A pbcore::err_group_dependency inside the function only exits the subshell. The message prints, then the build continues with PREFIX=''. Reproduced with an MPI module whose MPI variables weren't set: configure would get --prefix=, and the test wrote documentation into /share/doc/hello on the host before reporting "done".	Call the function directly and let it set PREFIX itself: replace echo "${prefix}" with PREFIX="${prefix}" and PREFIX=$(…) with a plain call. Verified: the modulefiles land in …/Tools/modulefiles/hello/, and the MPI case now stops with exit 2.
   C10 †	C	libpbuild.bash:1305	pbcore::remove_module declares local -e modulefile_dir=…, but -e isn't a valid option. local fails, the variable is never set, and line 1311 aborts with modulefile_dir: unbound variable, after rm -rf "${PREFIX}" has already run. As a result, --clean-install deletes the installation and then dies, so it never rebuilds, and relstage: remove leaves the modulefiles pointing at a deleted prefix.	local -r. Verified: with that and C9 fixed, --clean-install removes the prefix and modulefiles, then rebuilds.
   H6 †	H	modbuild.in:594–595	cfg::err declares local -r name=… and then does name+=… on the next line. Assigning to a readonly variable kills the script, so every cfg::err_* message is lost. The user sees only line 595: name: readonly variable, with exit 1 instead of CONFIG (2). This is also what hid C8's and M17's real errors in my first run.	Drop -r, or build the string in one assignment.
   H7 †	H	libpbuild.bash:703–705	When unpacking fails, pbcore::unpack deletes the source file unless it came from ${BUILDBLOCK_DIR}. But pbcore::search_source_file also returns ${BUILDBLOCK_DIR}/files, so a tarball kept in the recipe's files/ directory, which is version-controlled, gets deleted. Reproduced: files/hello-1.0.tar.gz was gone after one failed build.	Delete only what this run downloaded, e.g. by having pbuild::prep remember whether it called pbcore::download_source_file.
   H8 †	H	libpbuild.bash:1166	pbcore::cleanup_modulefiles reads ModuleConfig['mod_version'], a key that doesn't exist; elsewhere it is 'version'. --cleanup-modulefiles always aborts with ModuleConfig['mod_version']: unbound variable.	ModuleConfig['version']
   M17 †	M	libstd.bash:819–822	yml::get_keys on an empty map: yq prints {}, which passes the null/empty test, then keys[] yields nothing, and readarray turns that into one empty key. As a result, config: {} under a version is rejected with invalid key(s) in config section - followed by a blank name.	Also treat {}/[] as empty, or skip readarray when the second yq output is empty.
   M18 †	M	modbuild.in:299	-v/--verbose installs trap 'echo "$BASH_COMMAND"' DEBUG. That looks like a debugging leftover: it echoes every top-level command into stdout and the log (45 lines for a trivial build), which is not what "Verbose output" in --help promises.	Remove it. --debug already does set -x.
   L7	L	various	Leftovers from the refactor. Callers still pass a config name that is now ignored: cfg::get_config … mod_config_* (1492, 1562, 1614), cfg::check_config mod_config_* (1603, 1617) and cfg::is_variant_to_build mod_config_variant (1615). key isn't local in build::modules_yaml_v1 (1486, 1556) and leaks as a global. local -a config_args=() is declared twice (libpbuild.bash:453–454). The PMODULES_HOME branch of pbuild::module_is_avail (libpbuild.bash:191–202) is unreachable, because modbuild.in:81 refuses to run when PMODULES_HOME is set.	Delete.
   L8	L	various	Messages and minor behavior. pbcore::err_setting_relstage (libpbuild.bash:44) and the sub-package invalid-key message (1375) each have an unbalanced '. cfg::err_undef_group (modbuild.in:638) exits CMD rather than CONFIG, the one cfg::err_* the M14 fix missed or that regressed. pbuild::compile sets VERBOSE=1 with declare -g but doesn't export it, so make never sees it, and it stays set for later builds (libpbuild.bash:505).	As described.
   L9	L	libpmodules.bash:10	(( BASH_VERSINFO[0] >= 5 && BASH_VERSINFO[1] >= 3 )) would reject bash 6.0–6.2. modbuild.in:9 has the correct form.	Copy the modbuild.in test.

   Status of previously reported issues

   #	Status
   M16	Largely closed by the refactor. cfg::is_variant_to_build no longer takes a nameref, so its collision risk is gone. cfg::get_config still pairs the nameref yml_input with the plain locals key, keys and value, but its only callers pass yaml_module_config, so it's latent.

   Notes

   • The new global ModuleConfig resolves dynamically to whichever mod_config_default/_version/_variant is in scope. Any function that declares a local with one of those three names will silently capture it. Nothing does today; it's worth a comment next to the declare -ng.
   • "environemnt" now appears three times (libpbuild.bash:65, 390, 392). Also: "errror" (modbuild.in:58), "varian" (1400), "noone" (libpbuild.bash:1158), "copyies" (255).
   • With B3, B4, C8 and H6 patched in the sandbox, a simple Tools build runs cleanly end to end. With C9 and C10 also patched, the modulefile, .config, .modulerc, --clean-install and the MPI error path all behave correctly. So these six are the whole critical path; I found nothing else on it.
2. gsell

   linked a pull request that will close this issuemodbuild.in: use global nameref ModuleConfig, cleanup, bug fixes #38on Oct 21, 2026
3. gsell

   closed this as completedin #38on Oct 21, 2026