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 👎.}

[Copilot]

Pull request overview

This PR fixes ./mfc.sh load’s export behavior so toolchain/modules entries with multi-word values (e.g., compiler/linker flags) are exported intact without truncation, while still supporting the existing “multiple assignments per line” format used across cluster slugs.

Changes:

• Replaces eval "export $_entry" with a new __export_assignments() parser that groups tokens into assignments while preserving space-containing values.
• Keeps one-round $VAR expansion for values, while avoiding re-evaluation that would re-trigger word-splitting.
• Adds pytest coverage that drives the real shell function and sweeps all assignment-bearing lines in toolchain/modules.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
toolchain/bootstrap/modules.sh	Introduces __export_assignments() and routes module-line exports through it to preserve multi-word values safely.
toolchain/mfc/bootstrap_tests/test_modules_env.py	Adds regression tests covering existing assignment shapes, multi-word values, $VAR expansion, glob safety, and a sweep over shipped module lines.
toolchain/mfc/bootstrap_tests/init.py	Establishes a package namespace for bootstrap shell-script tests.

[Mohit-Ak]

Good catch, and this turned out to be a live bug rather than only a hardening point. Bash's builtin echo consumes -n/-e/-E as options, so the old expansion silently dropped them:

MYVAR=-n              ->  []          # old: eval echo
MYVAR=-e              ->  []
MYVAR=-E foo          ->  [-E foo]

I went with the positional parameters rather than printf:

eval "set -- $_val"
_val="$*"

printf '%s' was my first attempt and it fails on the quoted multi-word case — CRAY_CCE_LLD_ARGS="-O2 -g" expands to two arguments, and %s only consumes the first per format cycle, so the value came back as -O2-g with the separator lost. $* joins on the first character of IFS, which reproduces echo's spacing exactly while going nowhere near option parsing. That showed up as a real test failure (test_quoted_multi_word_value_survives_intact) before I switched.

After the change:

MYVAR=-n              ->  [-n]
MYVAR=-e              ->  [-e]
MYVAR=-E foo          ->  [-E foo]
MYVAR=-n -e trailing  ->  [-n -e trailing]

Covered by test_value_that_looks_like_an_echo_option_survives, parametrized over those four.

[Mohit-Ak]

Agreed — fixed. The preset-env preamble now goes through shlex.quote:

preamble = "\n".join(f"export {k}={shlex.quote(v)}" for k, v in (preset_env or {}).items())

No current test needs it (the presets are /opt/cuda and /sw/afar), but the harness generates bash source text, so an unquoted value with a space or a metacharacter would change the meaning of the generated script rather than just failing the assertion — a confusing failure to debug later. The _quote() helper already did this for the entry under test; the preamble was the inconsistent path.

[Mohit-Ak]

Thanks — point 1 is right, and it's the more interesting half of the bug. Pushed e125a0f.

The classification step. I reproduced it before touching anything, running the old echo "$ELEMENTS" | tr ' ' '\n' | grep -v = | xargs over the shapes in question:

entry                                                  ->  module load [...]
CC=nvc CXX=nvc++ FC=nvfortran                          ->  []
MFC_CUDA_CC=70,75,80 NVHPC_CUDA_HOME=$CUDA_HOME ...    ->  []
LDFLAGS=-L$CUDA_HOME/lib64 -lcudart                    ->  [-lcudart]     # aborts
CRAY_CCE_LLD_ARGS=-plugin-opt=... -plugin-opt=...      ->  []
MYFLAGS=-Wl,* -O2                                      ->  [-O2]          # aborts
python cmake/3.22.2                                    ->  [python cmake/3.22.2]

So exactly as you described: a word survives the filter iff it lacks an = of its own, and the #1690 case only works incidentally. The three tests you flagged were asserting shapes that die one step earlier.

I went with extending the parser rather than narrowing the tests, since the alternative is a fix whose own rationale documents a shape that doesn't work end to end.

Both readers now share one rule, factored into __module_words() alongside __export_assignments(): a word shaped like an identifier followed by = opens an assignment, everything after it is that assignment's value, and words ahead of the first assignment are module names.

One thing that fell out while wiring it up: the classification had to move to a per-line loop. __extract() concatenates every matching line into one string, so with an assignment-aware split a trailing multi-word value on one line would swallow the next line's module names. Splitting per line is the same shape the export loop already uses.

Verification. Sweep over every one of the 108 content lines in toolchain/modules, old classification vs new, plus the latent shapes from your comment:

### every content line in toolchain/modules      108 entries, 0 differences
### latent shapes
LDFLAGS=-L$CUDA_HOME/lib64 -lcudart        OLD [-lcudart]  NEW []   changed
MYFLAGS=-Wl,* -O2                          OLD [-O2]       NEW []   changed
CRAY_CCE_LLD_ARGS=-plugin-opt=... ...      OLD []          NEW []   same
CFLAGS=-O2 -march=native CC=gcc            OLD []          NEW []   same
python cmake/3.22.2 CC=nvc CXX=nvc++       OLD [python cmake/3.22.2]  NEW [same]

Nothing shipped today changes; only the two latent shapes do. There's a test_module_names_in_toolchain_modules_are_unchanged test asserting that invariant against the real file, so a future line that would alter classification fails loudly.

./mfc.sh lint → 396 passed. ./mfc.sh precheck -j 12 → all 7 gates pass.

For the RED side I kept the new tests and surgically restored just the old classification and old expansion in a throwaway worktree: 6 failed, 20 passed — the 20 passing are the control, confirming the failures are the behaviour and not the harness. (Reverting modules.sh wholesale to 38c24b2 is a false RED, incidentally: the test harness lifts the shared glob-guard helpers out of the file, so everything errors on the harness instead.)

On your notes 2–4.

(3), the return status. Fixed — worth doing since factoring the glob guard out meant touching those lines anyway. set -f/set +f now live in __noglob_push/__noglob_pop, and the pop ends on an explicit return 0. There's a test for it (test_return_status_is_zero_when_the_caller_already_set_noglob).

(2), __flush leaking. Added unset -f __flush after the final flush. __combine/__extract are top-level definitions that the loader genuinely needs, whereas __flush is a loop-body helper, so cleaning it up seemed closer to intent than matching the convention. Happy to drop that line if you'd rather keep them uniform.

(4), test count vs CLAUDE.md. Fair, and the number went up rather than down — the rework needed the classification side covered. My reasoning: the fix is a parser rewrite whose whole risk profile is "some input shape gets split differently than before", so per-shape cases are the thing that actually protects the behaviour, and test_module_names_in_toolchain_modules_are_unchanged is the one that would catch a real regression on the file as it ships. If you'd still rather see it trimmed, tell me which cases you consider redundant and I'll cut them — I'd rather not guess, since each one encodes a shape from either the module file or this review.

Copilot's two inline notes are addressed as well; I replied under those with the details. The echo one turned out to be a live bug, not just a hardening nit — MYVAR=-n was exporting an empty string.