Skip to content

Conversation

@sbryngelson
Copy link
Member

@sbryngelson sbryngelson commented Nov 11, 2025

PR Type

Enhancement, Documentation


Description

  • Add shorthand syntax for MFC command execution

    • Auto-detect case files and prepend 'run' command
    • Support both mfc case.py and mfc run case.py forms
  • Update help text and examples with new syntax

  • Improve user experience with simplified command interface

  • Update CI workflow to use new shorthand syntax


Diagram Walkthrough

flowchart LR
  A["User Input Detection"] -->|Case file detected| B["Auto-prepend 'run'"]
  A -->|Explicit 'run' command| C["Execute normally"]
  B --> D["Execute MFC"]
  C --> D
  D --> E["Run simulation"]
Loading

File Walkthrough

Relevant files
Enhancement
mfc.rb
Implement auto-detection for shorthand MFC syntax               

packaging/homebrew/mfc.rb

  • Add shorthand syntax mfc to help text alongside explicit mfc run
  • Implement smart detection logic to auto-prepend 'run' when first
    argument is a Python file or existing file
  • Update usage examples to demonstrate new shorthand syntax with options
  • Simplify caveats section to highlight shorthand form as primary usage
    method
+14/-5   
homebrew.yml
Update CI workflow to use shorthand syntax                             

.github/workflows/homebrew.yml

  • Update test case execution to use new shorthand syntax mfc
    "$TESTDIR/case.py"
  • Update comment to reflect auto-detection behavior of wrapper
+2/-2     

sbryngelson and others added 30 commits November 4, 2025 11:56
This commit adds package management infrastructure:

- Homebrew formula (mfc.rb) for macOS installation
- Spack package (package.py) for HPC environments
- Comprehensive documentation for package usage
- GitHub issue templates for better user support
- Star growth tracking and analytics

This enables easier installation and broader adoption of MFC.

Co-authored-by: AI Assistant <assistant@anthropic.com>
Replaced 5 separate documentation files with one comprehensive HOMEBREW.md that covers:
- Formula structure and dependencies
- Build process and installation
- Testing and validation
- Usage examples and post-installation
- Distribution methods
- Technical details and platform support
These files are general project status/summary documents, not specific
to the Homebrew formula. The homebrew-formula branch should only contain:
- mfc.rb (the formula)
- HOMEBREW.md (Homebrew-specific documentation)
Moved documentation updates to dedicated documentation-updates branch.
This branch now focuses solely on Homebrew packaging.
Organized Homebrew formula files into dedicated directory structure:
- mfc.rb -> packaging/homebrew/mfc.rb
- HOMEBREW.md -> packaging/homebrew/HOMEBREW.md

This matches the Spack organization and provides a unified packaging/
directory for all package management systems.
Synced README with documentation-updates branch which contains
the most current version from upstream/master.
…1022)

Co-authored-by: qodo-merge-pro[bot] <151058649+qodo-merge-pro[bot]@users.noreply.github.com>
Fixed three issues identified by AI PR reviewer:

1. **Duplicate Code**: Removed redundant mfc.sh installation that was
   immediately overwritten. Now mfc.sh is installed once to prefix.

2. **Path Reference**: Fixed wrapper script to correctly reference
   mfc.sh from prefix directory where it's actually installed.

3. **Enhanced Tests**: Added functional tests that verify:
   - Binaries can execute (test -h flag)
   - mfc.sh and toolchain are properly installed
   - All components are accessible

Changes ensure the formula works correctly and tests verify actual
functionality beyond just file existence.
Addressed three additional suggestions from AI PR reviewer:

1. **Use libexec for mfc.sh** (Importance: 9)
   - Changed from prefix.install to libexec.install for mfc.sh
   - Updated wrapper to reference #{libexec}/mfc.sh
   - Follows Homebrew best practices for executable scripts

2. **Remove hardcoded compiler variables** (Importance: 7)
   - Removed ENV["FC"], ENV["CC"], ENV["CXX"] settings
   - Removed ENV["BOOST_INCLUDE"] from build step
   - Let Homebrew's superenv handle compiler setup via gcc dependency
   - Kept BOOST_INCLUDE only in wrapper script where needed at runtime

3. **Enhanced functional testing** (Importance: 6)
   - Added full toolchain test: runs actual example case
   - Tests 'mfc run' with 1D_sodshocktube example
   - Verifies entire workflow works end-to-end
   - Updated assertions to check libexec/mfc.sh location

These changes make the formula more idiomatic and robust.
Fixed remaining issues identified by Copilot:

1. **Directory check issue** (Critical)
   - mfc.sh expects to be run from MFC's root (checks toolchain/util.sh)
   - Updated wrapper to 'cd' to installation prefix before exec
   - Added comment explaining why directory change is needed
   - This ensures mfc.sh finds toolchain/util.sh at runtime

2. **Hardcoded version in documentation** (Documentation)
   - Removed hardcoded '5.1.0' references from HOMEBREW.md
   - Changed to generic language: 'When a new MFC version is released'
   - Updated technical details to show example pattern: vVERSION
   - Prevents documentation from becoming outdated with each release

Issues 1 & 2 (duplicate code and path reference) were already fixed
in previous commits (now using libexec.install without duplication).

The wrapper now works correctly:
  cd #{prefix} && exec #{libexec}/mfc.sh

This allows mfc.sh to find its expected directory structure.
Fixed two remaining issues from Copilot review:

1. **Document toolchain installation rationale**
   - Added detailed comment explaining why full toolchain is needed
   - Documents dependencies: util.sh, main.py, bootstrap/, templates/
   - Clarifies this is not unnecessary bloat but required functionality

2. **Add syntax highlighting to code blocks**
   - Added 'bash' language identifier to all shell command blocks
   - Improves documentation readability
   - Fixed blocks at lines 62, 74, 83, 137

3. **Update environment documentation**
   - Removed outdated references to hardcoded compiler variables
   - Updated to reflect reliance on Homebrew's superenv
   - Clarified BOOST_INCLUDE is set in wrapper, not during build

All documentation now accurately reflects the current implementation
and provides proper syntax highlighting for better user experience.
Addressed two suggestions from qodo-merge-pro review:

1. **Add explicit error handling to wrapper** (Importance: 2)
   - Changed: cd "#{prefix}" && exec
   - To: cd "#{prefix}" || exit 1; exec
   - Ensures script fails fast if directory change fails
   - Prevents execution in wrong directory if prefix is missing

2. **Use lighter test command** (Importance: 7)
   - Changed: mfc run (full simulation)
   - To: mfc count (case file parsing only)
   - Much faster test execution
   - More reliable (doesn't depend on simulation completing)
   - Still validates: wrapper, toolchain, case parsing, Python env
   - Reduces test time from minutes to seconds

Both changes improve robustness and testing efficiency.
Added GitHub Actions workflow to test Homebrew formula on macOS:

**Triggers:**
- Push to master or homebrew-formula branches
- PRs to master
- Changes to packaging/homebrew/ or workflow file
- Manual workflow dispatch

**Test Steps:**
1. Set up Homebrew and update
2. Install all MFC dependencies
3. Validate formula syntax (audit + style check)
4. Install MFC from local formula (build from source)
5. Run Homebrew's built-in formula tests
6. Test MFC commands:
   - mfc wrapper (--help)
   - All three binaries (-h flags)
   - Case file parsing (mfc count)
7. Verify installation structure
8. Clean up

**Benefits:**
- Ensures formula works on macOS
- Catches syntax errors early
- Validates installation structure
- Tests actual functionality
- Runs on every formula change

**Runner:** macos-latest (GitHub-hosted)
Fixed all 16 style violations detected by brew style:

**Required Headers:**
- Added: # typed: true (Sorbet type checking)
- Added: # frozen_string_literal: true (Ruby optimization)
- Added: Top-level documentation comment for Mfc class

**Whitespace Issues:**
- Removed all trailing whitespace (7 locations)
- Removed trailing blank lines at end of file

**Assertion Updates:**
- Changed: assert_predicate -> assert_path_exists (5 locations)
- More idiomatic for Homebrew formulas

**CI Workflow:**
- Removed brew audit with path (now disabled in Homebrew)
- Keep brew style check which catches all these issues

All offenses are now resolved. The formula passes brew style checks.
- Create temporary local tap (mflowcode/test) to satisfy Homebrew's tap requirement
- Copy formula to tap before installation
- This works around Homebrew rejecting direct file path installations
MFC's build system installs binaries to build/install/<hash>/bin/ instead
of directly to build/install/bin/. Update the formula to dynamically find
the correct installation directory using Dir.glob.
Each MFC binary (pre_process, simulation, post_process) is installed to its
own hashed subdirectory. Update the formula to search for each binary
individually instead of assuming they're all in the same directory.
Remove the mfc wrapper script and toolchain installation since:
- The wrapper requires a writable source tree, which isn't available in
  the read-only Cellar installation directory
- Users installing via Homebrew primarily need the precompiled binaries
  (pre_process, simulation, post_process)
- Users needing full development functionality (build, test, etc.) should
  clone the repository directly

Changes:
- Remove mfc wrapper script creation
- Remove toolchain and mfc.sh installation
- Update tests to only verify binary functionality
- Update caveats to reflect binary-only installation
- Update CI workflow to remove wrapper tests
MFC binaries don't support -h for help. Instead, they expect input files
and will error if those files don't exist. Update tests to only verify
that binaries exist and are executable, without trying to run them.

Changes:
- Replace system() calls with assert_predicate checks
- Update CI workflow to use which and test -x instead of running with -h
- This avoids MPI_ABORT errors when binaries look for missing input files
…e :exist?

Homebrew's style checker requires using assert_path_exists for file existence
checks instead of assert_predicate with :exist?. Keep assert_predicate only
for the :executable? checks.
Reinstates the full MFC functionality including the mfc command-line tool
by creating a smart wrapper that works around Homebrew's read-only Cellar.

Changes to formula:
- Install mfc.sh script and full Python toolchain
- Create wrapper that uses temporary working directory
- Wrapper symlinks toolchain, mfc.sh, and examples into temp dir
- Automatically cleans up temp directory on exit
- Sets proper environment variables for Homebrew installation

Changes to CI:
- Test mfc wrapper functionality (--help, count)
- Run actual test case (1D Sod shock tube) with all CPU cores
- Verify full installation structure including toolchain

This allows users to use the full mfc CLI: mfc run, mfc test, mfc count, etc.
Remove the `mfc --help` call from the formula test block since it
triggers Python virtual environment creation and dependency installation
(including cantera==3.1.0 which doesn't exist).

The formula test now only verifies:
- All binaries exist and are executable
- Toolchain and mfc.sh are installed
- Examples are installed

The CI workflow still tests full functionality including mfc --help
and mfc run, so we maintain comprehensive testing without the
formula test triggering environment setup.
Install Cantera and create a Python virtual environment during formula
installation, eliminating the need for mfc to create its own venv on
first run. This enables full functionality immediately after installation.

Changes to formula:
- Add cantera as a runtime dependency
- Move python@3.12 from build-only to runtime dependency
- Create Python venv in libexec/venv during installation
- Install MFC Python package and all dependencies into venv
- Update wrapper to activate pre-installed venv
- Link venv into build directory so mfc.sh uses existing environment
- Add venv verification to test block
- Re-enable mfc --help test since venv is ready

Changes to CI:
- Add cantera to dependency installation step
- Verify venv contains cantera and mfc packages

Benefits:
- mfc commands work immediately (no setup on first run)
- All examples including those requiring Cantera work out of the box
- Much faster execution since venv is reused
- No cantera==3.1.0 not found errors
… deps from source

Cantera 3.1.0 has CMake compatibility issues when building from source with
CMake 4.x (tries to build old yaml-cpp that requires CMake < 3.5).

Solution: Use hybrid approach:
- Install Cantera from pre-built wheel (doesn't need custom LDFLAGS)
- Set LDFLAGS and compile MFC toolchain dependencies from source with
  --no-binary :all: to ensure orjson and other C extensions have proper
  header padding for bottle relocation

This allows bottles to be created while avoiding the Cantera build failure.
Removed --no-binary :all: flag that was forcing ALL packages to compile
from source, which caused failures for Rust-based packages like 'typos'
that require Rust compiler.

New approach:
- Set LDFLAGS with -headerpad_max_install_names in environment
- Let pip decide whether to use wheels or compile from source
- Pip will use LDFLAGS when compiling C extensions (like orjson)
- Most packages will use pre-built wheels (faster, no extra deps needed)
- Only packages that need compilation will be built from source

This is simpler, faster, and avoids requiring Rust/other compilers
while still fixing the bottle relocation issues for packages with
C extensions that need custom header padding.
The issue was that orjson (a transitive dependency) was being installed
as a pre-built wheel from PyPI, which doesn't have sufficient header
padding for Homebrew's bottle relocation process.

Solution:
1. Install all dependencies normally (including wheels)
2. Force-reinstall orjson from source with LDFLAGS set
3. Use --no-deps to avoid reinstalling dependencies

This ensures orjson is compiled with -Wl,-headerpad_max_install_names
which provides the necessary header space for brew test-bot to successfully
relocate the dylib during bottling.
The formula contains a Python virtual environment with pre-built C extensions
(orjson, etc.) that cannot be reliably relocated by Homebrew's bottling process.

Attempts to force-compile these packages from source fail because:
- orjson requires Rust (maturin build tool)
- typos requires Rust
- Setting LDFLAGS only helps packages built from source, not wheels

Solution: Use 'bottle :unneeded' to tell Homebrew not to create bottles.
Users will install from source, which is appropriate for scientific computing
software like MFC. The installation process remains the same and works correctly.
…w install

Homebrew no longer supports 'bottle :unneeded'. Its presence triggered
'mfc: wrong number of arguments (given 1, expected 0)' when installing
from a temporary local tap. Removing it restores installability.

Note: Tap CI may still fail on bottle relocation for orjson unless we
add a Rust build dependency and force-build orjson from source.
- Add Homebrew quick start section to getting-started.md
- Add Homebrew usage section to running.md
- Update README with Homebrew option in Try MFC table
- Add macOS Homebrew quick start section to README
- Clarify that -n X sets the number of MPI processes
- Create packaging/homebrew/README.md with user-friendly tap documentation
- Update deploy-tap.yml to copy README to tap repository
- README includes installation, quick start, usage, and documentation links
- Low-maintenance design that stays current automatically
- Users can now run 'mfc case.py' instead of 'mfc run case.py'
- Wrapper auto-detects .py files or existing files and prepends 'run'
- Both forms work: 'mfc case.py' and 'mfc run case.py'
- Updated help text and caveats to show both usage styles
- Improved UX for the primary use case while maintaining explicit syntax
Resolved conflicts in packaging/homebrew/mfc.rb:
- Kept smart detection feature for auto-prepending 'run' command
- Preserved enhanced help messages and usage examples
- Maintained user-friendly shorthand syntax (mfc <case.py>)
Change from 'mfc run case.py' to 'mfc case.py' to verify the
smart detection feature works correctly in CI.
@sbryngelson sbryngelson marked this pull request as ready for review November 12, 2025 14:52
Copilot AI review requested due to automatic review settings November 12, 2025 14:52
@sbryngelson sbryngelson merged commit eb96476 into MFlowCode:master Nov 12, 2025
20 checks passed
@qodo-merge-pro
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The auto-prepend logic for 'run' treats any existing path or any string ending with '.py' as a case file, which could unintentionally rewrite commands or collide with future subcommands; validate behavior when the first arg is a non-existent '.py' string, a directory ending with '.py', or when additional subcommands are added later.

# Smart detection: if first arg looks like a case file, auto-prepend "run"
if [[ "${SUBCMD}" =~ .py$ ]] || [[ -f "${SUBCMD}" ]]; then
  ARGS=("run" "${ARGS[@]}")
  SUBCMD="run"
fi
UX Consistency

Help text and caveats now advertise shorthand 'mfc <case.py>' and 'mfc run <case.py>'; ensure all other docs/scripts remain consistent to avoid confusion and verify error messages guide users correctly when non-'run' subcommands are used.

  if [[ ${#ARGS[@]} -eq 0 ]] || [[ "${SUBCMD}" == "--help" ]] || [[ "${SUBCMD}" == "-h" ]]; then
    cat <<'HHELP'
MFC (Homebrew) #{version}

Usage:
  mfc <case.py> [options]
  mfc run <case.py> [options]

Examples:
  mfc case.py -n 2
  mfc run case.py -n 2 -t pre_process simulation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High-level Suggestion

The PR adds a shorthand for the run subcommand, which is inconsistent with the main developer tool's CLI. To ensure a consistent user experience, this implicit behavior should be removed, requiring the explicit mfc run <case.py> command. [High-level, importance: 7]

Solution Walkthrough:

Before:

# In mfc wrapper script
ARGS=("$@")
SUBCMD="${ARGS[0]}"

# Smart detection: if first arg looks like a case file, auto-prepend "run"
if [[ "${SUBCMD}" =~ .py$ ]] || [[ -f "${SUBCMD}" ]]; then
  ARGS=("run" "${ARGS[@]}")
  SUBCMD="run"
fi

if [[ "${SUBCMD}" != "run" ]]; then
  echo "mfc (Homebrew): only 'run' is supported..."
  exit 2
fi
# ... proceed to execute 'run' command

After:

# In mfc wrapper script
ARGS=("$@")
SUBCMD="${ARGS[0]}"

# No smart detection. 'run' must be explicit.

if [[ "${SUBCMD}" != "run" ]]; then
  echo "mfc (Homebrew): only 'run' is supported..."
  echo "Use 'mfc run <case.py>'"
  exit 2
fi
# ... proceed to execute 'run' command

Comment on lines +120 to +124
# Smart detection: if first arg looks like a case file, auto-prepend "run"
if [[ "${SUBCMD}" =~ .py$ ]] || [[ -f "${SUBCMD}" ]]; then
ARGS=("run" "${ARGS[@]}")
SUBCMD="run"
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Modify the case file detection logic to handle command-line options placed before the filename. The current implementation only checks the first argument, which fails in this common scenario. [possible issue, importance: 7]

Suggested change
# Smart detection: if first arg looks like a case file, auto-prepend "run"
if [[ "${SUBCMD}" =~ .py$ ]] || [[ -f "${SUBCMD}" ]]; then
ARGS=("run" "${ARGS[@]}")
SUBCMD="run"
fi
# Smart detection: if 'run' is not specified, find a case file and prepend 'run'
if [[ "${SUBCMD}" != "run" ]]; then
for arg in "${ARGS[@]}"; do
# If we find an argument that is a file and not an option, prepend "run"
if [[ ! "${arg}" =~ ^- ]] && ([[ "${arg}" =~ \.py$ ]] || [[ -f "${arg}" ]]); then
ARGS=("run" "${ARGS[@]}")
SUBCMD="run"
break
fi
done
fi

Copilot finished reviewing on behalf of sbryngelson November 12, 2025 14:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds smart detection to the Homebrew formula's wrapper script, allowing users to omit the run subcommand when invoking MFC with a case file (e.g., mfc case.py instead of mfc run case.py). The changes update documentation, help text, and CI workflow to reflect this new convenience feature.

Key Changes

  • Implemented auto-detection logic that prepends run when the first argument appears to be a case file
  • Updated help text, caveats, and examples to showcase the shorthand syntax
  • Modified CI workflow to test the new shorthand invocation

Reviewed Changes

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

File Description
packaging/homebrew/mfc.rb Added smart detection logic to auto-prepend run subcommand; updated help text, error messages, and caveats to document the shorthand syntax
.github/workflows/homebrew.yml Updated test to use the new shorthand syntax mfc case.py instead of mfc run case.py

fi
# Smart detection: if first arg looks like a case file, auto-prepend "run"
if [[ "${SUBCMD}" =~ .py$ ]] || [[ -f "${SUBCMD}" ]]; then
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex pattern .py$ is missing the escape for the dot, which means it will match any character followed by "py" at the end, not specifically ".py" files. The pattern should be \.py$ to properly match Python files.

This could cause false positives where strings ending with any character + "py" (e.g., "xpy", "1py") would incorrectly trigger the auto-detection.

Suggested change
if [[ "${SUBCMD}" =~ .py$ ]] || [[ -f "${SUBCMD}" ]]; then
if [[ "${SUBCMD}" =~ \.py$ ]] || [[ -f "${SUBCMD}" ]]; then

Copilot uses AI. Check for mistakes.
fi
# Smart detection: if first arg looks like a case file, auto-prepend "run"
if [[ "${SUBCMD}" =~ .py$ ]] || [[ -f "${SUBCMD}" ]]; then
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file existence check [[ -f "${SUBCMD}" ]] is too broad and will trigger auto-detection for any file, not just Python case files. This means commands like mfc myfile.txt would incorrectly be transformed to mfc run myfile.txt.

Consider making the check more specific, e.g., [[ "${SUBCMD}" =~ \.py$ ]] || [[ "${SUBCMD}" =~ \.py$ && -f "${SUBCMD}" ]], or simply rely only on the file extension check since Python case files should always end in .py.

Suggested change
if [[ "${SUBCMD}" =~ .py$ ]] || [[ -f "${SUBCMD}" ]]; then
if [[ "${SUBCMD}" =~ \.py$ ]]; then

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

1 participant