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

Remove symbol files as these are no longer needed #141

Conversation

john-paulsmith
Copy link
Contributor

@john-paulsmith john-paulsmith commented Feb 7, 2024

Both the CMake and Makefile projects included compiler arguments to set symbol visibility to hidden and expose only the OfxGetNumberOfPlugins and OfxGetPlugin symbols. The CMake project has been changed to use built-in functionality to control symbol visibility, reducing the amount of platform-specific code that needs to be maintained.

Furthermore, all our examples use __attribute__((visibility("default"))) to expose necessary symbols, rendering the symbol files unnecessary, so all references to the symbol files, and the files themselves have been removed.

@john-paulsmith
Copy link
Contributor Author

john-paulsmith commented Feb 7, 2024

I have only tested this complete changeset on macOS, although I verified the concept on Linux too. Comments from a Windows perspective would be appreciated, I believe Windows symbol visibility is hidden by default and the use of __declspec(dllexport) will be exposing the correct symbols already.

Is there any reason to keep the symbol files around for plug-in/host build scripts?

@john-paulsmith john-paulsmith requested a review from garyo February 7, 2024 00:09
@john-paulsmith
Copy link
Contributor Author

If this PR is merged, I believe #122 can be discarded.

Both the CMake and Makefile projects included compiler arguments to set symbol visibility to hidden and expose only the OfxGetNumberOfPlugins and OfxGetPlugin symbols. The CMake project has been changed to use built-in functionality to control symbol visibility, reducing the amount of platform-specific code that needs to be maintained.

Furthermore, all our examples use __attribute__((visibility("default"))) to expose necessary symbols, rendering the symbol files unnecessary, so all references to the symbol files, and the files themselves have been removed.

Signed-off-by: John-Paul Smith <[email protected]>
garyo added a commit that referenced this pull request Feb 24, 2024
Signed-off-by: Gary Oberbrunner <[email protected]>
Copy link
Contributor

@garyo garyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- I've pulled these changes into the work I did in #122 since there were some related changes in there to make sure the CI builds work.

@garyo
Copy link
Contributor

garyo commented Feb 24, 2024

Closing; all in #122 now.

@garyo garyo closed this Feb 24, 2024
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