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

Portable shared library linking #44

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

breadbored
Copy link

No description provided.

@breadbored
Copy link
Author

This detects the OS and uses the appropriate method to add links for the compiled shared library.

I couldn't get this to compile on macOS 12.0.1 as --whole-archive is not supported, and this solved my issue (using -force_load instead of --whole-archive). I tested on the Dockerfile and it works on Ubuntu as well. I assume this works on Windows (using /WHOLEARCHIVE instead of --whole-archive), but I did not test it.

For use with languages like Python when compiled to a shared library. LibSWIFFT_Compute takes the output as a param, but this function will return the result for wrappers.
@gvilitech
Copy link
Contributor

@breadbored, thanks for submitting a PR. Both portability and supporting wrappers are good goals to aim for.

Regarding the portability part, could you post logs of the builds you tried on each platform to support this part of the change?

The wrapping part of the change cannot be accepted, though, as it has a bug (see the commit review) and the natural solution is to use an already existing LibSWIFFT function.

@breadbored
Copy link
Author

I cannot believe I missed this PR! I've been flooded with work PRs and it got lost in the weeds.

I'll set a reminder to rebuild this weekend and I can describe what this PR aims to resolve.

I was attempting to expose the library to Python at the time, but introduced an error referenced in gvilitechltd/LibSWIFFT PR#44.
@gvilitech
Copy link
Contributor

@breadbored, the code-removal in the last commit looks OK. Reminder - could you post logs of the builds you tried on each platform to support the change?

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

Successfully merging this pull request may close these issues.

2 participants