From 38c24b2c765e4e4697e00e310e4a1a417a0e2a3d Mon Sep 17 00:00:00 2001 From: Mohit Arvind Khakharia Date: Mon, 3 Aug 2026 02:17:43 -0400 Subject: [PATCH] Keep multi-word values intact when loading toolchain/modules A `KEY=value` line in toolchain/modules was exported with `eval "export $_entry"`, which word-splits the value. Everything after the first word was treated as a further name to export; that failed, and the failure went to stderr, which `./mfc.sh load` output is routinely redirected away from. The variable ended up set but holding only its first word, with nothing to say so. The consequences are 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 was applied -- the build succeeded and the numerical workaround was simply absent. Splitting on the first '=' instead, as the issue suggests, fixes multi-word values but breaks the multi-assignment lines that are already in the file (CC=nvc CXX=nvc++ FC=nvfortran would collapse into CC). So the loader now walks the line word by word and starts a new assignment only at a word shaped like an identifier followed by '=', treating anything else as a continuation of the current value. Values are still expanded once so "$VAR" references to earlier exports keep working, but the expanded result is exported directly rather than re-evaluated, which is what dropped the extra words before. Fixes #1690 --- toolchain/bootstrap/modules.sh | 47 +++++- toolchain/mfc/bootstrap_tests/__init__.py | 1 + .../mfc/bootstrap_tests/test_modules_env.py | 144 ++++++++++++++++++ 3 files changed, 190 insertions(+), 2 deletions(-) create mode 100644 toolchain/mfc/bootstrap_tests/__init__.py create mode 100644 toolchain/mfc/bootstrap_tests/test_modules_env.py diff --git a/toolchain/bootstrap/modules.sh b/toolchain/bootstrap/modules.sh index d68811b2f..4ac7479e3 100644 --- a/toolchain/bootstrap/modules.sh +++ b/toolchain/bootstrap/modules.sh @@ -85,6 +85,50 @@ __extract() { __combine "$(grep -E "^$1\s+" toolchain/modules | sed "s/^$1\s\+//")" } +# Export the KEY=value assignments on one toolchain/modules line. +# +# A line may carry several assignments (CC=nvc CXX=nvc++ FC=nvfortran), and a +# value may itself contain spaces (linker/compiler flags). Splitting on +# whitespace would truncate the latter; splitting on the first '=' would merge +# the former. So start a new assignment only at a word shaped like an +# identifier followed by '=', and treat every other word as a continuation of +# the current value. +# +# Values are still passed through eval so that "$VAR" references to previously +# exported variables keep expanding, as they always have. +__export_assignments() { + local _entry="$1" _word _acc="" _key _val _noglob=0 + + # Word-splitting below must not glob values such as -Wl,*. + case "$-" in *f*) _noglob=1 ;; esac + set -f + + __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="" + } + + for _word in $_entry; do + if [[ "$_word" =~ ^[A-Za-z_][A-Za-z0-9_]*= ]]; then + __flush + _acc="$_word" + else + _acc="$_acc $_word" + fi + done + __flush + + [ "$_noglob" -eq 0 ] && set +f +} + COMPUTER="$(__extract "$u_c")" if [[ -z "$COMPUTER" ]]; then @@ -122,8 +166,7 @@ fi for _suffix in "all" "$cg"; do while IFS= read -r _entry; do if echo "$_entry" | grep -q '='; then - log " $ export $(eval "echo \"$_entry\"")" - eval "export $_entry" + __export_assignments "$_entry" fi done < <(grep -E "^$u_c-$_suffix\s+" toolchain/modules | sed "s/^$u_c-$_suffix\s\+//") done diff --git a/toolchain/mfc/bootstrap_tests/__init__.py b/toolchain/mfc/bootstrap_tests/__init__.py new file mode 100644 index 000000000..ae61419a4 --- /dev/null +++ b/toolchain/mfc/bootstrap_tests/__init__.py @@ -0,0 +1 @@ +"""Tests for the shell bootstrap scripts under toolchain/bootstrap/.""" diff --git a/toolchain/mfc/bootstrap_tests/test_modules_env.py b/toolchain/mfc/bootstrap_tests/test_modules_env.py new file mode 100644 index 000000000..fd6e37edd --- /dev/null +++ b/toolchain/mfc/bootstrap_tests/test_modules_env.py @@ -0,0 +1,144 @@ +"""Tests for the KEY=value loader in toolchain/bootstrap/modules.sh. + +`. ./mfc.sh load` exports the assignments written on `toolchain/modules` lines. +Those lines carry two shapes that pull in opposite directions: + + * several assignments on one line -- `CC=nvc CXX=nvc++ FC=nvfortran` + * a single value that itself contains spaces -- linker/compiler flags + +The loader has to keep both intact. The tests below drive the real shell +function out of modules.sh rather than a copy of it, so they fail if the +implementation regresses. +""" + +import re +import shutil +import subprocess +import textwrap +from pathlib import Path + +import pytest + +MODULES_SH = Path(__file__).resolve().parents[2] / "bootstrap" / "modules.sh" + +pytestmark = pytest.mark.skipif(shutil.which("bash") is None, reason="bash is required to exercise modules.sh") + + +def _extract_function(name): + """Return the source text of a shell function defined in modules.sh. + + modules.sh is meant to be `source`d with arguments and runs an interactive + loader at import time, so the function under test is lifted out on its own. + """ + text = MODULES_SH.read_text() + match = re.search(rf"^{name}\(\) \{{.*?^\}}", text, re.MULTILINE | re.DOTALL) + assert match, f"{name}() not found in {MODULES_SH}" + return match.group(0) + + +def _load(entry, preset_env=None): + """Run one `toolchain/modules` entry through the loader. + + Returns the resulting environment as a dict, so a test can assert on the + variables the entry was supposed to set. + """ + preamble = "\n".join(f"export {k}={v}" for k, v in (preset_env or {}).items()) + script = textwrap.dedent( + """\ + log() {{ :; }} # modules.sh logging stub + {preamble} + {function_src} + __export_assignments {entry} + env + """ + ).format(preamble=preamble, function_src=_extract_function("__export_assignments"), entry=_quote(entry)) + + proc = subprocess.run(["bash", "-c", script], capture_output=True, text=True, check=True) + env = {} + for line in proc.stdout.splitlines(): + key, sep, value = line.partition("=") + if sep: + env[key] = value + return env + + +def _quote(value): + """Single-quote a string for safe interpolation into the bash script.""" + return "'" + value.replace("'", "'\\''") + "'" + + +@pytest.mark.parametrize( + "entry,expected", + [ + # Every multi-assignment shape currently in toolchain/modules. + ( + "MFC_CUDA_CC=70,75,80 NVHPC_CUDA_HOME=$CUDA_HOME CC=nvc CXX=nvc++ FC=nvfortran", + {"MFC_CUDA_CC": "70,75,80", "NVHPC_CUDA_HOME": "/opt/cuda", "CC": "nvc", "CXX": "nvc++", "FC": "nvfortran"}, + ), + ("CC=nvc CXX=nvc++ FC=nvfortran", {"CC": "nvc", "CXX": "nvc++", "FC": "nvfortran"}), + ("MPICC=mpiicc MPICXX=mpiicpc MPIFC=mpiifort", {"MPICC": "mpiicc", "MPICXX": "mpiicpc", "MPIFC": "mpiifort"}), + ('UCX_NET_DEVICES="mlx5_4:1,mlx5_7:1"', {"UCX_NET_DEVICES": "mlx5_4:1,mlx5_7:1"}), + ('PYTHONPATH=""', {"PYTHONPATH": ""}), + ("NVHPC_CUDA_HOME=$CUDA_HOME", {"NVHPC_CUDA_HOME": "/opt/cuda"}), + ], +) +def test_existing_module_lines_still_load(entry, expected): + """Assignment shapes already in toolchain/modules keep their old meaning.""" + env = _load(entry, preset_env={"CUDA_HOME": "/opt/cuda"}) + for key, value in expected.items(): + assert env.get(key) == value + + +def test_multi_word_value_survives_intact(): + """A value with spaces is exported whole, not truncated to its first word. + + Written unquoted, `CRAY_CCE_LLD_ARGS` used to keep only the first flag: the + rest were treated as further names to export and the failure went to stderr, + which `./mfc.sh load` routinely discards. The build then succeeds with a + compiler workaround silently missing. + """ + flags = "-plugin-opt=-mattr=-mai-insts -plugin-opt=-disable-promote-alloca-to-vector" + env = _load(f"CRAY_CCE_LLD_ARGS={flags}") + assert env["CRAY_CCE_LLD_ARGS"] == flags + + +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" + + +def test_every_assignment_line_in_toolchain_modules_loads_cleanly(): + """No line shipped in toolchain/modules may error out or lose a value.""" + modules_file = MODULES_SH.resolve().parents[1] / "modules" + entries = [] + for line in modules_file.read_text().splitlines(): + match = re.match(r"^[a-z][a-z0-9]*-(?:all|cpu|gpu)\s+(.*)$", line) + if match and "=" in match.group(1): + entries.append(match.group(1)) + + assert entries, "expected at least one KEY=value line in toolchain/modules" + + for entry in entries: + env = _load(entry, preset_env={"CUDA_HOME": "/opt/cuda", "OLCF_AFAR_ROOT": "/sw/afar"}) + for name in re.findall(r"(?:^|\s)([A-Za-z_][A-Za-z0-9_]*)=", entry): + assert name in env, f"{name} was not exported by: {entry}"