build: rebuild dependencies built by a different toolchain - #1935
sbryngelson wants to merge 2 commits into
Conversation
Dependencies are keyed by name alone, so one built under another environment was reused as-is. Two ways this broke a build, both seen in practice: - LAPACK, HDF5 and Silo built under amdflang were reused after switching to the gnu modules, and post_process failed to link (undefined FIRModule symbols). - The superbuild satisfied FFTW with a library found on the system (an AFAR toolchain on PATH) and installed nothing. In an environment where that library is not visible, simulation could not find FFTW, and since the empty install still counted as installed, nothing ever rebuilt it. After an MFC target is configured, compare the compilers in its CMakeCache.txt with each installed dependency's (resolving symlinks); rebuild any that differ and reconfigure the target. If configure itself fails, the cache still records the compilers, so do the same check there, also rebuilding dependencies that a system library satisfied (empty install), and retry once. --sys-* dependencies and --no-build are left alone.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 3
Open (4)
_all_dependenciesdoes recursive recomputation (_all_dependencies(dep)for each edge) and uses… · Newcase=Noneinto_stale_dependencies, but inbuild.pythe function is… · New_stale_dependencies, which is a private helper by naming convention. If this… · New
What changed in this PR
This PR improves the build system’s robustness when switching toolchains/environments by detecting and rebuilding dependencies that were previously “installed” under incompatible compiler caches or accidentally satisfied by system libraries.
Changes:
- Detect compiler toolchain mismatches via
CMakeCache.txtand rebuild affected (including transitive) dependencies before continuing. - Detect “system-found” dependencies that installed nothing on configure failure and rebuild them from source, then retry configure.
- Add unit tests covering cache parsing, symlink resolution, transitive dependency checks, and system-found detection behavior.
| File | Description |
|---|---|
| toolchain/mfc/build.py | Adds cache-based compiler detection and dependency rebuild logic when toolchains mismatch or dependencies were system-found. |
| toolchain/mfc/test_dependency_toolchain.py | Adds tests validating stale-dependency detection, including transitive and system-found scenarios. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for dep in target.requires.compute(): | ||
| for d in _all_dependencies(dep) + [dep]: | ||
| if d.isDependency and d not in deps: | ||
| deps.append(d) |
There was a problem hiding this comment.
Fixed in the latest commit: now an iterative walk with a visited set. The graph is about 6 static nodes, so performance never mattered, but __build_target guards cycles with history and this helper shouldn't be the one place a cycle hangs. Added a cycle test.
| def test_symlinked_paths_to_one_compiler_match(tmp_path): | ||
| real = tmp_path / "amdllvm" | ||
| real.write_text("") | ||
| link = tmp_path / "amdflang" | ||
| link.symlink_to(real) | ||
| assert compiler_mismatches({"Fortran": str(real)}, {"Fortran": str(link)}) == [] |
There was a problem hiding this comment.
Not changing: MFC's toolchain is POSIX-only (mfc.sh is bash), toolchain CI runs on Ubuntu and macOS, and test_preflight.py already uses symlink_to.
| write_cache(fftw.get_staging_dirpath(None), fortran=GFORTRAN) | ||
| write_cache(post.get_staging_dirpath(None), fortran=GFORTRAN) | ||
|
|
||
| stale = _stale_dependencies(post, None, include_system_found=False) |
There was a problem hiding this comment.
Not changing: the repo runs no type checker. These helpers only pass case through to the target's methods, and the test fakes ignore it. In production case is always an MFCInputFile, so marking it Optional would describe the code incorrectly.
|
|
||
| import os | ||
|
|
||
| from mfc.build import _stale_dependencies, cmake_cache_compilers, compiler_mismatches |
There was a problem hiding this comment.
Keeping the underscore: the helper is internal to build.py, and toolchain tests already import private helpers when they test internal behavior (e.g. test_coverage_unit.py imports _env_without_git).
Address review: _all_dependencies recursed without cycle protection. The graph is small and acyclic today, but __build_target guards cycles with its history set, and this helper should not be the one place a cycle hangs the build.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1935 +/- ##
=======================================
Coverage 62.82% 62.82%
=======================================
Files 86 86
Lines 22394 22394
Branches 3305 3305
=======================================
Hits 14070 14070
Misses 6071 6071
Partials 2253 2253 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Problem
Dependencies are installed under their name alone (
build/{staging,install}/<dep>), so a dependency built in one environment is reused unchanged in another. Two failures this causes:amdflangget reused after switching to a GNU toolchain, andpost_processfails to link (undefinedFIRModulesymbols fromliblapack.a).find_packagefinds a system library (e.g. an FFTW onPATH), it installs nothing. The empty install still counts as installed, since dependency install manifests are always empty. In an environment where that library isn't visible,simulationcan't find FFTW, and nothing ever rebuilds it.Change
In
__build_target, for MFC targets:CMAKE_{Fortran,C,CXX}_COMPILERin the target'sCMakeCache.txtagainst each installed dependency's, including transitive dependencies, with symlinks resolved. Any mismatched dependency is wiped and rebuilt, and the target is reconfigured.--sys-*dependencies, uninstalled dependencies and--no-buildare left alone. When toolchains match, the only cost is reading a few cache files.Testing
On HPC Fund, using dependency caches left by real builds:
amdflang-built HDF5, Silo and LAPACK, then a GNU./mfc.sh build -t post_process: all three are rebuilt andpost_processlinks.-t post_process simulation: FFTW is built from source and both targets build.toolchain/mfc/test_dependency_toolchain.py;./mfc.sh precheckpasses.Limitation: a system library found by the same compiler in a different environment is caught only when the dependent target's configure fails.
Acknowledgement