Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

Adds formula for C++ graphql parser #44389

Closed
wants to merge 9 commits into from

Conversation

hwhelchel
Copy link
Contributor

This will make it easier to install the libgraphqlparser and use bindings for it in higher level languages. This is my first formula so I may have missed something. Thanks!

/cc @dylanahsmith @rmosolgo @eapache

@hwhelchel hwhelchel changed the title Adds formula for C and C++ graphql parser Adds formula for C++ graphql parser Sep 27, 2015
def install
system "cmake", ".", *std_cmake_args
system "make"
bin.install "dump_json_ast"
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn’t there a make install we could use?

Choose a reason for hiding this comment

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

this is just an example program. I doesn't need to be installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dylanahsmith do you have any recommendations on how to test a successful install? This was the easiest way I could see.

Copy link
Member

Choose a reason for hiding this comment

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

If it's just a test executable, you can keep & use it, but leave it in libexec or pkgshare so users don't bump into it unnecessarily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bfontaine looks like there is no make install. /cc @eapache

2015-09-27 20:51:45 -0400

make
install

make: *** No rule to make target `install'.  Stop.

Choose a reason for hiding this comment

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

Since you aren't using make install you are probably missing headers.

Try to compile a simple program that uses libgraphqlparser with it installed. E.g.

$ cat > main.c <<EOF
#include "graphqlparser/c/GraphQLParser.h"
#include "graphqlparser/c/GraphQLAstNode.h"

int main(int argc, char **argv) {
    struct GraphQLAstNode *node = graphql_parse_string("{ field }", NULL);
    if (node != NULL) {
        graphql_node_free(node);
        return 0;
    }
    return 1;
}
EOF
$ cc -lgraphqlparser main.c -o main

Choose a reason for hiding this comment

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

looks like there is no make install

There is in the latest master, but not in version 0.1.0. Maybe we should ask them to make a release so that it can more easily be packaged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep the graphqlparser/c/GraphQLParser.h header file wasn't found. I've submitted a request on the original library for an updated release. Thank you for your help.

}
EOS

json_ast = JSON.parse(sample_ast)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless we need sample_ast later in the test we should probably set the variable directly:

ast = {
  "kind" => "Document",
  "loc" => {
    "start" => 1,
    "end" => 2,
    ...
  }
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great idea

@hwhelchel hwhelchel closed this Sep 29, 2015
@hwhelchel hwhelchel reopened this Sep 29, 2015
@hwhelchel hwhelchel closed this Sep 29, 2015
@hwhelchel hwhelchel reopened this Sep 29, 2015
@hwhelchel
Copy link
Contributor Author

@bfontaine there is so much activity on the project I'm having a hard time getting a green build on travis. We're hitting Github's API rate limit. Can you all add a token to the travis environment? Should I create an issue?

Error: GitHub API rate limit exceeded for 208.78.110.199. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)
Try again in 40 minutes 45 seconds, or create an personal access token:
  https://github.com/settings/tokens
and then set the token as: HOMEBREW_GITHUB_API_TOKEN

@DomT4
Copy link
Member

DomT4 commented Sep 29, 2015

Just ignore Travis here. Jenkins is the primary CI still and won't have that problem.

@ablyler
Copy link
Contributor

ablyler commented Sep 29, 2015

@Homebrew/owners the mavericks brew bot machine is stuck working on this, this job has been running for 9+ hours: http://bot.brew.sh/job/Homebrew%20Pull%20Requests/version=mavericks/33886/

@bfontaine
Copy link
Contributor

@ablyler Thanks, I’m killing the build for now.

@ablyler
Copy link
Contributor

ablyler commented Sep 29, 2015

@bfontaine thanks, looks like the box might need rebooted. it is odd that it aborted after 180 minutes but stopped when it could delete a file:

Build timed out (after 180 minutes). Marking the build as aborted. FATAL: Unable to delete script file /var/folders/_z/11_kwl954xvch_n92gnzdjzw0000gp/T/hudson1289499836159169147.sh

@ablyler
Copy link
Contributor

ablyler commented Sep 29, 2015

@bfontaine looks like it is currently hung up at: Started calculate disk usage of build Finished Calculation of disk usage of build in 0 seconds

@ablyler
Copy link
Contributor

ablyler commented Sep 29, 2015

@bfontaine looks like the build has been killed, but the mavericks box isn't coming back up?

@bfontaine
Copy link
Contributor

@ablyler I’m on it but I’m at work so I don’t have access to the credentials necessary to connect to the VM.

@bfontaine
Copy link
Contributor

@ablyler update: I woke up other team members and we’re on it.

@DomT4
Copy link
Member

DomT4 commented Sep 29, 2015

It's alive again now.

@bfontaine
Copy link
Contributor

Thanks @DomT4 ❤️

@hwhelchel hwhelchel closed this Sep 29, 2015
@hwhelchel hwhelchel reopened this Sep 29, 2015
@hwhelchel
Copy link
Contributor Author

@DomT4 I wanted to let you know that I re-kicked off the build by closing/re-opening the PR. I did this because the last one failed on jenkins but due to a git error. The new build has been hanging for awhile. My apologies but thought you would like to know in case closing/re-opening a PR exposes a bug in the build process. Thanks

@DomT4
Copy link
Member

DomT4 commented Sep 30, 2015

You can ping one of us to restart builds as well, or we generally get around to going back through bot-failed builds without obvious cause and restarting them throughout the day. The new build hasn't actually started yet, so not hanging per se, it's just in the queue :)

@hwhelchel
Copy link
Contributor Author

Awesome thanks

On Tue, Sep 29, 2015 at 10:39 PM Dominyk Tiller notifications@github.com
wrote:

You can ping one of us to restart builds as well, or we generally get
around to going back through bot-failed builds without obvious cause and
restarting them throughout the day. The new build hasn't actually started
yet, so not hanging per se, it's just in the queue :)


Reply to this email directly or view it on GitHub
#44389 (comment).

@@ -0,0 +1,203 @@
require "open3"
Copy link
Member

Choose a reason for hiding this comment

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

To be a little annoying, if both of these are required only for the test do stage, let's move them to the top of that block.

@hwhelchel
Copy link
Contributor Author

Good call on simplifying the sample query

@hwhelchel
Copy link
Contributor Author

Looks like we've got a passing Jenkins build! What can I do to help get this merged?

sha256 "5064f63024c20cdc2c41970a6e9a5c7b053565db22f5f8dfb946923cb077f9de"

depends_on "cmake" => :build
depends_on "bison" => :recommended
Copy link
Contributor

Choose a reason for hiding this comment

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

Just double-checking, this can't use the system bison and flex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would that make the dependency implicit? Do all targeted systems have bison and flex? The library comes with a checked in parser and lexer it fallsback to using if the appropriate version of bison and flex aren't present. That's why I was thinking recommended made the most sense.

https://github.com/graphql/libgraphqlparser#building-libgraphqlparser

Copy link
Contributor

Choose a reason for hiding this comment

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

Xcode 7 has bison 2.3 and flex 2.5.35; what's the minimum version we need here?

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like Bison 3 https://github.com/graphql/libgraphqlparser/blob/master/CMakeLists.txt#L13 -- but if we can just use the distributed ones, that seems fine, too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool i've removed the depends_on bison and flex

@dunn
Copy link
Contributor

dunn commented Oct 4, 2015

Merged in 81c9da5, thanks for all your work here!

@dunn dunn closed this in 81c9da5 Oct 4, 2015
@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants