Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Pluggable proto compilers gpb #203

Merged
merged 4 commits into from Oct 31, 2014

Conversation

Projects
None yet
5 participants
Contributor

tomas-abrahamsson commented Dec 22, 2013

Hi, this pull request adds support for the gpb (https://github.com/tomas-abrahamsson/gpb) as a compiler for Google protocol buffers (*.proto files). A new config option, proto_compiler, is used to select which compiler to use, the protobuffs is the default, for backwards compatibility reasons.

The pull request is divided into two commits: the first commit introduces a plugin mechanism for protocol buffer compilers and the protobuffs compiler is turned into a plugin, the second commit adds a plugin for the gpb protocol buffer compiler.

Such a plugin is a module that implements the functions proto_compile/3, proto_clean/3, proto_info/2 and key/0. This is very similar to how other compiler modules are plugged in, but on a sub-level.

I couldn't find any other rebar compiler that supported alternative implementations, from which I could borrow ideas, so I had to find some sensible way to go. Hope this is reasonable.

BRs
Tomas

Contributor

tuncer commented Jan 11, 2014

Thanks @tomas-abrahamsson. I haven't tested/reviewed the patch (yet), but generally speaking this looks reasonable to me. Also, I'd welcome your opinion on non-OTP compiler support in #144.

cc @jaredmorrow

@tsloughter tsloughter closed this Jun 15, 2014

Contributor

tuncer commented Jun 15, 2014

@tsloughter why did you close this?

Contributor

ferd commented Jun 15, 2014

Hi, this issue was closed in an attempt to do quick basic filtering, with the benediction of rebar project owners. These issues and pull requests are not issues or code we're spitting on, but given the burden of the task and how much code rot may have happened since these were open is unknown from maintainers at this time. All tickets prior to March 2014 were closed and will be reopened on a per-request basis if we see interest from the reporter or contributor, or if some of the issues reported are still valid after the various patches that have made it since they were opened.

This is a fairly brutal first step to help us get a proper understanding of what is still valid or not, but that has been proven efficient in the past. Sorry for the inconvenience, things should go smoother from there on.

Contributor

tomas-abrahamsson commented Jun 16, 2014

Hi, I understand the need to prune the list of issues every once in a while, but I'd welcome it if this issue could be re-opened. The patch still applies without conflicts, and I think there might be a need for it, based on a question for it on the erlang-questions mailing list, see http://thread.gmane.org/gmane.comp.lang.erlang.general/69167 (granted though that this is the only question I've seen about how to use gpb with rebar, and also granted that there exists a workaround, albeit it is a bit clumsy)

The #144 was mentioned, previously, I think that's definitely the way to generally aim for.

@ferd ferd added the enhancement label Jun 16, 2014

@ferd ferd reopened this Jun 16, 2014

Contributor

tuncer commented Jun 17, 2014

@tomas-abrahamsson wrote:

The #144 was mentioned, previously, I think that's definitely the way to generally aim for.

If we merge this patch, and later implement #144, would the new rebar.config option still be relevant? If that's the case, it's probably safe to merge this without waiting for #144.

Contributor

tomas-abrahamsson commented Jun 17, 2014

If we merge this patch, and later implement #144, would the new rebar.config option still be relevant?

Yes, I should think that will mostly be the case, but I suppose it will ultimately depend on how the #144 will be implemented. For example, this patch adds a configuration option {proto_compiler,ProtoCompiler}. where ProtoCompiler :: protobuffs | gpb which makes sense in this context, but if #144 introduces some notion of file name extensions to compiler mapping, then this configuration option might need to be adapted somehow, to follow the new pattern, and it is difficult to say beforehand exactly how.

Contributor

tomas-abrahamsson commented Sep 27, 2014

I rebased the branch onto the latest master, since there was a merge conflict. (I took care to include/port/adapt the change that caused the conflict, it was the "Fix OS X resource fork handling (Reported-by: Richard O'Keefe)")

Contributor

tuncer commented Oct 23, 2014

Not a big issue, but for consistency, any objection to writing the name as "Rascao" and keep the sources ASCII?

@tuncer tuncer commented on an outdated diff Oct 23, 2014

inttest/proto_gpb/proto_gpb_rt.erl
+ "test3_gpb.beam",
+ "test4_gpb.beam",
+ "test5_gpb.beam"]).
+
+files() ->
+ [
+ {copy, "../../rebar", "rebar"},
+ {copy, "rebar.config", "rebar.config"},
+ {copy, "include", "include"},
+ {copy, "src", "src"},
+ {create, "ebin/foo.app", app(foo, ?MODULES)}
+ ].
+
+run(_Dir) ->
+ ?assertMatch({ok, _}, retest_sh:run("./rebar get-deps", [])),
+ ?assertMatch({ok, _}, retest_sh:run("./rebar compile", [])),
@tuncer

tuncer Oct 23, 2014

Contributor

We could use ./rebar prepare-deps instead of the two commands, if you want to.

@tuncer tuncer commented on an outdated diff Oct 23, 2014

inttest/proto_gpb/proto_gpb_rt.erl
+ [
+ {copy, "../../rebar", "rebar"},
+ {copy, "rebar.config", "rebar.config"},
+ {copy, "include", "include"},
+ {copy, "src", "src"},
+ {create, "ebin/foo.app", app(foo, ?MODULES)}
+ ].
+
+run(_Dir) ->
+ ?assertMatch({ok, _}, retest_sh:run("./rebar get-deps", [])),
+ ?assertMatch({ok, _}, retest_sh:run("./rebar compile", [])),
+ ok = check_beams(true),
+ ok = check_debug_info(true),
+ ?assertMatch({ok, _}, retest_sh:run("./rebar clean", [])),
+ ?assertMatch({ok, _}, retest_sh:run("./rebar compile", [])),
+ ok = check_beams(true),
@tuncer

tuncer Oct 23, 2014

Contributor

The test seems to be based on inttest/erlc, and if you don't necessarily need to check for debug info, we might want to simplify it.

@tuncer tuncer commented on an outdated diff Oct 23, 2014

inttest/proto_protobuffs/proto_protobuffs_rt.erl
+ {copy, "rebar.config", "rebar.config"},
+ {copy, "include", "include"},
+ {copy, "src", "src"},
+ {create, "ebin/foo.app", app(foo, ?MODULES)}
+ ].
+
+run(_Dir) ->
+ ?assertMatch({ok, _}, retest_sh:run("./rebar get-deps", [])),
+ ?assertMatch({ok, _}, retest_sh:run("./rebar compile", [])),
+ ok = check_beams(true),
+ ok = check_debug_info(true),
+ ?assertMatch({ok, _}, retest_sh:run("./rebar clean", [])),
+ ?assertMatch({ok, _}, retest_sh:run("./rebar compile", [])),
+ ok = check_beams(true),
+ ok.
+
@tuncer

tuncer Oct 23, 2014

Contributor

As this test looks very similar to the other one, see my comments above.

@tuncer tuncer commented on the diff Oct 23, 2014

rebar.config.sample
@@ -89,6 +89,16 @@
%% Options for the ErlyDTL compiler
{erlydtl_opts, []}.
+%% == Proto compiler ==
+{proto_compiler, protobuffs}.
+%% Available compilers for protocol buffer files (*.proto):
@tuncer

tuncer Oct 23, 2014

Contributor

Is it "protocol buffer files" or "protocol buffers files"?

@tuncer tuncer commented on the diff Oct 23, 2014

rebar.config.sample
@@ -89,6 +89,16 @@
%% Options for the ErlyDTL compiler
{erlydtl_opts, []}.
+%% == Proto compiler ==
+{proto_compiler, protobuffs}.
+%% Available compilers for protocol buffer files (*.proto):
+%% protobuffs (default)
+%% gpb
+
+%% Options for the gpb protocol buffer compiler,
@tuncer

tuncer Oct 23, 2014

Contributor

Is it "protocol buffer" or "protocol buffers"?

@tuncer tuncer commented on an outdated diff Oct 23, 2014

src/rebar_proto_compiler.erl
+%% LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+%% OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+%% THE SOFTWARE.
+%% -------------------------------------------------------------------
+-module(rebar_proto_compiler).
+
+-export([compile/2,
+ clean/2]).
+
+%% for internal use only
+-export([info/2]).
+
+-include("rebar.hrl").
+
+-record(proto_compiler,
+ {key :: atom(), %% Corresponds to the {proto_compiler,Key}
@tuncer

tuncer Oct 23, 2014

Contributor

To work better in erlang-mode (erlang.el), that comment should be either on a separate line or use a single % if left on this line.

@tuncer tuncer commented on an outdated diff Oct 23, 2014

src/rebar_proto_compiler.erl
+
+%% ===================================================================
+%% Internal functions
+%% ===================================================================
+
+info(help, compile) ->
+ info_help("Build protocol buffer (*.proto) sources", compile);
+info(help, clean) ->
+ info_help("Delete protocol buffer (*.proto) build results", clean).
+
+info_help(GeneralDescr, Cmd) ->
+ ?CONSOLE(
+ "~s.~n"
+ ++ "~n"
+ ++ "Valid rebar.config options:~n"
+ ++ " {proto_compiler,Compiler}~n"
@tuncer

tuncer Oct 23, 2014

Contributor

Maybe space after ,?

@tuncer tuncer commented on an outdated diff Oct 23, 2014

src/rebar_proto_compiler.erl
+ case code:ensure_loaded(Module) of
+ {module, Module} ->
+ lists:all(fun({Function, Arity}) ->
+ erlang:function_exported(Module, Function, Arity)
+ end,
+ [{key, 0},
+ {proto_compile, 3},
+ {proto_clean, 3},
+ {proto_info, 2}]);
+ _ ->
+ false
+ end.
+
+select_proto_compiler(Config) ->
+ Default = get_default_compiler(),
+ Key = rebar_config:get(Config, proto_compiler, Default),
@tuncer

tuncer Oct 23, 2014

Contributor

If not strictly necessary, we shouldn't use the inheriting rebar_config get fun for new options (see #58). Same goes for gbp_opts.

Member

tsloughter commented Oct 23, 2014

I wouldn't expect #144 to ever be implemented, so definitely don't bother considering it :)

@tuncer tuncer commented on an outdated diff Oct 23, 2014

src/rebar_proto_compiler.erl
+ case lists:keyfind(Key, #proto_compiler.key, AvailCompilers) of
+ #proto_compiler{module=CompilerModule} ->
+ CompilerModule;
+ false ->
+ ?ABORT("No such protocol buffer compiler known, '~s'~n"
+ ++ "The following are known:~n"
+ ++ "~s~n",
+ [Key, format_proto_compiler_list()])
+ end.
+
+format_proto_compiler_list() ->
+ Default = get_default_compiler(),
+ lists:flatten(
+ [?FMT(" ~p~s~n", [Key, if Key == Default -> " (default)";
+ true -> ""
+ end])
@tuncer

tuncer Oct 23, 2014

Contributor

Why not initialize the string before the lists:flatten call and avoid the if expression here?

@tuncer tuncer commented on an outdated diff Oct 23, 2014

src/rebar_proto_gpb_compiler.erl
+ filename:join(["src",
+ Prefix ++ filename:basename(Proto, ".proto") ++ ".erl"]).
+
+hrl_relpath(Proto, Prefix) ->
+ filename:join(["include",
+ Prefix ++ filename:basename(Proto, ".proto") ++ ".hrl"]).
+
+delete_file(File) ->
+ case file:delete(File) of
+ ok ->
+ ok;
+ {error, enoent} ->
+ ok;
+ {error, Reason} ->
+ ?ERROR("Failed to delete ~s: ~p\n", [File, Reason])
+ end.
@tuncer

tuncer Oct 23, 2014

Contributor

Should this be shared with the code base or should we modify rebar_file_utils?

@tuncer tuncer commented on an outdated diff Oct 23, 2014

src/rebar_proto_gpb_compiler.erl
+ []);
+proto_info(help, clean) ->
+ ?CONSOLE("", []).
+
+gpb_opts(Config) ->
+ rebar_config:get(Config, gpb_opts, []).
+
+gpb_is_present() ->
+ code:which(gpb) =/= non_existing.
+
+compile_gpb(Source, _Target, Config) ->
+ SourceFullPath = filename:absname(Source),
+ DefaultDestOpts = [{o_erl, "src"}, {o_hrl, "include"}],
+ SelfIncludeOpt = [{i,filename:dirname(SourceFullPath)}],
+ GpbOpts = gpb_opts(Config) ++ DefaultDestOpts ++ SelfIncludeOpt,
+ ok = filelib:ensure_dir(filename:join("ebin","dummy")),
@tuncer

tuncer Oct 23, 2014

Contributor

missing space after , for consistency

Contributor

tuncer commented Oct 23, 2014

@tsloughter wrote:

I wouldn't expect #144 to ever be implemented

Why do you think that?

Contributor

tuncer commented Oct 23, 2014

@tomas-abrahamsson I did a quick review, but didn't test or review it in depth. In addition to that, I was wondering if any of the compilers can be made to return protobuf (not compile:file) warnings and errors for rebar to format them locally like we do with .xrl or .erl.

Contributor

lrascao commented Oct 24, 2014

@tuncer wrote:

Not a big issue, but for consistency, any objection to writing the name as "Rascao" and keep the sources ASCII?

No objection from me

Contributor

tomas-abrahamsson commented Oct 24, 2014

Thanks for the review, I will look into it.

Regarding:

Is it "protocol buffer" or "protocol buffers"?

Not being a native English speaker, I checked with google what seems to be most common The results seems to indicate that the non-plural form is more common.

Number of results: /

"protocol buffers files": 6790 / 0
"protocol buffer files": 7770 / 0

"protocol buffers compiler": 40800 / 2
"protocol buffer compiler": 80600 / 804

(I googled for the phrase, within quotation marks, then added site:developers.google.com/protocol-buffers/ to limit the search to Google's own website for protobuf)

Contributor

tomas-abrahamsson commented Oct 27, 2014

Hi again, I have fixed up my branch locally, but have one question before I push some amended changes:

If not strictly necessary, we shouldn't use the inheriting
rebar_config get fun for new options (see #58). Same goes for
gbp_opts.

I did not fully understand: I agree it does not make sense to use the inheriting rebar_config:get fun, but what should be used instead? The #58 does not appear to contain any code, and is still open. The rebar_config.erl has been updated with an is_recursive function, as part of #56, but I do not see how I could use that. My guess would be rebar_config:get_local is that the way to go? (Guess is based on some git revision history digging, as there was not very much guiding documentation in rebar_config).

Also, I believe I never ansered this question:

In addition to that, I was wondering if any of the compilers can be made
to return protobuf (not compile:file) warnings and errors for rebar to format
them locally like we do with .xrl or .erl.

Yes, gpb can do that. But it appears protobuffs cannot; I briefly checked both the ngerakines' and basho's implementations, which I believe are the most widely used. (https://github.com/ngerakines/erlang_protobuffs and https://github.com/basho/erlang_protobuffs) I found no format_error or similar function.

What did you have in mind regarding this? (not all proto compilers supporting it would mean that any use of it could currently only go into the gpb-specific proto compiler and so would be of limited general use?)

Member

tsloughter commented Oct 29, 2014

@tuncer I don't think it will ever be implemented, unless you are currently working on it. No one else is and I don't know of anyone who plans to at any point.

Contributor

tuncer commented Oct 29, 2014

@tomas-abrahamsson

My guess would be rebar_config:get_local is that the way to go?

Yes, unless we really want to inherit options.

What did you have in mind regarding this? (not all proto compilers supporting it would mean that any use of it could currently only go into the gpb-specific proto compiler and so would be of limited general use?)

If gpb supports it and we can handle this in rebar_proto_gpb_compiler without affecting rebar_protobuffs_compiler, we might be able to print warnings/errors via rebar_base_compiler (see rebar_lfe_compiler.erl or rebar_erlc_compiler.erl).

Contributor

tuncer commented Oct 29, 2014

@tsloughter

@tuncer I don't think it will ever be implemented, unless you are currently working on it. No one else is and I don't know of anyone who plans to at any point.

Okay, I see your point, but it's a low-prio ticket, so nobody got around to it yet.

tomas-abrahamsson and others added some commits Dec 17, 2013

Introduce pluggable protocol buffer compilers
Make it possible for plug in alternative protocol buffer compilers.
The compilers are picked up based on if they export all of the
functions key/0, proto_compile/3, proto_clean/3 and proto_info/2.
The set of compiler modules to choose from, is fetched from the rebar
application environment, from the app_dir modules.

A new config option, {proto_compiler,Compiler}, specifies which of
the available protocol buffer compilers to use. The 'protobuffs'
compiler is now one such compiler (the only one), and it is also the
default, for backwards compatibility.
Add support for compiling proto files using gpb
This adds the config option {proto_compiler,gpb} for selecting gpb
as the compiler for protocol buffer files. When gpb is used as
compiler, it reads the gpb_opts config item for options.
Add proto compiler gpb inttest
exercises rebar/gpb integration

The bulk of these tests are written by Luis Rascão, hence he is the
author of this commit. As the committer, I have cherry-picked his two
commits 4c87bcd and ebb8182, from the feature/support_gpb_protobuf
branch in the git://github.com/lrascao/rebar repo, and have slightly
adapted it to fit this pluggable-proto-compilers-gpb branch.

Update the THANKS file
Add inttest for default proto compiler (protobuffs)
This borrows heavily from the inttest for gpb, many thanks
to Luis Rascão for providing a most useful example.
Contributor

tomas-abrahamsson commented Oct 29, 2014

@tuncer I've just pushed a reworked version, I believe I've addressed all issues, will update with more comments in a minute.

About:

If gpb supports it and we can handle this in rebar_proto_gpb_compiler without affecting rebar_protobuffs_compiler, we might be able to print warnings/errors via rebar_base_compiler (see rebar_lfe_compiler.erl or rebar_erlc_compiler.erl).

It turned that even though there is a gpb_compile:format_error, the error message does not have the exact same format as the errors from the erlang compiler, so it was not possible to hook into that right away. I could updat gpb, but I would also have to add a check for the case if someone has an older version and fallback so that proto compilation errors are not hidden. Maybe in some future version.

Contributor

tomas-abrahamsson commented Oct 29, 2014

The following are the issues that you commented on, but where I haven't changed the code:

In rebar.config.sample:
Is it "protocol buffer files" or "protocol buffers files"?

Not changed: the singular form seems to dominate, as per the investigation

In rebar.config.sample:
Are we missing sample config for the other compiler [=protobuffs] here?

It seems the existing rebar_protobuffs_compiler only reads
options from erl_opts, it does not seem to pick up any other
options.

That's it. I think the rest of the issues have all been addressed. I pushed an modified version of the branch, in the same vein as before of not adding extra commits for fixups, but I see now that github says the previous comment is for an outdated diff. Sorry about that. If I should revert the branch to look like previously, but with fixup commits instead, please just say it and I will do it. (I was unsure what github would do. Now I know)

Contributor

tuncer commented Oct 30, 2014

@tomas-abrahamsson the git history is fine. github's review system fails to properly reserve old patch sets with the associated review comments. @ferd, @tsloughter, can one of you do a thorough test and code review.

Contributor

ferd commented Oct 30, 2014

It's impossible for me to test this properly at this point, as was discussed back in #365 (comment)

I couldn't find any single project in the wild, pre or post patch that worked with protobuffs, but I did manage to compile some of the projects with either of these branches. At this point I've given up on backwards compatibility because we apparently have a purely broken protobuffs implementation in master already.

I'll give this pull request an overall review and run again to see if everything still makes sense, but I defer to our protobuffs users to make sure things work here. And thanks again for the whole lot of work that went through that stuff!

Contributor

tuncer commented Oct 30, 2014

@ferd works for me.

ferd added a commit that referenced this pull request Oct 31, 2014

@ferd ferd merged commit 17e0b14 into rebar:master Oct 31, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Contributor

tomas-abrahamsson commented Oct 31, 2014

Thanks for merging!
BRs

Contributor

lrascao commented Nov 3, 2014

i think i've found a bug related with this feature, should i report it here or open a new issue?

Contributor

ferd commented Nov 3, 2014

I'd open an issue, but mention people who were directly involved with this pull request in it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment