-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[macOS] Ask CMake to look in the right place for include & link directories #12516
Conversation
…_directories on macOS (arm64 and x86_64) Homebrew installs to a different location on Arm Macs compared to Intel Macs. This change prevents this error when compiling on Arm: `ld: warning: directory not found for option '-L/opt/local/lib'`
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.
LGTM 👍
An annoying decision on Brew's side to change directories like that, but nothing we can do about it 🤷♂️
The CI build machine is x86 so if that's building then we should be fine.
The CI build won't necessarily fail if the directories are not found, it depends on the CMake options selected. So I think it's better for someone to manually build on Intel and confirm the warning doesn't appear. Homebrew changed the location so you can have both x86 installs and Arm installs on the same machine without it getting messy. Yes it's annoying, but I can understand why they did it. |
@Codex- This PR needs testing on Intel, not M1. I already tested it for M1. RPCS3 currently does not build on ARM due to a regression. See #12577 for details. Once that regression is fixed, you can see #12115 for Arm build instructions. (also, you should make the change from this PR too). Edit: If you happen to have qt@6 installed the build will fail also. |
Oh, my apologies, I completely misinterpreted based on the [arm] title tag and fell down the rabbit hole 😓 |
Use STREQUAL instead of MATCHES, since MATCHES is supposed to be used with regular expressions. Also update the homebrew directories for Intel, since they were wrong.
I was finally able to test this on Intel. With these recent changes the warning does not appear on Intel or Arm. As I mentioned in my edit in the main description, it turns out the path for the Intel homebrew directories were originally incorrect, so this warning was showing on Intel builds too. On Intel it's usr/local, not opt/local. I also changed MATCHES to STREQUAL in the conditional check, because apparently MATCHES is supposed to be used with Regexes, not strings. For reference: I think this PR can be merged. Note that RPCS3 master can't be built for arm64 right now due to an unrelated regression (#12577) |
The old /opt/local directory is likely because of Macports, which by default installs to |
Sure, it would be nice to keep it, but considering that the build instructions uses homebrew, and the build script also uses homebrew, I think that should take precedence. |
Yeah that's fair, makes sense to me |
Remove the conditional check for x86 Apple, as usr/local is part of the default search path. Tested on Intel and the warning does not appear.
Homebrew installs to a different location on Arm Macs compared to Intel Macs. (usr/local/ for Intel and opt/homebrew/ for Arm)
Edit: Turns out the existing Intel one was wrong. It's usr/local, not opt/local.
This PR sets the correct location for the include_directories and link_directories.
When building on Arm, this change prevents the following error that displays when compiling is complete:
ld: warning: directory not found for option '-L/opt/local/lib'
Testing: I have tested this on Arm, but compilation needs to be tested on Intel. Just ensure the above error does not appear when compilation has completed.
This PR should not affect Windows or Linux.
Note: the elseif() could probably be shorter and just have elseif(APPLE) to reduce the number of checks, but I think for readability it's better to have the second check so it's clear it's intended for Intel Macs. Or alternatively I could just add a comment...