Skip to content

Conversation

@sbryngelson
Copy link
Member

@sbryngelson sbryngelson commented Nov 12, 2025

PR Type

Documentation


Description

  • Update Homebrew wrapper documentation to reflect shorthand syntax support

  • Document that both mfc case.py and mfc run case.py commands work

  • Clarify that shorthand version auto-detects case files

  • Simplify notes about Homebrew wrapper capabilities


Diagram Walkthrough

flowchart LR
  A["Documentation Files"] -->|"Update command examples"| B["README.md"]
  A -->|"Update getting-started guide"| C["docs/documentation/getting-started.md"]
  A -->|"Update running guide"| D["docs/documentation/running.md"]
  A -->|"Update packaging docs"| E["packaging/homebrew/README.md"]
  B -->|"Reflect"| F["Shorthand syntax support"]
  C -->|"Reflect"| F
  D -->|"Reflect"| F
  E -->|"Reflect"| F
Loading

File Walkthrough

Relevant files
Documentation
README.md
Update primary command example to shorthand syntax             

README.md

  • Changed primary example from mfc run case.py to mfc case.py
  • Added note that both command syntaxes work
  • Updated documentation to reflect shorthand auto-detection capability
+2/-2     
getting-started.md
Update getting-started guide with shorthand syntax             

docs/documentation/getting-started.md

  • Updated example command from mfc run case.py to mfc case.py
  • Added bullet point clarifying both syntaxes work with auto-detection
  • Separated developer commands note for clarity
+3/-2     
running.md
Add shorthand syntax documentation to running guide           

docs/documentation/running.md

  • Added shorthand syntax example mfc
  • Included explicit mfc run syntax as alternative
  • Updated note to clarify both syntaxes are supported
+3/-1     
README.md
Update Homebrew packaging documentation with shorthand syntax

packaging/homebrew/README.md

  • Updated example from mfc run case.py to mfc case.py
  • Added shorthand syntax with explicit alternative
  • Clarified that both syntaxes work with auto-detection
  • Updated note to reflect wrapper supports case running only
+5/-3     

Note

Updates Homebrew-related docs to show mfc <case.py> shorthand works alongside mfc run <case.py> and clarifies wrapper vs. developer commands.

  • Documentation (Homebrew usage):
    • README updates in README.md, docs/documentation/getting-started.md, docs/documentation/running.md, packaging/homebrew/README.md.
    • Note that both mfc <case.py> and mfc run <case.py> are supported (shorthand auto-detects case files).
    • Clarify that developer commands (build, test, clean, etc.) require cloning the repo and using ./mfc.sh.
    • Adjust quick-start examples to use mfc <case.py> -n 2; retain explicit mfc run ... as an alternative.

Written by Cursor Bugbot for commit a6d2641. Configure here.

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 <[email protected]>
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 [email protected] 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
- Use shorthand 'mfc case.py' consistently across all documentation
- Clarify that both 'mfc case.py' and 'mfc run case.py' work
- Update README.md, packaging/homebrew/README.md, getting-started.md, and running.md
- Improves consistency and showcases the smart detection feature

Addresses Copilot AI review comment about documentation inconsistency
@sbryngelson sbryngelson marked this pull request as ready for review November 12, 2025 16:42
@sbryngelson sbryngelson requested a review from a team as a code owner November 12, 2025 16:42
Copilot AI review requested due to automatic review settings November 12, 2025 16:42
@qodo-merge-pro
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Consistency

Multiple docs now claim both mfc case.py and mfc run case.py work. Verify the Homebrew wrapper actually supports the shorthand invocation in all environments (path detection, file auto-detect) to avoid user confusion.

brew install mflowcode/mfc/mfc
mkdir -p ~/mfc_quickstart && cd ~/mfc_quickstart
cp $(brew --prefix mfc)/examples/1D_sodshocktube/case.py .
mfc case.py -n 2

Use -n X to select the number of MPI processes. Both mfc case.py and mfc run case.py work. For developer commands (build, test, etc.), clone the repo and use ./mfc.sh.


</details>

<details><summary><a href='https://github.com/MFlowCode/MFC/pull/1039/files#diff-ac1cb40c48f1535400ea846a15de7f139377474136ba2a611ac03e0d55da8e19R13-R23'><strong>Duplication</strong></a>

Guidance about shorthand versus explicit `run` is repeated across README, getting-started, running, and packaging docs; consider extracting a single canonical note to avoid drift.
</summary>

```markdown
If you installed MFC via Homebrew, run cases with the `mfc` wrapper:

```bash
mfc <path/to/case.py> -n 2
# or explicitly:
mfc run <path/to/case.py> -n 2
  • Use -n X to control the number of MPI processes (ranks).
  • Both mfc case.py and mfc run case.py work (shorthand auto-detects case files).
  • To use developer commands (build, test, clean, etc.), clone the repository and use ./mfc.sh.

</details>

</td></tr>
</table>

Copilot finished reviewing on behalf of sbryngelson November 12, 2025 16:44
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 updates documentation across multiple files to introduce a shorthand syntax for the Homebrew MFC wrapper, allowing users to run mfc case.py directly instead of requiring mfc run case.py.

Key changes:

  • Updated command examples from mfc run case.py to mfc case.py (shorthand syntax)
  • Added explanatory notes that both syntaxes work, with shorthand auto-detecting case files
  • Clarified that only running cases is supported in the Homebrew wrapper (refined from "only mfc run")

Reviewed Changes

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

File Description
packaging/homebrew/README.md Updated quick start and usage examples to showcase shorthand syntax; clarified wrapper limitations
docs/documentation/running.md Updated Homebrew usage section to document both command syntaxes
docs/documentation/getting-started.md Updated Homebrew quick start example and notes to reflect shorthand syntax
README.md Updated macOS quick start section with shorthand syntax example

- Remove smart detection logic - wrapper now only accepts 'mfc <case.py>'
- Simplify help text and error handling
- Update all documentation to remove mentions of 'mfc run'
- Cleaner UX: one way to do things, no confusion

The wrapper always prepends 'run' internally, but users only need to
know the simple 'mfc <case.py>' syntax.
@sbryngelson sbryngelson requested a review from Copilot November 12, 2025 16:48
@sbryngelson
Copy link
Member Author

/improve

Comment on lines 117 to 121
# Always prepend "run" since this wrapper only supports running cases
ARGS=("run" "${ARGS[@]}")
# Create a temporary working directory (Cellar is read-only)
TMPDIR="$(mktemp -d)"
Copy link
Contributor

@qodo-merge-pro qodo-merge-pro bot Nov 12, 2025

Choose a reason for hiding this comment

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

Suggestion: Add validation to ensure the first non-flag argument is a Python case file before prepending the run command, providing clearer error messages for invalid input. [general, importance: 7]

New proposed code:
 # Always prepend "run" since this wrapper only supports running cases
+# Validate that the first non-flag arg looks like a case file for clearer errors
+first_nonflag=""
+for a in "${ARGS[@]}"; do
+  if [[ "$a" != -* ]]; then
+    first_nonflag="$a"
+    break
+  fi
+done
+
+if [[ -z "${first_nonflag}" ]]; then
+  echo "mfc (Homebrew): missing case file. Usage: mfc <case.py> [options]"
+  exit 2
+fi
+
+if [[ ! "${first_nonflag}" =~ \.py$ ]] && [[ ! -f "${first_nonflag}" ]]; then
+  echo "mfc (Homebrew): first non-flag argument must be a Python case file."
+  echo "Given: ${first_nonflag}"
+  exit 2
+fi
+
 ARGS=("run" "${ARGS[@]}")
 
 # Create a temporary working directory (Cellar is read-only)
 TMPDIR="$(mktemp -d)"
 trap 'rm -rf "${TMPDIR}"' EXIT

Copilot finished reviewing on behalf of sbryngelson November 12, 2025 16:50
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

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

Address AI review feedback: prevent 'mfc run run case.py' issue by
detecting and rejecting the 'run' subcommand with a helpful error message.

If users try 'mfc run case.py', they now get:
  mfc (Homebrew): The 'run' command is not needed.
  Usage: mfc <case.py> [options]
  Example: mfc case.py -n 2
Address qodo-merge-pro feedback:
- Add --version/-v flag handling to show version at wrapper level
- Validate that first non-flag argument is a Python case file
- Provide clearer error messages for common mistakes:
  * Missing case file
  * Invalid case file (not .py or doesn't exist)
  * Using 'mfc run' syntax

This prevents confusing errors from being passed to the underlying
mfc.sh and gives users immediate, actionable feedback.
Remove redundant backslash in .py regex pattern
@sbryngelson sbryngelson requested a review from Copilot November 12, 2025 17:18
@sbryngelson
Copy link
Member Author

/improve

Copilot finished reviewing on behalf of sbryngelson November 12, 2025 17:19
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

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

sbryngelson and others added 3 commits November 12, 2025 12:21
Co-authored-by: qodo-merge-pro[bot] <151058649+qodo-merge-pro[bot]@users.noreply.github.com>
- Remove redundant backslash escape in .py regex
- Change validation from AND to OR (more correct logic)
- Improve error message to mention 'readable' file
Address Copilot feedback: Move 'run' check after finding first_nonflag
so that 'mfc -n 2 run case.py' is properly detected and rejected.

Previously checked ARGS[0] which would miss 'run' if flags came first.
Now correctly finds and validates the first non-flag argument.
@sbryngelson sbryngelson merged commit 9027349 into MFlowCode:master Nov 12, 2025
20 checks passed
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