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

Protobuffs compiler take more opts other than just erl_opts #317

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants

Basically, this issue was brought up by #150 but it failed to get merged back. I did some improvements on that pull request and Im trying to bringing it up again since I believe it is pretty important for some ppl like me.

@tuncer tuncer commented on an outdated diff Jul 15, 2014

rebar.config.sample
@@ -89,6 +89,11 @@
%% Options for the ErlyDTL compiler
{erlydtl_opts, []}.
+%% == Protobuffs Compiler ==
+
+%% Options for the Protobuffs (https://github.com/basho/erlang_protobuffs) compiler
@tuncer

tuncer Jul 15, 2014

Contributor

Overlong line.

@tuncer tuncer commented on the diff Jul 15, 2014

src/rebar_protobuffs_compiler.erl
@@ -115,10 +124,11 @@ compile_each(_, []) ->
compile_each(Config, [{Proto, Beam, Hrl} | Rest]) ->
case needs_compile(Proto, Beam) of
true ->
- ?CONSOLE("Compiling ~s\n", [Proto]),
+ ?CONSOLE("Compiling proto file ~s\n", [Proto]),
@tuncer

tuncer Jul 15, 2014

Contributor

Did you change this to be similar to rebar_abnfc_compiler and rebar_neotoma_compiler? If so, it's fine, but it's better to make it a separate commit.

@UglyTroLL

UglyTroLL Jul 15, 2014

I'm not sure if I understand what did you mean, but this line is only printing out more specified log to the console.

@tuncer

tuncer Jul 15, 2014

Contributor

This change is unrelated to the patch's main goal, and it's fine to include because it fits in with what abnf_compiler and neotoma_compiler print. However, it's bad style (Structuring your commits) to not use a separate commit for this. In this case, adding an extra note (like While at it, made the log message more specific.) in the original commit message is also fine. Does that make sense?

@UglyTroLL

UglyTroLL Jul 15, 2014

Sure, the commit message was changed, please let me know if it looks good to you

@tuncer

tuncer Jul 16, 2014

Contributor

Thanks, but unfortunately it's not quite right (see Writing Commit Messages).

You wrote:

Protobuffs compiler take more opts other than just erl_opts, and print out more specific log message to console when compiling protobuf files

What it should look like:

Introduce {protobuffs_opts, []}

It wasn't possible to properly pass specific options to the protobuffs
compiler. To fix that, introduce {protobuffs_opts, []}.

While at it, adjust the "Compiling ..." log message to be more specific.
@UglyTroLL

UglyTroLL Jul 16, 2014

Sure, sounds good.
Commit message was changed.

@tuncer

tuncer Jul 17, 2014

Contributor

Thanks, but you extended the summary line, and as you can see here, here, and here, the summary is too long.

While fixing that, please also squash the overlong line fixup commit into the original commit.

@tuncer

tuncer Jul 17, 2014

Contributor

Also, feel free to append your name to the THANKS file.

@tuncer

tuncer Jul 17, 2014

Contributor

Thanks, now the length is fine, but you end the summary with . (see Writing Commit Messages). The THANKS file update can also be squashed into the original commit.

@UglyTroLL

UglyTroLL Jul 17, 2014

Done, sorry for that, I should've read the doc carefully :(

@tuncer

tuncer Jul 17, 2014

Contributor

No problem, and thanks for the quick fixes. While the summary should not end with ., it's missing from the last two sentences in the commit description of 2f886f3.

@UglyTroLL

UglyTroLL Jul 17, 2014

Haha sure, fixed :)

@UglyTroLL UglyTroLL Introduce a new option {protobuffs_opts, []}
It wasn't possible to properly pass specific options to the protobuffs
compiler. To fix that, introduce {protobuffs_opts, []}.

While at it, adjust the "Compiling ..." log message to be more specific
for compiling protobuffs, and added protobuffs_opts information to info_help/1.

Updated the THANKS file as well.
36ee265

any update on this?

Contributor

tuncer commented Jul 22, 2014

@UglyTroLL wrote

any update on this?

@seancribbs, you reported riak_pb issues in #150. Any thoughts?

@tsloughter, this appears to require a rebase, and we might want to add a protobuffs inttest, but it won't do much on travis-ci without extending the test-only deps in rebar.config.script first.

Sorry, any updates on this?

Contributor

tuncer commented Aug 14, 2014

BTW, I've asked @wb14123 if it's okay for this patch to be merged instead, and @wb14123 said Sure, as long as it works.

Member

tsloughter commented Aug 14, 2014

First, this can't be merged automatically. So please rebase.

Second, I think this will go into my examples of reasons to separate compilers that aren't rebar_erlc_compiler out as plugins :)

Contributor

tuncer commented Aug 14, 2014

@tsloughter wrote:

Second, I think this will go into my examples of reasons to separate compilers that aren't rebar_erlc_compiler out as plugins :)

I agree, see #144.

Contributor

seancribbs commented Aug 14, 2014

Don't wait on me for this, we will not be using those options.

Contributor

ferd commented Aug 21, 2014

Hi, so I was trying to move this forward, but I still get the same bug @jaredmorrow reported in #150 :

$  cp rebar/rebar riak_pb
$ cd riak_pb
$ make
$ make test
...
module 'encoding_test'
  encoding_test:8: pb_test_ (content encode decode)...[0.028 s] ok
  encoding_test:34: pb_test_ (deleted header encode decode)...[0.001 s] ok
  encoding_test:52: pb_test_ (indexes encode decode)...ok
  encoding_test:65: pb_test_ (empty content encode decode)...ok
  encoding_test:76: pb_test_ (empty repeated metas are removed/ignored)...ok
  encoding_test:87: pb_test_ (msg code encode decode)...*failed*
in function riak_pb_messages:msg_type/1
  called as msg_type(0)
in call from encoding_test:msg_code_encode_decode/1 (test/encoding_test.erl, line 94)
**error:undef


  [done in 0.071 s]
module 'bucket_props_codec_eqc'
=======================================================
  Failed: 1.  Skipped: 0.  Passed: 5.
ERROR: One or more eunit tests failed.
ERROR: eunit failed while processing /Users/ferd/code/experiments/riak_pb: rebar_abort
make: *** [test] Error 1

The same test breaks with the current master, though. The one that passed was rebar 2.1.0-pre R16B02-basho3 20131218_160534 git 2.1.0-pre-170-g117c0f7-dirty

I'm fine with the pull req and it's trivial to make it mergeable again (the diff is pasted below), but I'm still having absolutely no repository to test things on to make sure nothing breaks with this, at least more than right now.

If there's any project we have that works with the current master that I could try, that would be pretty good, although it'd also help if we could figure out what broke between 2.1.0 and the current master (that's out of scope for this pull req though).

diff --cc THANKS
index 3db3b4c,5fd3ed5..c219345
--- a/THANKS
+++ b/THANKS
@@@ -126,4 -126,4 +126,5 @@@ Yuki It
  alisdair sullivan
  Alexander Verbitsky
  Andras Horvath
 +Drew Varner
+ Zhibo Wei
diff --cc src/rebar_protobuffs_compiler.erl
index e89c700,dda47ba..6cab3f3
--- a/src/rebar_protobuffs_compiler.erl
+++ b/src/rebar_protobuffs_compiler.erl
@@@ -39,7 -39,16 +39,16 @@@
  %% ===================================================================

  compile(Config, _AppFile) ->
-     case rebar_utils:find_files("src", "^[^._].*\\.proto$") of
+     %% Check if we have imports_dir configured, if so, use the configured
+     %% value instead of 'src' to scan for all proto files
+     ProtobufOpts = rebar_config:get(Config, protobuffs_opts, []),
+     FoundFiles = case proplists:get_value(imports_dir, ProtobufOpts) of
+                      undefined ->
 -                         rebar_utils:find_files("src", ".*\\.proto$");
++                         rebar_utils:find_files("src", "^[^._].*\\.proto$");
+                      ImportDir ->
 -                         rebar_utils:find_files(ImportDir, ".*\\.proto$")
++                         rebar_utils:find_files(ImportDir, "^[^._].*\\.proto$")
+                  end,
+     case FoundFiles of
          [] ->
              ok;
          FoundFiles ->
Contributor

seancribbs commented Aug 21, 2014

@ferd That project uses a rebar plugin to generate code from the CSV file in src, and there is no reliable way to make sure it runs before the erlc compiler. This is a constant pain with plugins. If someone depends on riak_pb and happens to have the wrong rebar version in their project, it won't build correctly.

Contributor

tuncer commented Aug 21, 2014

@seancribbs I don't know if the plugin breaks with a newer rebar, or if there's another problem, but have you tried hooking pre_compile/2 instead of the current preprocess/2 fun?

Related: #104.

Contributor

seancribbs commented Aug 21, 2014

@tuncer I'll investigate that, thank you.

Contributor

tuncer commented Nov 16, 2014

What's the status here?

Contributor

tuncer commented Dec 21, 2014

Contributor

tomas-abrahamsson commented Dec 21, 2014

I don't have any opinions about the change as such, I'm not very well versed in the protobuffs (only gpb), but generally, the change seems to make sense.

I do get merge conflicts, but I think a resolution would be fairly straight-forward, probably adding to the ProtoFiles supplied as argument from the rebar_proto_compiler if imports_dir is specified, or something along those lines. (If I read basho's protobuffs_compile right, it appears to add to the src dir rather than replacing it, see https://github.com/basho/erlang_protobuffs/blob/develop/src/protobuffs_compile.erl#L70)

Contributor

tuncer commented Apr 2, 2015

ping?

Contributor

lrascao commented Apr 2, 2015

i too i'm not familiar with basho's protobuffs, in general i agree with tomas's input

Actually, I tried to merge the current master with my fix, and I found that this feature has already been supported from bf2b3e2

And the current solution is better than mine here, I believe this PR can be closed if no one has further concerns?

@ferd ferd closed this Apr 3, 2015

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