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

chore: update dependencies for prost, nix, and protobuf #158

Closed
wants to merge 3 commits into from

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Aug 17, 2022

Updates all dependencies to keep up with ecosystem

Closes #153
Closes #154
Closes #155
Closes #156

@alamb alamb changed the title chore: update dependencies chore: update dependencies for prost, nix, and protobuf Aug 17, 2022
@alamb
Copy link
Contributor Author

alamb commented Aug 17, 2022

cc @YangKeao @Xuanwo

@Xuanwo
Copy link
Member

Xuanwo commented Aug 17, 2022

Mostly LGTM.

But as discussed in #136, we do have concerns on the adoption of protobuf v3.

Maybe skip the protobuf this time?

Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
@alamb
Copy link
Contributor Author

alamb commented Aug 18, 2022

Maybe skip the protobuf this time?

Thanks -- PR has been upgraded

Copy link
Member

@YangKeao YangKeao left a comment

Choose a reason for hiding this comment

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

LGTM

@alamb
Copy link
Contributor Author

alamb commented Aug 18, 2022

🎉

@YangKeao
Copy link
Member

I wonder why this PR doesn't trigger any CI

@YangKeao
Copy link
Member

https://github.com/tikv/pprof-rs/actions/runs/2883100562 It has been queued for a long time. It seems that the github has some bugs 🤦

@Xuanwo
Copy link
Member

Xuanwo commented Aug 19, 2022

https://github.com/tikv/pprof-rs/actions/runs/2883100562 It has been queued for a long time. It seems that the github has some bugs facepalm

Please ping a tikv's maintianer to check the runner status of the tikv org.

Seems other projects also been blocked, FYI: tikv/raft-rs#460

@alamb
Copy link
Contributor Author

alamb commented Aug 21, 2022

Please ping a tikv's maintianer to check the runner status of the tikv org.

Hi @Xuanwo -- how would I find who are the tikv maintainers?

Should I look for people merging stuff in https://github.com/tikv/tikv?

@YangKeao
Copy link
Member

Please ping a tikv's maintianer to check the runner status of the tikv org.

Hi @Xuanwo -- how would I find who are the tikv maintainers?

Should I look for people merging stuff in https://github.com/tikv/tikv?

It looks fine now 👍

…tish_2

Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
@YangKeao
Copy link
Member

The new version of prost_build removes the vendored protoc, which means it requires our user to pre-install the protoc to compile any libraries / binaries depend on the pprof with prost.

How do you think about it? @sticnarf @BusyJay

@sticnarf
Copy link
Contributor

The new version of prost_build removes the vendored protoc, which means it requires our user to pre-install the protoc to compile any libraries / binaries depend on the pprof with prost.

How do you think about it? @sticnarf @BusyJay

It is possible that the user of pprof-rs does not use prost-build because they don't have their own proto files. So, it may be a bit unfriendly to require all users to install protoc.

As the proto in pprof-rs does not change frequently, I think it may be acceptable to build the proto into rs file manually before publishing the crate.

@benesch
Copy link

benesch commented Aug 29, 2022

As the proto in pprof-rs does not change frequently, I think it may be acceptable to build the proto into rs file manually before publishing the crate.

This is what the prost maintainer recommends as well!

The one thing I think going for us in this area is that I don't think there are a large amount of libs that use tonic directly and/or with gRPC users can generally include the protobuf themselves. That said, I think the real solution moving forward is to check in the generated protobuf.

tokio-rs/prost#657 (comment)

@alamb
Copy link
Contributor Author

alamb commented Aug 31, 2022

So is the consensus to check in the generated .rs files and remove the dependency on prost-build?

@YangKeao
Copy link
Member

YangKeao commented Sep 1, 2022

So is the consensus to check in the generated .rs files and remove the dependency on prost-build?

Yes 😸 ! But I haven't found the best practice to generate files under the source directory. Should we add a binary rust crate to call prost-build to help us generate the files? Do you have any suggestions?

@tustvold
Copy link

tustvold commented Sep 1, 2022

Should we add a binary rust crate to call prost-build to help us generate the files? Do you have any suggestions?

You could look into using https://github.com/neoeinstein/protoc-gen-prost

@alamb
Copy link
Contributor Author

alamb commented Sep 4, 2022

I do not think I will have time to work on this ticket for at least the next several weeks. If anyone else is interested, it would be great to work on it.

This is relatively low on my priority list, so I may also just inline the pprof crate into our project and make the needed changes there (as we have already setup protoc in our build environments)

Thank you all for your comments

@alamb
Copy link
Contributor Author

alamb commented Sep 25, 2023

It appears this change was done by @sticnarf in #166

@alamb alamb closed this Sep 25, 2023
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.

None yet

6 participants