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

proto3 switch-over RFC #3

Merged
merged 3 commits into from
Nov 10, 2017
Merged

Conversation

cschuet
Copy link

@cschuet cschuet commented Nov 8, 2017

@SirVer
Copy link

SirVer commented Nov 8, 2017

Should the plan for cartographer_ros to become a thin wrapper using the gRPC based cores be documented in this RFC as well?

@cschuet
Copy link
Author

cschuet commented Nov 9, 2017

I think that's a great idea but should be documented in a different RFC.

Copy link
Contributor

@gaschler gaschler left a comment

Choose a reason for hiding this comment

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

Looks good, one comment

## Discussion Points
[discussion]: #discussion

[FirstPR]: https://github.com/googlecartographer/cartographer/pull/635
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be cartographer-project/cartographer#644 instead

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@cschuet cschuet merged commit e3262e8 into cartographer-project:master Nov 10, 2017
@cschuet cschuet deleted the proto3 branch November 10, 2017 08:40
- Refactor any code that uses proto2 features ('has...') and convert (at the very least) those messages that we intend to send over the wire to proto3 style.

This [PR][FirstPR] summarizes the plan for step 1 for the Cartographer repo:
- We add a script 'install_proto3.sh' which clones proto3 from HEAD, compiles and installs it to '/usr/local'.
Copy link
Contributor

Choose a reason for hiding this comment

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

From HEAD, or from a certain version, like Ceres?

Copy link
Author

Choose a reason for hiding this comment

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

Good call. I think Holger already has a follow-up PR where he does exactly that.

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.

5 participants