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

Inital pass at generating Protocol Buffer headers/source for C #70

Merged
merged 4 commits into from Jan 4, 2014

Conversation

hazen
Copy link

@hazen hazen commented Dec 5, 2013

Here is a basic attempt to generate C header and source file from .proto files.

Note: This now requires the protoc-c package

Currently c_release does nothing, but in the future could install into a configurable location, such as /usr/local

# C specific build steps
c_compile:
@echo "==> C (compile)"
@./build-c.sh

Choose a reason for hiding this comment

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

Couldn't this be simpler by (since protoc is required already) using:

protoc --c_out=c src/*.proto

Google's protoc will search the path for the protoc-c plugin.

Copy link
Author

Choose a reason for hiding this comment

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

That's basically what the script does. I was just trying to be consistent with the other languages, which check for the tool and build. Could easily put all of the steps in the Makefile instead.

Choose a reason for hiding this comment

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

I was wrong about protobuff-c, it doesn't include a protoc-gen-c binary. 😦

@seancribbs
Copy link

This works as intended, but I'd like to see it make less use of the shell script and more use of the Makefile. e.g.

PROTOS := $(wildcard src/*.proto)
C_FILES := $(patsubst src/%.proto,c/%.pb-c.c,$(PROTOS))
c_compile: c_announce c $(C_FILES)

c_announce:
    echo "==> C (compile)"

c:
    mkdir -p c/

c/%.pb-c.c c/%.pb-c.h: c src/%.proto
    echo "Generating $@ from $<"
    protoc-c -Isrc $< --c_out=c

If the protoc-c binary is not present, the error will happen at the generation stage. Of course, there are likely errors and improvements to be made in the example because I'm not a Makefile guru.

@seancribbs
Copy link

I'd also remove targets that don't do anything, e.g. c_release. 😄

- Remove build-c.sh
- Move creation of .c/.h files directly into Makefile
- Add c_release target to copy files to a local directory (/usr/local/riak_pb_c)

$(C_DIR)/%.pb-c.c $(C_DIR)/%.pb-c.h: src/%.proto
@echo "Generating $@ from $<"
$(PROTOC) -Isrc $< --c_out=$(C_DIR)

Choose a reason for hiding this comment

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

This could use an @.

Copy link
Author

Choose a reason for hiding this comment

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

I debated about that. Thought it was useful to actually see the call, but am happy to hide it.

Choose a reason for hiding this comment

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

Ok, maybe you could omit the echo on the previous line then. Either is fine by me.

@seancribbs
Copy link

This works as expected, maybe just add that @ to the line that invokes protoc-c, then 👍 to merge.

hazen pushed a commit that referenced this pull request Jan 4, 2014
Inital pass at generating Protocol Buffer headers/source for C
@hazen hazen merged commit 302c304 into develop Jan 4, 2014
@hazen hazen deleted the feature/bch/build-c-headers branch January 4, 2014 05:39
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