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
Add typical SDK paths on macOS #758
Conversation
macOS removed `/usr/include` in recent versions of macOS. This forces the usage of the internal libffi although macOS has a usable one itself. Related to ffi#757
"/usr/local/include", | ||
"/usr/include/ffi", | ||
"/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/ffi", | ||
"/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/ffi") |
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.
Wouldn't using xcrun --sdk macosx --show-sdk-path
to get the SDK path and then append /usr/include/ffi
be more future-proof?
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.
Shelling out to xcrun
can cause some unexpected behaviour unless you catch those cases and I'm not sure I'd recommend it.
Namely that if you haven't properly configured your development setup on macOS you'll get a graphical pop-up smack back in the middle of your screen asking if you'd like to install the CLT, and if you haven't accepted the Xcode/CLT license xcrun
just errors out telling you to do that first. They are edge cases but, people love to stumble upon edge cases.
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.
Compiling won't work until the CLT are installed anyway, so maybe it's fine to get the popup then if the user still did not install command-line tools?
Re the license, we could validate the output and only use it if it exists as a directory.
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.
I opened #765 for a version using xcrun
. I don't have a strong opinion to any of both. And I don't have macOS, so maybe this is something for someone who can actually try the patch out 😄
Looks like macOS' FFI doesn't have @larskanis Could you rebase so this also runs in GitHub Actions? |
See #766 (comment), on GitHub Actions' macOS libffi seems recent enough. Sorry for the comment about that one, seems the Xcode MacOSX.sdk isn't useful for libffi anyway. |
find_header("ffi.h", | ||
"/usr/local/include", | ||
"/usr/include/ffi", | ||
"/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/ffi", |
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.
I suggest to simplify and just remove this path since it doesn't seem to have libffi.
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.
Correction: that path has it too but seems less reliable and more likely to move in newer macOS versions.
MacOS removed `/usr/include` in recent versions of macOS. That forced the usage of the internal libffi although macOS has a usable one itself. This commit adds typical paths of the SDK which contain libffi. Since it's possible to use non default SDK paths, there is another fallback to determine the path per "xcrun" command. Fixes ffi#757 Closes ffi#765 Closes ffi#758
gclosure.c:28:10: fatal error: 'ffi.h' file not found ^~~~~~~ * See ffi/ffi#758 * Restore static libs recommendations. * Fix license.
macOS removed
/usr/include
in recent versions of macOS. This forces the usage of the internal libffi although macOS has a usable one itself.Related to #757