X Tutup
Skip to content

Unified action for installing macos deps#7379

Merged
youknowone merged 2 commits intoRustPython:mainfrom
ShaharNaveh:ci-install-macos-deps
Mar 7, 2026
Merged

Unified action for installing macos deps#7379
youknowone merged 2 commits intoRustPython:mainfrom
ShaharNaveh:ci-install-macos-deps

Conversation

@ShaharNaveh
Copy link
Contributor

@ShaharNaveh ShaharNaveh commented Mar 7, 2026

Taken and modified from https://github.com/bevyengine/bevy/blob/162a7080911dc8b782475553fef8888c8d103655/.github/actions/install-linux-deps/action.yml

Summary by CodeRabbit

Chores

  • Refactored macOS build environment setup for improved consistency and maintainability across CI/CD workflows.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 7, 2026

📝 Walkthrough

Walkthrough

Introduces a new reusable GitHub Action to install macOS build dependencies and refactors the CI workflow to use this action instead of inline brew commands, consolidating dependency setup logic in two workflow jobs.

Changes

Cohort / File(s) Summary
New macOS Dependency Action
.github/actions/install-macos-deps/action.yml
Defines a composite GitHub Action with inputs for autoconf, automake, libtool, and openssl. Conditionally installs selected packages via brew based on input flags.
CI Workflow Updates
.github/workflows/ci.yaml
Replaces inline brew install commands in rust_tests and snippets_cpython jobs with invocations of the new install-macos-deps action; adds openssl: true parameter in snippets_cpython job.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested reviewers

  • youknowone

Poem

🐰 A rabbit hops through workflows, neat,
Bundling brew commands—oh what a treat!
Dependencies dance on macOS so bright,
Reusable actions make CI just right! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Unified action for installing macos deps' accurately describes the main change: introducing a composite GitHub Action to standardize macOS dependency installation across workflows.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/actions/install-macos-deps/action.yml:
- Around line 39-47: The "Install macOS dependencies" step currently runs "brew
install" even when no formulae are selected, causing a failure; change the step
to compute the list of chosen inputs (inputs.autoconf, inputs.automake,
inputs.libtool, inputs.openssl) into a variable and skip running "brew install"
when that computed list is empty (i.e., no-op or echo a message), otherwise run
"brew install <packages>"; update the step's run script to perform this
conditional check so the action does nothing if all inputs are false.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: d572a819-438a-405a-b51f-df21ad724e9e

📥 Commits

Reviewing files that changed from the base of the PR and between abfb51e and 37b82ca.

📒 Files selected for processing (2)
  • .github/actions/install-macos-deps/action.yml
  • .github/workflows/ci.yaml

Comment on lines +39 to +47
- name: Install macOS dependencies
shell: bash
if: ${{ runner.os == 'macOS' }}
run: >
brew install
${{ fromJSON(inputs.autoconf) && 'autoconf' || '' }}
${{ fromJSON(inputs.automake) && 'automake' || '' }}
${{ fromJSON(inputs.libtool) && 'libtool' || '' }}
${{ fromJSON(inputs.openssl) && 'openssl@3' || '' }}
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 7, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Skip brew install when no packages were selected.

If a caller sets every input to false, this step still resolves to brew install with no formulae and fails. Since the action is meant to be reusable/configurable, it should no-op when the computed package list is empty.

Suggested fix
-      run: >
-        brew install
-        ${{ fromJSON(inputs.autoconf) && 'autoconf' || '' }}
-        ${{ fromJSON(inputs.automake) && 'automake' || '' }}
-        ${{ fromJSON(inputs.libtool) && 'libtool' || '' }}
-        ${{ fromJSON(inputs.openssl) && 'openssl@3' || '' }}
+      run: |
+        packages=()
+        ${{ fromJSON(inputs.autoconf) && 'packages+=("autoconf")' || '' }}
+        ${{ fromJSON(inputs.automake) && 'packages+=("automake")' || '' }}
+        ${{ fromJSON(inputs.libtool) && 'packages+=("libtool")' || '' }}
+        ${{ fromJSON(inputs.openssl) && 'packages+=("openssl@3")' || '' }}
+
+        if [ "${`#packages`[@]}" -gt 0 ]; then
+          brew install "${packages[@]}"
+        fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Install macOS dependencies
shell: bash
if: ${{ runner.os == 'macOS' }}
run: >
brew install
${{ fromJSON(inputs.autoconf) && 'autoconf' || '' }}
${{ fromJSON(inputs.automake) && 'automake' || '' }}
${{ fromJSON(inputs.libtool) && 'libtool' || '' }}
${{ fromJSON(inputs.openssl) && 'openssl@3' || '' }}
- name: Install macOS dependencies
shell: bash
if: ${{ runner.os == 'macOS' }}
run: |
packages=()
${{ fromJSON(inputs.autoconf) && 'packages+=("autoconf")' || '' }}
${{ fromJSON(inputs.automake) && 'packages+=("automake")' || '' }}
${{ fromJSON(inputs.libtool) && 'packages+=("libtool")' || '' }}
${{ fromJSON(inputs.openssl) && 'packages+=("openssl@3")' || '' }}
if [ "${`#packages`[@]}" -gt 0 ]; then
brew install "${packages[@]}"
fi
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/actions/install-macos-deps/action.yml around lines 39 - 47, The
"Install macOS dependencies" step currently runs "brew install" even when no
formulae are selected, causing a failure; change the step to compute the list of
chosen inputs (inputs.autoconf, inputs.automake, inputs.libtool, inputs.openssl)
into a variable and skip running "brew install" when that computed list is empty
(i.e., no-op or echo a message), otherwise run "brew install <packages>"; update
the step's run script to perform this conditional check so the action does
nothing if all inputs are false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, but I think it's a bug if this gets called with everything being disabled

Copy link
Contributor

Choose a reason for hiding this comment

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

@ShaharNaveh, that's a fair point — failing loudly on a misconfigured call (all packages disabled) is actually better behavior than silently no-oping, since it surfaces the bug immediately. Happy to defer to your judgment here.


✏️ Learnings added
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 7379
File: .github/actions/install-macos-deps/action.yml:39-47
Timestamp: 2026-03-07T11:52:42.756Z
Learning: In the RustPython project, ShaharNaveh prefers that the `.github/actions/install-macos-deps/action.yml` composite action fails (rather than silently no-ops) when called with all package inputs set to `false`. This is intentional: such a call is considered a bug/misconfiguration and should surface as a failure rather than being handled gracefully.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 5932
File: .github/workflows/comment-commands.yml:18-24
Timestamp: 2025-07-10T10:08:43.330Z
Learning: In GitHub Actions workflows for the RustPython project, the maintainer ShaharNaveh prefers to keep workflows simple and doesn't mind if steps fail when the desired state is already achieved (e.g., user already assigned to an issue). Avoid suggesting complex error handling for edge cases they don't consider problematic.

@youknowone youknowone merged commit f680f8a into RustPython:main Mar 7, 2026
12 of 13 checks passed
youknowone pushed a commit to youknowone/RustPython that referenced this pull request Mar 8, 2026
* Add `install-macos-deps` action

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

X Tutup