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 libgraphql parser to optionally be built with cmake #21

Merged
merged 3 commits into from
May 26, 2017

Conversation

ianks
Copy link
Contributor

@ianks ianks commented May 18, 2017

Manually installing graphql is a big burden for entry for using this gem. It takes time, manual effort, and we can't ensure the version of libgraphql parser is compatible.

Many users already have cmake and a modern c++ compiler installed on their system. As a result, we can compile libgraphql in the gem install process. This makes installing the gem a breeze for many users.

If they do not have cmake, or something goes wrong, we just fall back to the old method of looking up the library. Thus, this PR does not affect users which do not have cmake.

Copy link
Owner

@rmosolgo rmosolgo left a comment

Choose a reason for hiding this comment

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

Wow, impressivel! Looks good to me.

I guess since Travis is green, and isn't making a manual install, we can assume it's using the bundled-in version, right?

.gitmodules Outdated
@@ -0,0 +1,6 @@
[submodule "ext/--force"]
path = ext/--force
Copy link
Owner

Choose a reason for hiding this comment

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

Oops, did this sneak in somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup! fixed!

@ianks
Copy link
Contributor Author

ianks commented May 19, 2017

we can assume it's using the bundled-in version, right

Yup! Travis is no longer installing libgraphqlparser anymore. It comes from the bundled gem.

@ianks
Copy link
Contributor Author

ianks commented May 25, 2017

@rmosolgo Any thing else needed for this?

@rmosolgo
Copy link
Owner

This looks great, thanks for the bump!

@rmosolgo rmosolgo merged commit 8015753 into rmosolgo:master May 26, 2017
@rmosolgo
Copy link
Owner

Taking a try locally and hitting:

/Users/rmosolgo/code/graphql-libgraphqlparser-ruby/lib/graphql/libgraphqlparser.rb:5:in `require_relative': dlopen(/Users/rmosolgo/code/graphql-libgraphqlparser-ruby/lib/graphql_libgraphqlparser_ext.bundle, 9): Library not loaded: libgraphqlparser.dylib (LoadError)
  Referenced from: /Users/rmosolgo/code/graphql-libgraphqlparser-ruby/lib/graphql_libgraphqlparser_ext.bundle
  Reason: image not found - /Users/rmosolgo/code/graphql-libgraphqlparser-ruby/lib/graphql_libgraphqlparser_ext.bundle
	from /Users/rmosolgo/code/graphql-libgraphqlparser-ruby/lib/graphql/libgraphqlparser.rb:5:in `<top (required)>'
	from /Users/rmosolgo/code/graphql-libgraphqlparser-ruby/test/test_helper.rb:2:in `require'
	from /Users/rmosolgo/code/graphql-libgraphqlparser-ruby/test/test_helper.rb:2:in `<top (required)>'

It seems like libgraphqlparser/build/lib/libgraphqlparser.dylib isn't being found at runtime... hmm...

@ianks
Copy link
Contributor Author

ianks commented May 26, 2017

Interesting. Full disclosure, I developed this on linux and did not test on a mac. let me take a look and see if i can get it working on a mac.

@ianks
Copy link
Contributor Author

ianks commented Jun 9, 2017

@rmosolgo I figured out the issue. Essentially we need to use install_name_tool to fix the path for libgraphqlparser.dylib. I do not have a mac so this is hard to test, but maybe someone can give it a whirl? should be easy. If not, I can attempt it when I get around a mac at some point.

http://thecourtsofchaos.com/2013/09/16/how-to-copy-and-relink-binaries-on-osx/

@lucaspolonio
Copy link

Hey @rmosolgo, I see this has been merged but no new release has been created since then. Do you plan to create a new release that includes this change?

@rmosolgo
Copy link
Owner

Sorry, I got hung up trying to run it locally and haven't tried again! It looks like there's a lead above, but I want to be able to run the tests locally before releasing a version. Do you want to take a crack at it?

@rmosolgo
Copy link
Owner

opened a proper issue here: #25

rmosolgo added a commit that referenced this pull request Jul 25, 2017
This reverts commit 8015753, reversing
changes made to e41cba2.
@rmosolgo
Copy link
Owner

Sorry, I ended up reverting this in 5fe220a :'(

I would love to include a bundled copy of libgraphqlparser but I couldn't get past the issue described in #25.

I think the next step is to get a travis build on OSX and make sure that passes

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.

3 participants