Closed alamb closed 11 months ago
cc @YangKeao @Xuanwo
Mostly LGTM.
But as discussed in https://github.com/tikv/pprof-rs/pull/136, we do have concerns on the adoption of protobuf v3.
Maybe skip the protobuf this time?
Maybe skip the protobuf this time?
Thanks -- PR has been upgraded
🎉
I wonder why this PR doesn't trigger any CI
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:
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: https://github.com/tikv/raft-rs/pull/460
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?
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 :+1:
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
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.
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.
—https://github.com/tokio-rs/prost/pull/657#issuecomment-1151227982
So is the consensus to check in the generated .rs files and remove the dependency on prost-build?
So is the consensus to check in the generated .rs files and remove the dependency on prost-build?
Yes :smile_cat: ! 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?
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
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
It appears this change was done by @sticnarf in https://github.com/tikv/pprof-rs/pull/166
Updates all dependencies to keep up with ecosystem
Closes https://github.com/tikv/pprof-rs/pull/153 Closes https://github.com/tikv/pprof-rs/pull/154 Closes https://github.com/tikv/pprof-rs/pull/155 Closes https://github.com/tikv/pprof-rs/pull/156