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

Move command & core proto defs to new subproject #4096

Merged
merged 3 commits into from Mar 27, 2020

Conversation

ghubstan
Copy link
Member

Protobuf definition files were moved from common and core to a new
protodefinition subproject.

The two main reasons for doing this are to speed up builds by not
having to regenerate common and core protobuf classes
every time a change is made in those subprojects, and to remove
the grpc cli's direct dependency on core and the transitive dependency
on common.

In order to accomplish this, cli's BisqCliMain was stripped of
its dependencies on common and core. Cli can only get the version
and balance now.

gRPC stub boilerplate was moved from BisqCliMain to a CliCommand
class to avoid some of the bloat that is going to happen as the
read-response loop supports more rpc commands.

Protobuf definition files were moved from common and core to a new
protodefinition subproject.

The two main reasons for doing this are to speed up builds by not
having to regenerate common and core protobuf classes
every time a change is made in those subprojects, and to remove
the grpc cli's direct dependency on core, and the transitive dependency
on common.

In order to accomplish this, cli's BisqCliMain was stripped of
its dependencies on common and core.  Cli can only get the version
and balance now.

gRPC stub boilerplate was moved from BisqCliMain to a CliCommand
class to avoid some of the bloat that is going to happen as the
read-response loop supports more rpc commands.
@ghubstan ghubstan requested a review from cbeams as a code owner March 26, 2020 13:28
@cbeams
Copy link
Member

cbeams commented Mar 26, 2020

This is looking good, @ghubstan. I'm going to push one or more commits to this branch as my review.

This is done primarily for concision. This change also repackages
bisq.grpc => bisq.proto.grpc in anticipation of repackaging the
definitions in pb.proto from 'protobuf' to 'bisq.proto'. There should
not be any compatibility issues with doing this, but it's out of scope
here. When complete, the relationship between bisq.proto.grpc and
bisq.proto will be more intuitively clear, i.e. that bisq.proto.grpc has
certain dependencies on bisq.proto classes, but not the other way
around.
Copy link
Member

@cbeams cbeams left a comment

Choose a reason for hiding this comment

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

ACK, please see my two review commits.

I didn't mention this in the commit comments, but I also removed the FIXMEs from grpc.proto about IDEA complaining about not having references to imported proto definitions from pb.proto. I'm not getting any errors any more, and I can navigate from grpc.proto to pb.proto by clicking, e.g. CMD-B on the imported definition. Find Usages on, e.g. the TradeStatistics2 proto def shows its usage in grpc.proto, etc, so I think everything is working as expected. This is probably because of some bug fix in a recent version of the IDEA protobuf plugin.

@ghubstan
Copy link
Member Author

I knew something was wrong with my codeStyleSettings, and there still is after setting editor code style scheme to "default", formatting imports with that, then resetting it back to "project".
My import organizer still doesn't match yours. Not sure what's going on with it.

I let ^ALT-L organize imports, but doing it through the reformat dialog gives the same result.
And when I do it, idea edits .idea/codeStyles/codeStyleConfig.xml

@ghubstan
Copy link
Member Author

Thanks for reviewing this so quickly.

@cbeams
Copy link
Member

cbeams commented Mar 26, 2020

You might want to rm -rf .idea followed by git checkout .idea to make sure all settings are in sync / correct. Also, in my experience it usually takes a restart of IDEA to pick up changes to the import table.

@ghubstan
Copy link
Member Author

rm -rf .idea followed by git checkout .idea solved it

@ripcurlx ripcurlx merged commit e2a01b6 into bisq-network:master Mar 27, 2020
@ghubstan ghubstan deleted the move-protobuf-defs branch March 30, 2020 18:40
@ripcurlx ripcurlx added this to the v1.3.0 milestone Apr 1, 2020
ghubstan added a commit to ghubstan/bisq that referenced this pull request Apr 2, 2020
Merging PR bisq-network#4096, which moved protobuf defs out of core and common,
left :seednode without its required dependency on guava, causing
NoSuchMethodErrors.
@ripcurlx ripcurlx added the is:priority PR or issue marked with this label is up for compensation label Apr 3, 2020
@cbeams cbeams moved this from To do to Done in Ship Bisq Daemon and API May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:priority PR or issue marked with this label is up for compensation
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants