Skip to content

Add macOS CI#126

Draft
liopeer wants to merge 4 commits intomainfrom
lionel-add-mac-ci
Draft

Add macOS CI#126
liopeer wants to merge 4 commits intomainfrom
lionel-add-mac-ci

Conversation

@liopeer
Copy link
Copy Markdown
Contributor

@liopeer liopeer commented Jun 5, 2025

What has changed and why?

  • still a draft

Copy link
Copy Markdown

@JiwaniZakir JiwaniZakir left a comment

Choose a reason for hiding this comment

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

The ifeq ($(OS),Linux) check in the Makefile (around line 173) is likely broken. On Linux systems, the OS environment variable is not set by the OS itself (unlike Windows where it's Windows_NT), so $(OS) will be empty on Ubuntu CI runners. This means PINNED_TORCH_VERSION_PY38, PINNED_TORCH_VERSION_PY312, and related variables will be undefined when running on Linux CI, silently falling back to whatever default behavior the install targets have. The correct approach would be ifeq ($(shell uname -s),Linux) or checking RUNNER_OS (which GitHub Actions sets to "Linux" or "macOS").

Additionally, the test_unit.yml file is missing a newline at the end of file (noted in the diff), which is a minor but easily fixed style issue. It's also worth considering whether the "Setup cmake" step should be gated with if: ${{ matrix.runners == 'ubuntu-latest' }} the same way FFmpeg is — if cmake isn't needed or behaves differently on macOS, this could cause unexpected failures.

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