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

allow building of shared libraries #16

Merged
merged 5 commits into from
Oct 4, 2021

Conversation

umlaeute
Copy link
Contributor

(note: this PR is on top of #15, for convenience reasons (so i can build it :-))

this PR adds a build-target that creates a "libo2.so" file (a shared dynamic library).

more importantly, it adds code to hide symbols from the final binary, in order to not accidentally make private symbols available to host applications. (this was triggered because some private symbols disappeared between 1.0 and 1.1)

now i don't know, whether you - as upstream - support shared libraries at all. however, for the Debian package we would very much like to build a shared library, to avoid code duplication (and be able to fix issues only once).

i have therefore split the commits into those that allow control of visibility (87164a1) and those that enable the building of a shared library (920950d, with e47dd5e).
so if you don't want to support shared libraries upstream, i would be thankful if you could at least accept 87164a1 into the codebase. as it would make life as Debian packages easier.

but of course, i would prefer if you properly supported shared libs :-)

@umlaeute
Copy link
Contributor Author

@rbdannenberg i would highly appreciate your word on whether building a shared library of o2 is supported or not (that is: whether you have semantic versioning in mind)

@umlaeute
Copy link
Contributor Author

ping @rbdannenberg

not everybody has their o2 clone live in /Users/rbd/...
so it properly exports the required symbols, even if we build with a
default-visibility of "hidden".
@umlaeute
Copy link
Contributor Author

i see there was some recent pre-semester activity in this repository, so i'm trying again

ping @rbdannenberg

this PR has been updated to current master (based on the v2 branch)

@umlaeute umlaeute mentioned this pull request Sep 30, 2021
@rbdannenberg rbdannenberg merged commit 37729d2 into rbdannenberg:master Oct 4, 2021
@umlaeute
Copy link
Contributor Author

umlaeute commented Oct 4, 2021

i take the merge as: "yes, we do support linking against O2 as a shared library".
🚀

@rbdannenberg
Copy link
Owner

Thanks for the work on shared library support. I don't seem to be getting consistent notifications from GitHub and there are far too many social media feeds (this is only one) for me to poll them. Email to rbd@cs.cmu.edu is most effective. I hope you got my note about conditional symbols -- I think compiling by default is a reasonable solution, but I didn't get your response.

In terms of support, I would not use O2 as a shared library, but since you've done the work to enable it, I support making this an option. Thanks again.

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