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

MacOS support #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

MacOS support #2

wants to merge 1 commit into from

Conversation

avikagan
Copy link

@avikagan avikagan commented Mar 8, 2023

These changes feel a bit hacky, but it seems like you need to remove the \u{1} from the autogenerate bindgen code to make it work on all platforms. I tested this on MacOS, Windows 10 and Linux (Ubuntu 20.04) and it all seems to work and function as expected. I ran the xtask test and it seemed to work just fine.

To be honest I'm not sure if this is the correct solution, or if there is a bindgen setting that will always generate universal bindings or generate those bindings at compile time?

It seems like you need to remove the \u{1} from the autogenerate bindgen code to make it work on all platforms.  Will test on Windows and Linux to see if this works
@Tastaturtaste Tastaturtaste added the enhancement New feature or request label Mar 8, 2023
@Tastaturtaste
Copy link
Owner

Tastaturtaste commented Mar 8, 2023

Thank you for your contribution! The issue with the \u{1} prepended to the link names is odd and was previously unknown to me but seemingly not new.

Ideally I would want to avoid any manual cleanup of the generated bindings though. Maybe bindgen can be persuaded to generate the link names without the prefix? Alternatively a post processing step like the one to replace the typedefs would be ok if bindgen cannot do it natively. The option trust_clang_mangling(false) which is mentioned in the bindgen issue linked to above sadly does not work as it removes the #[link_name] attribute completely. Which is a little bit weird, since I remove the version suffixes by using a ParseCallback and bindgen still has the names as they are. The function name change has nothing to do with clangs name mangling and thus I wouldn't expect an option name mangling to influence them.

Also, I probably should look into setting up CI to test for such platform dependent changes. Since I don't have a Mac I cannot test on Mac myself. CI with Matlab seems to be possible, but preliminary experimentation didn't work out that well for me.

In the meantime please rebase your changes onto dev, as it is the branch were all development should land before being merged in batch into master. Also, could you comment with a list with the exported symbols of your libmex.dylib on Mac? I believe nm -Ug path/to/libmex.dylib should work for that.

@Tastaturtaste
Copy link
Owner

Tastaturtaste commented Mar 13, 2023

Hey, I just rebased your fork onto dev and created a new branch. Besides one small conflict with a change unrelated to Mac support there is no conflict with any code you have written/changed. I also added CI with MacOS on top of your fork so it should be easy to see for everyone if your fork works for Mac. If you do further work on this issue, please base it on top of the mentioned new branch I created from your fork to allow an easy merge into dev after you are done.

As I said above I really don't want to add manual labour when creating the bindings. Before I am willing to merge your fork I would like to have some solution to avoid this. Sadly it seems bindgen does not offer any solution for this out of the box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants