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

feat: Add vendored support #411

Merged
merged 7 commits into from
Jul 21, 2023
Merged

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Jul 13, 2023

Close #410

@Xuanwo Xuanwo force-pushed the add-vendor-support branch from 53bf03c to 36fccfd Compare July 13, 2023 18:13
Xuanwo added 2 commits July 14, 2023 02:15
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Copy link
Member Author

Xuanwo commented Jul 14, 2023

Should wait for #412

@Xuanwo
Copy link
Member Author

Xuanwo commented Jul 17, 2023

cc @andylokandy, @ekexium & @pingyu, would you like to take a review? Thanks!

@Xuanwo Xuanwo requested review from pingyu, andylokandy and ekexium July 17, 2023 05:38
Copy link

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

without install PROTOC

I'd prefer to bundle PROTOC or use protobuf-src instead. Like what is done in tikv/protobuf-build#67

@tisonkun
Copy link

Vendor generated looks like a step backward. If you do want to decouple with a PROTOC from the user side, it should be the default manner instead of a feature flag.

That is, we always generated, ship the generated, and check the generated updated with CI.

@Xuanwo
Copy link
Member Author

Xuanwo commented Jul 18, 2023

If you do want to decouple with a PROTOC from the user side, it should be the default manner instead of a feature flag.

We need a feature flag if we want to do this in build.rs. Otherwise, we will need an extra build script to do this which adds extra burden for developers.

Current design is:

  • Without vendored feature, generate code and use those code. (we can add a check in CI for this)
  • With vendored feature, use code directly.

Xuanwo added 2 commits July 18, 2023 13:07
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Copy link
Member Author

Xuanwo commented Jul 18, 2023

cc @tisonkun, would you like to take a review again?

@tisonkun
Copy link

@Xuanwo Perhaps you can ping @andylokandy for a review.

Or before we call any release, I can merge it now.

@andylokandy andylokandy merged commit 8b3ada2 into tikv:master Jul 21, 2023
@andylokandy
Copy link
Collaborator

Thanks!

tonic_build::configure()
.disable_doctests_for_types([".google.api.HttpRule"])
.emit_rerun_if_changed(false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way, it's interesting to know why the auto rerun is disabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

By the way, it's interesting to know why the auto rerun is disabled.

After this change, we are running codegen in a new bin instead of build.rs, the rerun hint for cargo is not working anymore. Instead, they just print to the console.

@Xuanwo Xuanwo deleted the add-vendor-support branch July 21, 2023 07:47
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.

Add vendor support so users don't need to install PROTOC
3 participants