Skip to content
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

os/mac/diagnostic: fix OpenCore check #18765

Merged
merged 4 commits into from
Nov 13, 2024
Merged

os/mac/diagnostic: fix OpenCore check #18765

merged 4 commits into from
Nov 13, 2024

Conversation

ZhongRuoyu
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Fixes #18763.

@cho-m
Copy link
Member

cho-m commented Nov 13, 2024

Interesting that CI runner triggers it. I wonder if it is actually set up with OpenCore.

@ZhongRuoyu
Copy link
Member Author

Yea, I only checked GitHub hosted runners which do not seem to use OpenCore.

If that's the case then maybe we can allow it in CI?

@cho-m
Copy link
Member

cho-m commented Nov 13, 2024

Can you add opencore_version to message? Curious what it is outputting.

@ZhongRuoyu
Copy link
Member Author

Warning: You have installed macOS using OpenCore Legacy Patcher, version 4D1FDA02-38C7-4A6A-9CC6-4BCCA8B30102:opencore-version REL-083-2022-08-01

@MikeMcQuaid
Copy link
Member

If that's the case then maybe we can allow it in CI?

Yeh, agreed to allow in CI unless it's failing specifically in our CI.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks good once CI check removed.

Copy link
Member

@EricFromCanada EricFromCanada left a comment

Choose a reason for hiding this comment

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

I can confirm that nvram 4D1FDA02-38C7-4A6A-9CC6-4BCCA8B30102:opencore-version only returns a value when the current installation was actually booted via OCLP. (Oddly, the other command nvram 4D1FDA02-38C7-4A6A-9CC6-4BCCA8B30102:OCLP-Version still returns a value even when booting off of a non-OCLP boot disk.)

An alternate detection method would be to check for Lilu in the list of loaded kexts, e.g.:

% kmutil showloaded | grep Lilu
No variant specified, falling back to release
   52   10 0                  0x2f000    0x2f000    as.vit9696.Lilu (1.6.7) 087C34D2-49F8-3FDA-8757-27E9425C9EE4 <9 7 6 3 2 1>

Library/Homebrew/extend/os/mac/diagnostic.rb Outdated Show resolved Hide resolved
Library/Homebrew/extend/os/mac/diagnostic.rb Outdated Show resolved Hide resolved
Co-authored-by: Eric Knibbe <enk3@outlook.com>
@ZhongRuoyu
Copy link
Member Author

Looks good once CI check removed.

@MikeMcQuaid Not sure if I followed. I added the CI check (ENV["CI"]) to allow CI to pass because our self-hosted runners do seem to be on OpenCore. Would you like it to be done differently?

@MikeMcQuaid
Copy link
Member

Not sure if I followed. I added the CI check (ENV["CI"]) to allow CI to pass because our self-hosted runners do seem to be on OpenCore. Would you like it to be done differently?

Ah, no worries, ignore me then. 🚢 it!

@ZhongRuoyu ZhongRuoyu enabled auto-merge November 13, 2024 15:55
@ZhongRuoyu ZhongRuoyu merged commit 69fd4a6 into master Nov 13, 2024
35 checks passed
@ZhongRuoyu ZhongRuoyu deleted the opencore-fix branch November 13, 2024 15:58
@Bo98
Copy link
Member

Bo98 commented Nov 13, 2024

Interesting that CI runner triggers it. I wonder if it is actually set up with OpenCore.

OpenCore is a bootloader whereas OCLP is what patches old hardware to run newer OS.
We use a custom bootloader in CI because we run multiple instances of macOS under a k8s environment on native hardware (so the host layer is technically Linux). We do not use OCLP.

So the unsupported configuration is more OCLP rather than OpenCore as that is what makes old CPUs run on macOS versions where our x86_64 binaries might not necessarily be built for said old CPUs (i.e. pre-Westmere). Given Eric's findings above, the reliable check might be to check if both OCLP-Version and opencore-version is set.

ZhongRuoyu added a commit that referenced this pull request Nov 13, 2024
@ZhongRuoyu
Copy link
Member Author

OpenCore is a bootloader whereas OCLP is what patches old hardware to run newer OS. We use a custom bootloader in CI because we run multiple instances of macOS under a k8s environment on native hardware (so the host layer is technically Linux). We do not use OCLP.

So the unsupported configuration is more OCLP rather than OpenCore as that is what makes old CPUs run on macOS versions where our x86_64 binaries might not necessarily be built for said old CPUs (i.e. pre-Westmere). Given Eric's findings above, the reliable check might be to check if both OCLP-Version and opencore-version is set.

Thanks for the explanation! Adding an additional check for OCLP in #18766. I suppose with that we won't need to skip the check in CI.

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.

check_for_opencore false positive
5 participants