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 gem to install h3 as a native extension #45

Merged
merged 1 commit into from
Jan 4, 2019
Merged

Conversation

seanhandley
Copy link
Owner

@seanhandley seanhandley commented Dec 28, 2018

Bit hacky, but it works.

Fixes #2 🎉

@seanhandley seanhandley force-pushed the h3_install branch 5 times, most recently from 328c2fc to 3e6fd3b Compare December 28, 2018 21:41
@seanhandley seanhandley requested a review from fxn December 28, 2018 21:41
@seanhandley seanhandley force-pushed the h3_install branch 24 times, most recently from dad1ce2 to 354aae8 Compare December 31, 2018 11:28
@seanhandley seanhandley force-pushed the h3_install branch 2 times, most recently from 6d9697e to 9e4f929 Compare January 2, 2019 14:05
Copy link
Contributor

@fxn fxn left a comment

Choose a reason for hiding this comment

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

One of the benefits of FFI is not requiring a compiler :). But maybe in this case this proposal makes sense because the library does not seem to be packaged (apt, brew, etc.) and users need to compile H3 themselves anyway, right?

Would it be a way to retrieve the last stable version?


desc "Remove compiled H3 library"
task :clean do
File.delete("ext/h3/src/Makefile") if File.exists?("ext/h3/src/Makefile")
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder... the Makefile of H3 has a clean target. It does not delete the directories themselves, but wouldn't it be enough to delegate to their make clean and assume they know what it needs to be cleaned?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good thinking - I'll see if it works 👍

Copy link
Owner Author

Choose a reason for hiding this comment

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

Tried it - it doesn't seem to remove any of the compiled code...

@@ -8,7 +8,7 @@ def self.extended(base)
base.extend FFI::Library
base.include Structs
base.include Types
base.ffi_lib ["libh3", "libh3.1"]
base.ffi_lib ["ext/h3/src/lib/libh3.dylib", "ext/h3/src/lib/libh3.so"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these relative paths work when a user uses the gem?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes. Tested it locally.

Copy link
Owner Author

Choose a reason for hiding this comment

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

FYI I tested this badly!

After releasing 3.3.0 I added it to stuart-api and discovered a LoadError. The paths do indeed need to be absolute 🤦‍♂️

Hotfixed here: StuartApp/h3_ruby@d192b41

I yanked 3.3.0 from Rubygems and released 3.3.1. Seems to work just fine now 😅

@seanhandley
Copy link
Owner Author

Regarding packaging - no, there's no binary packages at all for H3 and the other bindings assume it isn't preinstalled so go about downloading/building it directly.

@seanhandley
Copy link
Owner Author

Since we include it as a git submodule we can move HEAD to the correct place each time we release a new version.

@seanhandley seanhandley merged commit 8abdba1 into master Jan 4, 2019
@seanhandley seanhandley deleted the h3_install branch January 4, 2019 13:41
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