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

Adds support macOS dylib #86

Merged
merged 1 commit into from
Feb 1, 2019
Merged

Adds support macOS dylib #86

merged 1 commit into from
Feb 1, 2019

Conversation

pftg
Copy link
Contributor

@pftg pftg commented Jan 19, 2019

No description provided.

@pftg
Copy link
Contributor Author

pftg commented Jan 19, 2019

Hi @eric please check my changes. I found some issue to compile on macOS Mojave beta: somehow .a and .la files now are ignoring. So no old libraries are supported. Elsewhere it will be better to use dylib instead of .a.

ext/extconf.rb Outdated
@@ -83,10 +83,9 @@ def safe_sh(cmd)

# Absolutely prevent the linker from picking up any other zookeeper_mt
Dir.chdir("#{HERE}/lib") do
%w[st mt].each do |stmt|
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the line copying the mt version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

based on https://github.com/zk-ruby/zookeeper/pull/86/files#diff-b3c266c0a40fd6ea5ce51093b147dbe6R93 there is no mt usage. I could split into 2 commits with dylib adding and then with small refactoring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, yep I'll create separate PR for that refactoring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated PR without this change

Clang-7 linker does not support `.a` and `.la` libs.
Use dylibs for linker instead of old libraries
@pftg
Copy link
Contributor Author

pftg commented Jan 21, 2019

Hi @eric please check again. I removed extra cleanups. Just added support for .dylib

@pftg
Copy link
Contributor Author

pftg commented Jan 29, 2019

@eric @slyphon hi guys, what do you think about this PR?

@eric
Copy link
Member

eric commented Feb 1, 2019

Looks good

@eric eric merged commit 537532d into zk-ruby:master Feb 1, 2019
@timsutton timsutton mentioned this pull request Jun 7, 2021
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.

None yet

2 participants