Fix lots of build problems on macOS #65
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robertlipe
reviewed
Mar 3, 2024
| FindLibserialport | ||
| ------- | ||
|
|
||
| Finds the sigrok serial port library (``libserialport``) |
There was a problem hiding this comment.
Tying it to sigrok seems super weird.
It's not like this library is optional, IIRC. You either have it correctly installed or you don't, so adding layers to go hunt for it seems a distraction.
There was a problem hiding this comment.
- Agree, wasn't sure if there were any other libserialport libraries since it felt like a pretty common name.
- I don't really get what you're trying to say. In CMake, using a find script is the normal way to link to other libraries. With a find script, standard errors are given when the library isn't found, the call site is much cleaner, and this script can be reused in other projects.
|
I can't test/validate for macOS sadly. I'm happy to merge this as I think to my knowledge it looks good. Could you do a re-base on the latest just to make sure CI etc is happy? |
This new find module will replace the old logic used to locate a native copy of libserialport. THe old code didn't work on macOS, and was pretty messy.
While this might work when using bundled libraries, this breaks with system libraries as they are all compiled for arm64. Also, why would you need to compile an x86_64 version on arm64, and vise versa.
CMake interpreted `argtable3` to mean add `-largtable3` rather than to use the imported argtable3 target. This worked when using the bundled library, but broke with native libraries.
JetbladeDevsStuff
force-pushed
the
fix-build-macos
branch
from
5634b67 to
2268e53
Compare
|
Only 9 months later, lol. CI should be able to run now. |
|
Yeah huge apologies on this 😢 I never got notifications and the main maintainer has been snowed under by work+life. I'll be around now for future 😅 |
|
Thanks, and I have been loving the Pinecil too. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Building on macOS with native libraries was super broken. This PR fixes dependency location for both
argtable3andlibserialport, adding aFindLibserialportCMake module to help. This PR also disables building a x86_64 and arm64 version of blisp by default on macOS, as the cross compiled binary wouldn't be able to link with native libraries.