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

Make libffi dependency an optional, default feature #33

Closed
AndrewGaspar opened this issue Feb 6, 2018 · 7 comments
Closed

Make libffi dependency an optional, default feature #33

AndrewGaspar opened this issue Feb 6, 2018 · 7 comments
Assignees

Comments

@AndrewGaspar
Copy link
Contributor

AndrewGaspar commented Feb 6, 2018

On one of the platforms I'm running on, the libffi dependency fails to build. Since it seems like the libffi use is not required in general usage of the API, it would be nice to be able to remove the libffi dependency by making it an optional, default feature in the Cargo manifest.

It would be helpful if an additional UserOperation constructor could be added that takes a simple function pointer rather than a closure, though for my use case mpi::ffi will suffice.

@AndrewGaspar AndrewGaspar changed the title Add feature that optionally removes the libffi dependency Make libffi dependency on optional, default feature Feb 6, 2018
@bsteinb
Copy link
Collaborator

bsteinb commented Feb 6, 2018

Sounds good to me. I will give this a shot tomorrow.

Does it make sense to open an issue at tov/libffi-sys-rs or is libffi just in general not likely to build on that particular platform?

@bsteinb bsteinb self-assigned this Feb 6, 2018
@AndrewGaspar
Copy link
Contributor Author

The issue is that libtool isn't available on this platform (indirectly used by libffi by way of autoconf).

To be honest, this may actually be an issue with the platform - I have reason to believe it should be available and was recently available. I'm reaching out to the administrator to see if perhaps libtool was removed mistakenly. Maybe hold off on this - it may not be an issue anymore if I can get libtool reinstated.

@AndrewGaspar AndrewGaspar changed the title Make libffi dependency on optional, default feature Make libffi dependency an optional, default feature Feb 6, 2018
@AndrewGaspar
Copy link
Contributor Author

Indeed it was a mistake. 🙂 The admin fixed the problem. Feel free to close this.

@bsteinb
Copy link
Collaborator

bsteinb commented Feb 7, 2018

I am glad you could resolve the issue on your end. I have nevertheless made the user operation API an optional default feature which depends on libffi in ca8e6d7. It is indeed an optional, non-essential part of the API and it is not too much of a hassle to acknowledge this fact by making it an optional feature.

However, I did not see a way to provide a UserOperation constructor for a raw function pointer. The existing constructor injects (by creating another closure) a wrapper function between the user supplied closure and MPI that wraps the raw buffers that MPI passes to the callback inside some safer Rust types.

@AndrewGaspar
Copy link
Contributor Author

Excellent, thank you!

Hm... I guess what I wanted was not necessarily an enhancement to UserOperation, but perhaps an UnsafeUserOperation that implements Operation. It would take an unsafe function pointer (an ffi::MPI_User_function) in its constructor, rather than an Fn(DynBuffer, DynBufferMut). The user would have to explicitly handle converting the inputs to the actual data type. This would allow you to still use the Operation with the collective routines, and get automatic freeing of the operation, but without the dependency on libffi.

@bsteinb
Copy link
Collaborator

bsteinb commented Feb 8, 2018

Have a look at c08dca8. Is that roughly what you had in mind?

@AndrewGaspar
Copy link
Contributor Author

Yes, that's exactly right, thank you!

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

No branches or pull requests

2 participants