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

RSDK-4335 - Default to dynamic proto generation #134

Merged

Conversation

stuqdog
Copy link
Member

@stuqdog stuqdog commented Aug 3, 2023

Major Changes

Flips default behavior of dynamic proto generation and offline proto generation.

@stuqdog stuqdog requested a review from acmorrow August 3, 2023 12:03
@stuqdog stuqdog requested a review from a team as a code owner August 3, 2023 12:03
@stuqdog stuqdog requested review from maximpertsov and purplenicole730 and removed request for a team August 3, 2023 12:03
@stuqdog
Copy link
Member Author

stuqdog commented Aug 3, 2023

@acmorrow note that I updated defaults for OFFLINE_PROTO_GENERATION in addition to USE_DYNAMIC_PROTOS. Both flags have been necessary for me to compile, so I erred on the side of simplifying the cmake invocation as much as possible. Happy to chat if you had a preference against default enabling offline proto generation.

@acmorrow
Copy link
Member

acmorrow commented Aug 3, 2023

@acmorrow note that I updated defaults for OFFLINE_PROTO_GENERATION in addition to USE_DYNAMIC_PROTOS. Both flags have been necessary for me to compile, so I erred on the side of simplifying the cmake invocation as much as possible. Happy to chat if you had a preference against default enabling offline proto generation.

That seems reasonable to me given that without OFFLINE=ON you are likely to fail due to the missing protobuf version binding. I somewhat hope that once that is sorted out that we can revert OFFLINE to default OFF. The problem with offline mode is that it makes you dependent on the local tooling which is of unknown sanity. Can you add a comment on the ticket for fixing the missing version binding that we should evaluate restoring OFFLINE=OFF as the default as part of that work?

Copy link
Member

@acmorrow acmorrow left a comment

Choose a reason for hiding this comment

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

LGTM with a request to fold some details about reverting the default change for OFFLINE into an existing ticket.

@stuqdog
Copy link
Member Author

stuqdog commented Aug 3, 2023

@acmorrow yep that sounds reasonable, updated ticket.

@stuqdog stuqdog merged commit e8bddd0 into viamrobotics:main Aug 3, 2023
2 checks passed
@stuqdog stuqdog deleted the default-to-dynamic-proto-generation branch August 3, 2023 14:52
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.

2 participants