Keep multi-word values intact when loading toolchain/modules

[Mohit-Ak]

A KEY=value line in toolchain/modules was exported by the loader like this:

log " $ export $(eval "echo \"$_entry\"")"
eval "export $_entry"

The entry isn't quoted, so eval word-splits the value. Everything after the first word is treated as a further name to export, that fails, and the failure goes to stderr — which ./mfc.sh load output is routinely redirected away from. The result is a variable that looks set but holds only its first word, with nothing anywhere to say so:

$ line='CRAY_CCE_LLD_ARGS=-plugin-opt=-mattr=-mai-insts -plugin-opt=-disable-promote-alloca-to-vector'
$ eval "export $line"
bash: export: `-plugin-opt=-disable-promote-alloca-to-vector': not a valid identifier
$ echo "[$CRAY_CCE_LLD_ARGS]"
[-plugin-opt=-mattr=-mai-insts]

As #1690 notes, that's not cosmetic: on Frontier CRAY_CCE_LLD_ARGS needs two -plugin-opt flags for CCE 21, one of which works around a code-generation bug that silently discards stores. Written unquoted, only the first is applied — the build succeeds, the tests run, and the numerical workaround is simply absent.

Why not the split-on-first-= fix

The issue suggests splitting on the first = and exporting without re-evaluating. That does fix multi-word values, but it breaks the multi-assignment lines already in the file, because those carry several assignments per line. I swept both approaches over every assignment line currently in toolchain/modules:

--- ENTRY: MFC_CUDA_CC=70,75,80 NVHPC_CUDA_HOME=$CUDA_HOME CC=nvc CXX=nvc++ FC=nvfortran
  (A) current:      MFC_CUDA_CC=[70,75,80]  NVHPC_CUDA_HOME=[/opt/cuda]  CC=[nvc]  CXX=[nvc++]  FC=[nvfortran]
  (B) split-on-'=': MFC_CUDA_CC=[70,75,80 NVHPC_CUDA_HOME=$CUDA_HOME CC=nvc CXX=nvc++ FC=nvfortran]
                    NVHPC_CUDA_HOME=[]  CC=[]  CXX=[]  FC=[]

Eight lines in the file have this shape (b-gpu, a-gpu, w-gpu, e-gpu, p-gpu, pifx-cpu ×3, c-cpu, i-all, …), so split-on-= would quietly stop setting the compilers on most clusters.

What this does instead

The loader now walks the line word by word and starts a new assignment only at a word shaped like an identifier followed by =; anything else is a continuation of the current value. Both shapes then survive, including a multi-word value followed by another assignment.

Values still go through one round of expansion so "$VAR" references to previously-exported variables keep working (NVHPC_CUDA_HOME=$CUDA_HOME), but the expanded result is exported directly rather than re-evaluated — re-evaluating is what dropped the extra words. Word-splitting runs under set -f so a value like -Wl,* can't pick up filenames from the working directory, and the previous globbing state is restored afterward.

Testing

toolchain/mfc/bootstrap_tests/test_modules_env.py drives the real __export_assignments function out of modules.sh (rather than a copy), so it fails if the implementation regresses. It covers every assignment shape currently in toolchain/modules, the multi-word cases from the issue, $VAR expansion inside a multi-word value, glob safety, and a sweep asserting that every assignment line shipped in toolchain/modules exports each of its names.

Against the unpatched loader, 4 of the 12 fail, with the 8 existing-shape tests still passing:

FAILED test_modules_env.py::test_multi_word_value_survives_intact
FAILED test_modules_env.py::test_multi_word_value_followed_by_another_assignment
FAILED test_modules_env.py::test_variable_reference_inside_a_multi_word_value
FAILED test_modules_env.py::test_glob_characters_in_a_value_are_not_expanded
4 failed, 8 passed in 0.29s

With the fix:

$ ./mfc.sh lint
...
============== 382 passed, 8 warnings, 4 subtests passed in 9.63s ==============

(382 vs. 370 on master — the 12 new tests, and ruff clean.)

./mfc.sh precheck passes all seven gates (formatting, spelling, toolchain lint, source lint, doc references, parameter docs, example cases).

I also ran the real loader loop end-to-end for several cluster slugs to confirm nothing changed for existing configurations:

slug=p  cg=gpu  -> CC=[nvc] CXX=[nvc++] FC=[nvfortran] MFC_CUDA_CC=[70,75,80,89,90] NVHPC_CUDA_HOME=[/opt/cuda]
slug=h  cg=gpu  -> CC=[.../mpicc] CXX=[.../mpicxx] FC=[.../mpifort]
                   UCX_NET_DEVICES=[mlx5_4:1,mlx5_7:1,mlx5_8:1,mlx5_9:1,mlx5_10:1,mlx5_13:1,mlx5_14:1,mlx5_15:1]
slug=pifx cg=cpu -> CC=[icx] CXX=[icpx] FC=[ifx]

One judgement call worth flagging: I put the new tests under toolchain/mfc/bootstrap_tests/ since there was no existing home for shell-level tests. Happy to move them if you'd rather they lived elsewhere.

Fixes #1690

[sbryngelson]

Code review

The core fix looks correct. I diffed old-vs-new behavior across all 23 =-bearing lines in toolchain/modules and got identical results — $VAR expansion, quoted empty values, and glob-safety are all preserved. The new tests do get collected and run: toolchain/pyproject.toml sets testpaths = ["."] and ./mfc.sh lint runs a recursive pytest, which is a hard CI gate.

Found 1 issue worth addressing, plus three minor notes.

1. The fix is incomplete — the module-classification step upstream still splits multi-word values on whitespace.

Before the export loop ever runs, MODULES is built by filtering out any token containing =:

MFC/toolchain/bootstrap/modules.sh

Lines 153 to 162 in 38c24b2

	ELEMENTS="$(__extract "$u_c-all") $(__extract "$u_c-$cg")"
	MODULES=`echo "$ELEMENTS" | tr ' ' '\n' | grep -v = | xargs`
	log " $ module load $MODULES"
	if ! module load $MODULES; then
	error "Failed to load modules."
	return
	fi

Any word of a multi-word value that does not itself contain = survives that filter and is passed to module load, which fails and hits error "Failed to load modules."; return — aborting mfc.sh load before __export_assignments is ever reached:

entry: LDFLAGS=-L$CUDA_HOME/lib64 -lcudart                ->  module load [-lcudart]   # aborts
entry: CRAY_CCE_LLD_ARGS=-plugin-opt=... -plugin-opt=...  ->  module load []           # fine

The originally reported #1690 case works only incidentally, because every one of its words happens to contain =. Three of the new tests assert shapes that cannot work end-to-end, since they exercise the function in isolation and bypass this step:

MFC/toolchain/mfc/bootstrap_tests/test_modules_env.py

Lines 104 to 127 in 38c24b2

	def test_quoted_multi_word_value_survives_intact():
	"""Quoting the value in toolchain/modules works too, and drops the quotes."""
	env = _load('CRAY_CCE_LLD_ARGS="-O2 -g"')
	assert env["CRAY_CCE_LLD_ARGS"] == "-O2 -g"
	def test_multi_word_value_followed_by_another_assignment():
	"""A space-carrying value ends where the next NAME= begins."""
	env = _load("CFLAGS=-O2 -march=native CC=gcc")
	assert env["CFLAGS"] == "-O2 -march=native"
	assert env["CC"] == "gcc"
	def test_variable_reference_inside_a_multi_word_value():
	"""Values are still expanded once, so they can reference earlier exports."""
	env = _load("LDFLAGS=-L$CUDA_HOME/lib64 -lcudart", preset_env={"CUDA_HOME": "/opt/cuda"})
	assert env["LDFLAGS"] == "-L/opt/cuda/lib64 -lcudart"
	def test_glob_characters_in_a_value_are_not_expanded():
	"""A value such as `-Wl,*` must not pick up filenames from the cwd."""
	env = _load("MYFLAGS=-Wl,* -O2")
	assert env["MYFLAGS"] == "-Wl,* -O2"

-g", -O2, -Wl,*, and -lcudart all lack =. No line in toolchain/modules hits this today, so it is latent rather than broken — but it is exactly the shape the PR's own rationale invites contributors to add next. Either extend the parser to the classification step as well, or narrow the tests and comments to the =-in-every-word case the fix genuinely covers.

Minor notes, take or leave:

2. __flush is defined nested inside __export_assignments. Bash does not scope function definitions, so it leaks into the sourcing shell and is never unset -f'd, unlike the adjacent unset _suffix _entry. This matches how __combine/__extract already behave, so it is consistent with existing convention.

MFC/toolchain/bootstrap/modules.sh

Lines 105 to 118 in 38c24b2

	__flush() {
	[ -z "$_acc" ] && return 0
	_key="${_acc%%=*}"
	_val="${_acc#*=}"
	# One round of expansion (so "$VAR" references still work), then export
	# the result directly -- re-evaluating it would word-split the value
	# again, which is the bug this function exists to avoid.
	_val="$(eval "echo \"$_val\"")"
	log " \$ export $_key=$_val"
	export "$_key=$_val"
	_acc=""
	}

3. [ "$_noglob" -eq 0 ] && set +f is the function's last statement, so it returns 1 whenever noglob was already set by the caller. Inert today — there is no set -e in mfc.sh, toolchain/util.sh, or modules.sh, and the call site discards the status — but an explicit return 0 would remove the trap.

MFC/toolchain/bootstrap/modules.sh

Lines 128 to 130 in 38c24b2

	[ "$_noglob" -eq 0 ] && set +f
	}

4. CLAUDE.md says "Tests: add one only when it protects real behavior — it would fail before your change and covers behavior a real case depends on. Prefer one targeted case over many broad ones." 8 of the 12 new tests pass against the old implementation, and the patch is 188 net new lines against the "If a patch exceeds roughly 100 net new lines, stop and justify" checkpoint. Regression guards around a parser rewrite are defensible, so this is a judgment call rather than a request to cut them.

MFC/CLAUDE.md

Lines 39 to 46 in 38c24b2

	Tests: add one only when it protects real behavior — it would fail before your change and
	covers behavior a real case depends on. Prefer one targeted case over many broad ones.
	Before editing, state: the smallest viable fix, what changes, what deliberately does not
	change, and whether a test is needed. After editing, report: files changed, net LOC, and
	anything that could still be deleted or simplified. If a patch exceeds roughly 100 net new
	lines, stop and justify before continuing.

Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}