Update Makefile for Darwin #612
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds automatic MATLAB detection on macOS and warns users when MATLAB is not installed before running collectors.
- Introduce platform-specific logic to locate MATLAB on Darwin systems
- Provide user-friendly fallback warnings and instructions if MATLAB is missing
- Refactor and extend clean targets for Python caches and collected test values
Comments suppressed due to low confidence (1)
Makefile:67
- [nitpick] Target name uses an underscore while other targets use dashes (e.g.,
clean-python). Rename toclean-collected-valuesfor consistency.
clean-collected_values:
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #612 +/- ##
=======================================
Coverage 73.95% 73.95%
=======================================
Files 50 50
Lines 7000 7000
Branches 1338 1338
=======================================
Hits 5177 5177
Misses 1280 1280
Partials 543 543
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <[email protected]>
|
@coderabit-ai review |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMakefile adds macOS-aware MATLAB path detection, introduces MATLAB_PATH, and conditionally sets MATLAB. The collected values rule now tolerates missing MATLAB by creating the output directory and printing guidance. The test target depends on data collection and then runs pytest. Messaging and clean target text are adjusted. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer
participant Make as make
participant Test as test target
participant Collect as $(COLLECTED_VALUES_DIR) rule
participant ML as MATLAB (detected)
participant Py as pytest
Dev->>Make: invoke "make test"
Make->>Test: resolve dependencies
Test->>Collect: ensure collected values dir
rect rgba(230,245,255,0.5)
note over Collect: macOS: detect MATLAB_PATH under /Applications/MATLAB_R*/
alt MATLAB found and executable
Collect->>ML: run data collection
ML-->>Collect: data generated
else MATLAB missing/not executable
Collect-->>Collect: mkdir output dir
Collect-->>Dev: print guidance
end
end
Test->>Py: run pytest
Py-->>Dev: test results
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing touches🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Makefile (2)
47-54: Move multi-line user guidance out of the recipe (keep Makefile lean).Recommend relocating these instructions to README or a
helptarget and replacing with a single concise line.
41-55: Don’t rely on -x for command names; use command -v and quote MATLAB. Create the target dir up front and support pre-R2019a.
[ -x "$(MATLAB)" ]fails whenMATLAB=matlab(PATH executable), causing false “not found” and skipping collectors. Also, quoting is missing on invocation; and the directory should be created before both branches. Add a fallback for MATLAB versions lacking-batch.$(COLLECTED_VALUES_DIR): @echo "Running MATLAB script to collect values..."; \ - if [ -x "$(MATLAB)" ]; then \ - $(MATLAB) -batch "run('$(MATLAB_SCRIPT)');"; \ + mkdir -p $(COLLECTED_VALUES_DIR); \ + if command -v "$(MATLAB)" >/dev/null 2>&1 || [ -x "$(MATLAB)" ]; then \ + # Prefer -batch when available; fall back to -nodisplay/-nosplash + if "$(MATLAB)" -batch "exit" >/dev/null 2>&1; then \ + "$(MATLAB)" -batch "run('$(MATLAB_SCRIPT)');"; \ + else \ + "$(MATLAB)" -nodisplay -nosplash -r "try; run('$(MATLAB_SCRIPT)'); catch ME; disp(getReport(ME)); exit(1); end; exit"; \ + fi; \ else \ echo "MATLAB not found. Skipping MATLAB data collection."; \ echo "Some tests may fail if they depend on MATLAB data."; \ echo "To run all tests, you need to:"; \ echo "1. Install MATLAB from https://www.mathworks.com/products/matlab.html"; \ echo "2. Make sure MATLAB is in your PATH or installed in /Applications/MATLAB_R* on macOS"; \ echo "3. Run 'make clean' to clean any partial test data"; \ echo "4. Run 'make test' again"; \ echo ""; \ echo "For now, continuing with available tests..."; \ - mkdir -p $(COLLECTED_VALUES_DIR); \ fi
🧹 Nitpick comments (1)
Makefile (1)
25-26: PHONY target mismatch:.PHONYlistsrun-tests, but the target istest.Update
.PHONYto includetest(or add arun-testsalias).Outside this hunk, change:
-.PHONY: all run-examples run-tests clean-python clean-collected_values clean +.PHONY: all run-examples test clean-python clean-collected_values clean
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(2 hunks)
🔇 Additional comments (4)
Makefile (4)
11-13: LGTM on artifact dir definition.Clear and uses an absolute path; no issues.
57-59: LGTM on clean aggregation.Targets are composed cleanly.
60-65: LGTM on Python clean.Commands are safe and scoped.
66-70: LGTM on collected values clean.Removes only the intended directory.
| # Platform-specific MATLAB detection | ||
| # On macOS, automatically find the latest MATLAB installation | ||
| ifeq ($(shell uname), Darwin) | ||
| # Find the latest MATLAB version installed in /Applications | ||
| # Sort in reverse order and take the first (latest) version | ||
| MATLAB_PATH := $(shell ls -d /Applications/MATLAB_R* 2>/dev/null | sort -r | head -n 1) | ||
| ifneq ($(MATLAB_PATH),) | ||
| MATLAB := $(MATLAB_PATH)/bin/matlab | ||
| endif | ||
| endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix MATLAB detection: support PATH installs and avoid overriding user overrides.
- Current Darwin probe won’t detect
matlabavailable via PATH; it only works whenMATLABis an absolute executable path. - Also, don’t override a user-provided
MATLABvalue.
Apply this diff to the Darwin block to prefer user overrides, quote paths, and constrain to .app bundles:
ifeq ($(shell uname), Darwin)
- # Find the latest MATLAB version installed in /Applications
- # Sort in reverse order and take the first (latest) version
- MATLAB_PATH := $(shell ls -d /Applications/MATLAB_R* 2>/dev/null | sort -r | head -n 1)
- ifneq ($(MATLAB_PATH),)
- MATLAB := $(MATLAB_PATH)/bin/matlab
- endif
+ # Find the latest MATLAB .app bundle in /Applications (if user hasn't set MATLAB)
+ ifeq ($(origin MATLAB), default)
+ MATLAB_PATH := $(shell ls -d /Applications/MATLAB_R*.app 2>/dev/null | sort -r | head -n 1)
+ ifneq ($(MATLAB_PATH),)
+ MATLAB := "$(MATLAB_PATH)"/bin/matlab
+ endif
+ endif
endifAdditionally (outside this hunk), consider making the top-level defaults override-friendly:
PYTHON ?= python
MATLAB ?= matlab🤖 Prompt for AI Agents
In Makefile around lines 14 to 23, the Darwin MATLAB probe overrides
user-provided MATLAB and only detects /Applications/MATLAB_R* directories,
missing PATH-installed matlab; change the logic to first respect any existing
MATLAB variable (do not overwrite if set), set top-level defaults using ?=
(e.g., PYTHON ?= python and MATLAB ?= matlab), and in the Darwin block only run
the .app detection when MATLAB is not already set: search /Applications for
directories matching MATLAB_R*\.app, pick the latest, set MATLAB to the quoted
path to its bin/matlab, and fall back to the PATH-resolved matlab if no .app is
found so PATH installs are supported.
1 participant
This PR finds MATLAB installs on MacOS to run the MATLAB collectors and warns users if MATLAB is not installed that some tests will fail.
Summary by CodeRabbit
New Features
Tests
Chores