Migrate to gpb protobuf library #197
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced May 25, 2016
lukebakken
force-pushed
the
features/lrb/use-gpb
branch
from
04bd7ea
to
b56a8c3
Compare
lukebakken
force-pushed
the
features/lrb/use-gpb
branch
from
258b346
to
0f7a1c1
Compare
|
If the underlying reason for this (WIP) PR is building riak_pb with rebar3 under the assumption that a rebar3 protobuf plugin using gpb is available while one using basho/erlang_protobuffs is not, that might not be the case any longer. Please refer to #180 (RIAK-1796) (RIAK-2334) (RIAK-2756) . |
3 tasks
lukebakken
force-pushed
the
features/lrb/use-gpb
branch
3 times, most recently
from
eddcc2f
to
c5b804e
Compare
|
@lucafavatella once this is merged in the |
lukebakken
changed the title
|
@lukebakken any idea what's up with the dialyzer failures here? |
|
@JeetKunDoug I fixed the later two on the develop branch, they were introduced with the GSet changes. Not sure of the first. |
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
JeetKunDoug
force-pushed
the
features/lrb/use-gpb
branch
from
d41fda3
to
a976aa3
Compare
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
|
I think we should make the ignores in |
|
It is not possible to remove the ignores on the generated files. There is one function left that has a wrong specification: The function and types are defined like this: -spec decode_commit_hooks([ #rpbcommithook{} ]) -> [ commit_hook_property() ].
decode_commit_hooks(Hooks) ->
[ decode_commit_hook(Hook) || Hook <- Hooks,
Hook =/= #rpbcommithook{modfun=undefined, name=undefined} ].
-type commit_hook_property() :: [ {struct, [{commit_hook_field(), binary()}]} ].
-type commit_hook_field() :: binary().It calls -spec decode_modfun(#rpbmodfun{}, atom()) -> modfun_property().
decode_modfun(MF, linkfun) ->
{M,F} = decode_modfun(MF, undefined),
{modfun, M, F};
decode_modfun(#rpbmodfun{module=Mod, function=Fun}, commit_hook) ->
{struct, [{<<"mod">>, Mod}, {<<"fun">>, Fun}]};
decode_modfun(#rpbmodfun{module=Mod, function=Fun}=MF, _Prop) ->
try
{binary_to_existing_atom(Mod, latin1), binary_to_existing_atom(Fun, latin1)}
catch
error:badarg ->
error_logger:warning_msg("Creating new atoms from protobuffs message! ~p", [MF]),
{binary_to_atom(Mod, latin1), binary_to_atom(Fun, latin1)}
end.First of all, things of the structure So the best return type for -type modfun_property() :: {module(), function()} | {modfun, module(), function()} | {struct, [{binary(), binary()}]}.Notice that the last part of this type I suggest we change the return type and remove the entry in the line since we have already solved that problem. |
|
Thanks for the super-clear explanation @lehoff - I'll get that later on today. |
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
|
+1 |
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
updated tools.mk, getting eunit tests working
All eunit tests and dialyzer working with gpb library
Change gpb options so gpb.hrl is not a compile-time dependency
Remove need to add gpb/include to include search path
Restore _pb suffix for modules to prevent clashes
Changes required due to using _pb module name suffix
Ensure #rpberrorresp errmsg field is a binary
Separate out generation of _pb.erl and _pb.hrl files into erl_protogen make target
No longer using gpb application
Use gpb working version with epb compat
Add option to decode string fields into erlang binaries
Incorporate latest epb compat stuff in gpb
Update gpb dependency, manually change query atom since it is a keyword
Compile protobufs with latest gpb dependency
Update gpb to latest tag, re-run erl_protogen
Conversion to gpb compiles
updated tools.mk, getting eunit tests working
All eunit tests and dialyzer working with gpb library
Change gpb options so gpb.hrl is not a compile-time dependency
Remove need to add gpb/include to include search path
Restore _pb suffix for modules to prevent clashes
Changes required due to using _pb module name suffix
Ensure #rpberrorresp errmsg field is a binary
Separate out generation of _pb.erl and _pb.hrl files into erl_protogen make target
No longer using gpb application
Use gpb working version with epb compat
Add option to decode string fields into erlang binaries
Incorporate latest epb compat stuff in gpb
Update gpb dependency, manually change query atom since it is a keyword
Compile protobufs with latest gpb dependency
Update gpb to latest tag, re-run erl_protogen
Add test for optional fields
Update gpb to 3.26.4, re-gen from .proto files
add slack notifications
re-gen using R1602-basho10
Quote "query" for R15 compatibility
Remove unused `encode_type/2` to fix dialyzer issue.
It appears that `encode_type/2` is no longer used (we never pass `Mods`
as a parameter, except as `[]` in `encode_type/1`). Dialyzer figured
this out and was complaining:
riak_pb_dt_codec.erl:193: The pattern {AtomType, _} can never match the type 'false'"
Fix dialyzer warnings and update ignore-warnings
The existing entries in dialyzer.ignore-warnings were not correct anymore.
Added new ones and an explanation of how to change this when we get to a newer
Erlang version where we can use the -dialyzer directive in the .erl files.
Enable type_specs option
Remove exclusion of riak_yokozuna.erl since it is not needed
lukebakken
force-pushed
the
features/lrb/use-gpb
branch
from
bd3d51f
to
6bc4218
Compare
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.