Skip to content

Fix Homebrew install: #1355

Merged
sbryngelson merged 2 commits into
MFlowCode:masterfrom
sbryngelson:fix/homebrew-fortran-constants
Apr 5, 2026
Merged

Fix Homebrew install: #1355
sbryngelson merged 2 commits into
MFlowCode:masterfrom
sbryngelson:fix/homebrew-fortran-constants

Conversation

@sbryngelson
Copy link
Copy Markdown
Member

add fallback constants when m_constants.fpp is unavailable

The Homebrew formula only ships toolchain/ and examples/, but not src/. The toolchain reads Fortran constants from src/common/m_constants.fpp at import time, causing a RuntimeError in Homebrew installs. Add fallback values for the 4 required constants so the toolchain works without the Fortran source tree.

AI code reviews

Reviews are not triggered automatically. To request a review, comment on the PR:

  • @coderabbitai review — incremental review (new changes only)
  • @coderabbitai full review — full review from scratch
  • /review — Qodo review
  • /improve — Qodo code suggestions
  • @claude full review — Claude full review (also triggers on PR open/reopen/ready)
  • Add label claude-full-review — Claude full review via label

…unavailable

The Homebrew formula only ships toolchain/ and examples/ but not src/.
The toolchain reads Fortran constants from src/common/m_constants.fpp at
import time, causing a RuntimeError in Homebrew installs. Add fallback
values for the 4 required constants so the toolchain works without the
Fortran source tree.
@sbryngelson sbryngelson added the claude-full-review Trigger Claude Code review label Apr 5, 2026
@sbryngelson sbryngelson marked this pull request as ready for review April 5, 2026 16:57
@sbryngelson sbryngelson merged commit cf13fa1 into MFlowCode:master Apr 5, 2026
35 of 65 checks passed
@sbryngelson sbryngelson deleted the fix/homebrew-fortran-constants branch April 5, 2026 16:57
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Add fallback Fortran constants for Homebrew compatibility

🐞 Bug fix ✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add fallback Fortran constants for Homebrew installations
• Prevents RuntimeError when src/common/m_constants.fpp unavailable
• Maintains compatibility with incomplete Homebrew package distribution
• Includes 4 required constants with documented sync requirement
Diagram
flowchart LR
  A["get_fortran_constants()"] --> B["Parse m_constants.fpp"]
  B --> C{"Constants found?"}
  C -->|Yes| D["Return parsed constants"]
  C -->|No| E["Use _FALLBACK_CONSTANTS"]
  E --> F["Return fallback values"]
Loading

Grey Divider

File Changes

1. toolchain/mfc/params/namelist_parser.py 🐞 Bug fix +14/-1

Add fallback constants for missing Fortran source

• Updated get_fortran_constants() docstring to clarify fallback behavior for Homebrew installs
• Added fallback logic to use _FALLBACK_CONSTANTS when parsing returns empty dict
• Introduced _FALLBACK_CONSTANTS dictionary with 4 required constants: num_fluids_max,
 num_probes_max, num_patches_max, num_bc_patches_max
• Added comment noting fallback values must stay synchronized with src/common/m_constants.fpp

toolchain/mfc/params/namelist_parser.py


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Apr 5, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

Grey Divider


Remediation recommended

1. Fallback masks parse failures 🐞 Bug ≡ Correctness
Description
get_fortran_constants() falls back to hardcoded values whenever parse_fortran_constants() returns an
empty dict, which happens both when src/common/m_constants.fpp is missing and when the parser fails
to match anything. This can silently hide toolchain/Fortran drift that params/definitions.py
explicitly intends to fail fast on, potentially leading to incorrect registry bounds and validation
limits.
Code

toolchain/mfc/params/namelist_parser.py[R501-502]

+        if not _FORTRAN_CONSTANTS_CACHE:
+            _FORTRAN_CONSTANTS_CACHE = _FALLBACK_CONSTANTS.copy()
Evidence
parse_fortran_constants() returns an empty dict both on FileNotFoundError and when the regex finds
no matches, and get_fortran_constants() treats any empty dict as “source unavailable” and silently
substitutes fallback values. Meanwhile, params/definitions.py is designed to raise a RuntimeError
when required constants are missing to ensure the toolchain stays in sync with Fortran; the new
behavior can defeat that safety check in some failure modes.

toolchain/mfc/params/namelist_parser.py[467-482]
toolchain/mfc/params/namelist_parser.py[489-503]
toolchain/mfc/params/definitions.py[15-24]
src/common/m_constants.fpp[19-28]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`get_fortran_constants()` currently falls back to `_FALLBACK_CONSTANTS` whenever `parse_fortran_constants()` returns an empty dict. Since `parse_fortran_constants()` returns `{}` both when the file is missing and when parsing finds no matches, this can silently mask real parsing failures / format drift.

### Issue Context
The toolchain uses these constants at import time in `params/definitions.py` and is intended to fail fast when constants can’t be parsed (to stay in sync with Fortran). The fallback should apply only when the Fortran source tree is genuinely unavailable.

### Fix Focus Areas
- toolchain/mfc/params/namelist_parser.py[467-503]
- toolchain/mfc/params/definitions.py[15-24]

### Suggested fix
1. In `parse_fortran_constants`, return `None` (or raise a dedicated exception) on `FileNotFoundError`, and return a dict (possibly empty) only for successful reads.
2. In `get_fortran_constants`, fall back only when the file is missing (`None` / explicit missing condition). If the file exists but parsing yields no constants, raise a clear `RuntimeError` indicating the parser is out of sync with `m_constants.fpp` (or at least emit a warning).
3. (Optional) Add a small unit test that simulates: (a) missing file => fallback, (b) existing file with non-matching content => error/warn.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 5, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a77141be-d8c1-4cb1-8f30-438c3ce3a6b9

📥 Commits

Reviewing files that changed from the base of the PR and between fbaa8c6 and c4ad2ea.

📒 Files selected for processing (1)
  • toolchain/mfc/params/namelist_parser.py

📝 Walkthrough

Walkthrough

The get_fortran_constants() function in the namelist parser module is updated to include a fallback mechanism. When parsing the Fortran constants file fails or returns no results, the function now uses a predefined fallback constants dictionary containing num_fluids_max, num_probes_max, num_patches_max, and num_bc_patches_max instead of returning empty. The function signature remains unchanged, and the docstring is updated to document this fallback behavior.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.88%. Comparing base (cf81f25) to head (c4ad2ea).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1355      +/-   ##
==========================================
+ Coverage   64.85%   64.88%   +0.03%     
==========================================
  Files          70       70              
  Lines       18228    18249      +21     
  Branches     1506     1507       +1     
==========================================
+ Hits        11821    11841      +20     
  Misses       5446     5446              
- Partials      961      962       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-full-review Trigger Claude Code review

Development

Successfully merging this pull request may close these issues.

1 participant