-
Notifications
You must be signed in to change notification settings - Fork 121
Homebrew formula #1046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Homebrew formula #1046
Conversation
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
- 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.
Catch up with latest changes from upstream
- 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
- 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.
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
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.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High-level Suggestion
Reintroduce the brew audit step into the homebrew.yml CI workflow. This ensures the Homebrew formula is validated for correctness, style, and checksums, which was a quality check removed during simplification. [High-level, importance: 8]
Solution Walkthrough:
Before:
# .github/workflows/homebrew.yml
jobs:
homebrew-mfc:
runs-on: macos-latest
steps:
- name: Checkout repo
...
- name: Set up Homebrew
...
- name: Install MFC via Homebrew
run: |
brew install mflowcode/mfc/mfc || brew install --build-from-source mflowcode/mfc/mfc
- name: Run MFC test case
...
- name: Basic installation verification
...
After:
# .github/workflows/homebrew.yml
jobs:
homebrew-mfc:
runs-on: macos-latest
steps:
- name: Checkout repo
...
- name: Set up Homebrew
...
- name: Validate formula
run: |
brew audit --online packaging/homebrew/mfc.rb
- name: Install MFC via Homebrew
run: |
brew install mflowcode/mfc/mfc || brew install --build-from-source mflowcode/mfc/mfc
- name: Run MFC test case
...
- name: Basic installation verification
...
| # Require a readable Python file (not a directory) | ||
| if [[ ! "${first_nonflag}" =~ .py$ ]] || [[ ! -f "${first_nonflag}" ]]; then | ||
| echo "mfc (Homebrew): first argument must be a readable Python case file." | ||
| echo "Given: ${first_nonflag}" | ||
| echo "Usage: mfc <case.py> [options]" | ||
| exit 2 | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Correct the regex for Python file validation by escaping the dot in \\.py$ to ensure it matches a literal dot instead of any character. [possible issue, importance: 6]
| # Require a readable Python file (not a directory) | |
| if [[ ! "${first_nonflag}" =~ .py$ ]] || [[ ! -f "${first_nonflag}" ]]; then | |
| echo "mfc (Homebrew): first argument must be a readable Python case file." | |
| echo "Given: ${first_nonflag}" | |
| echo "Usage: mfc <case.py> [options]" | |
| exit 2 | |
| fi | |
| # Require a readable Python file (not a directory) | |
| if [[ ! "${first_nonflag}" =~ \.py$ ]] || [[ ! -f "${first_nonflag}" ]]; then | |
| echo "mfc (Homebrew): first argument must be a readable Python case file." | |
| echo "Given: ${first_nonflag}" | |
| echo "Usage: mfc <case.py> [options]" | |
| exit 2 | |
| fi |
There was a problem hiding this 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 simplifies the Homebrew formula wrapper syntax by removing the need for the explicit run subcommand. Users can now execute cases directly with mfc <case.py> instead of mfc run <case.py>.
Key changes:
- Simplified command syntax:
mfc <case.py>replacesmfc run <case.py> - Enhanced argument validation with better error messages for common mistakes
- Streamlined CI workflow with faster, more focused tests
- Consistent documentation updates across all files
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packaging/homebrew/mfc.rb | Refactored wrapper script to auto-prepend 'run', added robust validation for case files, and improved help/version handling |
| packaging/homebrew/README.md | Updated usage examples to reflect simplified syntax without 'run' command |
| docs/documentation/running.md | Updated Homebrew usage section with simplified command syntax |
| docs/documentation/getting-started.md | Updated quick start examples to use simplified syntax |
| README.md | Updated macOS quick start example to use simplified syntax |
| .github/workflows/homebrew.yml | Simplified workflow from multi-job validation to single focused test job with installation verification |
| .github/workflows/deploy-tap.yml | Minor cleanup: removed obsolete branch trigger, improved quoting, and removed trailing whitespace |
| fi | ||
| # Require a readable Python file (not a directory) | ||
| if [[ ! "${first_nonflag}" =~ .py$ ]] || [[ ! -f "${first_nonflag}" ]]; then |
Copilot
AI
Nov 15, 2025
There was a problem hiding this comment.
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 an escape for the literal dot. It should be \\.py$ to match the literal '.py' extension, consistent with line 90 which correctly uses \\. in its pattern.
Without the escape, this pattern matches any character followed by 'py' at the end (e.g., 'xpy', '1py'), not specifically '.py' files.
| if [[ ! "${first_nonflag}" =~ .py$ ]] || [[ ! -f "${first_nonflag}" ]]; then | |
| if [[ ! "${first_nonflag}" =~ \.py$ ]] || [[ ! -f "${first_nonflag}" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 8
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| echo "Usage: mfc <case.py> [options]" | ||
| echo "Example: mfc case.py -n 2" | ||
| exit 2 | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Wrapper Argument Rejection Breaks Formula Tests
The wrapper now rejects run as the first non-flag argument (lines 141-146), but the test do block at line 303 still calls system bin/"mfc", "run", "case.py", "-j", "1". This causes the formula's test to fail because the wrapper exits with error code 2 when it detects run, even though the test is otherwise valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| on: | ||
| push: | ||
| branches: [ main, master, homebrew-formula ] | ||
| paths: | ||
| - 'packaging/homebrew/mfc.rb' | ||
| - 'packaging/homebrew/README.md' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Limit deploy workflow to protected branches
The deploy-tap workflow no longer restricts which branches can trigger it—on.push now runs for any branch that happens to modify packaging/homebrew/**. Because the job always ends by committing and pushing the formula to MFlowCode/homebrew-mfc using the PAT, any contributor pushing a feature branch (or even a draft PR) will now publish their unreviewed formula straight to the public tap. Previously this workflow ran only after changes landed on main/master/homebrew-formula, guaranteeing review before publishing. Please reinstate a branch filter (or otherwise guard the job) so that only vetted branches can trigger the tap sync.
Useful? React with 👍 / 👎.
| on: | ||
| push: | ||
| branches: [ main, master, homebrew-formula ] | ||
| paths: | ||
| - 'packaging/homebrew/mfc.rb' | ||
| - 'packaging/homebrew/README.md' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tag releases never trigger deploy workflow
A new tag-specific path was added (if: github.ref_type == 'tag'), but the workflow still declares a paths: filter under on.push. GitHub only evaluates the paths filter against files changed in the pushed commits, and tag pushes do not carry any file changes, so the workflow will never trigger for refs/tags/v*.*.*. As a result the “Update formula (for tag events)” step and the subsequent tap sync never run when creating a release tag, breaking the intended automation. Remove the paths restriction for tag events (or split them into a separate trigger) so that release tags actually invoke this workflow.
Useful? React with 👍 / 👎.
User description
bottle
PR Type
Enhancement, Bug fix
Description
Simplify Homebrew wrapper syntax from
mfc run <case.py>tomfc <case.py>Improve argument parsing with better validation and error handling
Add
--versionflag support to wrapper commandStreamline CI/CD workflows by removing redundant smoke tests and consolidating to single macOS job
Update documentation and examples across multiple files to reflect simplified syntax
Diagram Walkthrough
File Walkthrough
mfc.rb
Simplify wrapper syntax and enhance argument validationpackaging/homebrew/mfc.rb
mfc runcommand requirement; wrapper now auto-detects casefiles and prepends
runinternallySUBCMDvariable with direct${ARGS[0]-}checks for cleanerlogic
--versionflag support to display MFC versionfiles, rejects
mfc runsyntax, validates Python file existencemfc case.pysyntaxinstead of
mfc run case.pyhomebrew.yml
Consolidate and simplify CI/CD workflow.github/workflows/homebrew.yml
macOS test job
and pull requests
homebrew-formulabranch from triggersbrew installwith fallback to sourcebuild
set-euo pipefaildeploy-tap.yml
Fix workflow triggers and shell quoting.github/workflows/deploy-tap.yml
homebrew-formulabranch from trigger conditions (now onlyresponds to tag events and path changes)
"$GITHUB_OUTPUT"instead of$GITHUB_OUTPUTREADME.md
Update Homebrew example syntaxREADME.md
mfc run case.py -n 2tomfccase.py -n 2getting-started.md
Update getting started guide with simplified syntaxdocs/documentation/getting-started.md
mfc run case.py -n 2tomfc case.py -n 2mfcto run cases"running.md
Update running guide with simplified command syntaxdocs/documentation/running.md
mfc runtomfcruncommandREADME.md
Update Homebrew README with simplified syntaxpackaging/homebrew/README.md
mfc run case.py -n 2tomfc case.py -n 2mfc runtomfc"only
mfc run"