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.

Sign up for GitHub

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

Jump to bottom

Migrate from protoc to quick-protobuf and remove the buildscripts #3024

Closed
thomaseizinger opened this issue Oct 14, 2022 · 12 comments · Fixed by #3312
Closed

Migrate from protoc to quick-protobuf and remove the buildscripts #3024

thomaseizinger opened this issue Oct 14, 2022 · 12 comments · Fixed by #3312
Labels
difficulty:moderate getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Oct 14, 2022 •
edited

Description

Migrate our codebase from protoc to quick-protobuf.
We also want to get rid of the buildscripts and instead check the generated code into Git and add a CI step that verifies that they are directly generated from the protobuf files.

Motivation

Current Implementation

We depend on protoc being installed because prost no longer ships with it.

Are you planning to do it yourself in a pull request?

No.
@thomaseizinger thomaseizinger added difficulty:moderate help wanted getting-started Issues that can be tackled if you don't know the internals of libp2p very well labels Oct 14, 2022
@dariusc93
Copy link
Contributor

Havent ran into any issues relating to CI and protoc, but migrating from it to rust-protobuf would possibly be one less external dependency needed and would cause less confusion for users building the code

@mxinden
Copy link
Member

mxinden commented Oct 19, 2022

Adding another data point. We facilitated a rust-libp2p workshop today. Installing protoc was a significant slow down for many semi technical participants.

@thomaseizinger
Copy link
Contributor Author

I'd really appreciate help on this front @nloadholtes :)

@kckeiks kckeiks mentioned this issue Oct 20, 2022
Closed
4 tasks
@thomaseizinger
Copy link
Contributor Author

I'd really appreciate help on this front @nloadholtes :)

Just a heads-up that there is now #3050 @nloadholtes.

@thomaseizinger thomaseizinger changed the title Migrate from protoc to rust-protobuf Migrate away from protoc Oct 28, 2022
@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Oct 28, 2022 •
edited

@kckeiks Thanks a lot for your efforts on this issue! I also have to apologize for not putting my thoughts / intentions into this more clearly. The main thing I/we'd want out of this is to get rid of the protoc dependency. There are numerous ways of achieving this and I should have been more clear that particularly migrating to rust-protobuf is only one of the options.

As I mentioned on #3066, I'd prefer the code that quick-protobuf generates. Whilst thinking about this, I did have another idea. Our protobuf files basically never change because they are part of the spec. Instead of generating them on build-time, we could also just check them into Git. I see several benefits:

There are however also some downsides:

  • We need a separate CI job that ensures the generated code does not get modified
  • Some people may disagree on the aesthetics on generated code in Git

I'd also like to mention that this is what used to be done (#183) and later changed to prost in #1390.

@tomaka Did you experience any problems back then with the "checking in the generated code" approach that caused you to move away from it?

@libp2p/rust-libp2p-maintainers Please voice your opinion on this!

@tomaka
Copy link
Member

tomaka commented Oct 28, 2022

@tomaka Did you experience any problems back then with the "checking in the generated code" approach that caused you to move away from it?

No, as far as I remember I was in favor of keeping the "checking in the generator code" approach.

@mxinden
Copy link
Member

mxinden commented Nov 2, 2022

We need a separate CI job that ensures the generated code does not get modified

As long as we can make sure the checked-in code is correct, i.e. as long as we have a CI step, I am in favor of checking in the generated code.

@kckeiks Thanks a lot for your efforts on this issue!

Also want to thank you here for your work. Sorry in case it does not end up being merged.

@kckeiks
Copy link
Contributor

kckeiks commented Nov 2, 2022 •
edited

@kckeiks Thanks a lot for your efforts on this issue!

Also want to thank you here for your work. Sorry in case it does not end up being merged.

No worries! I'm glad we did not rush in :)

@thomaseizinger thomaseizinger mentioned this issue Nov 5, 2022
Closed
@thomaseizinger
Copy link
Contributor Author

@libp2p/rust-libp2p-maintainers Please voice your opinion on this!

It's been almost 3 weeks so I think we can move forward here! :)

@kckeiks Would you be open to expand #3066 to all crates but with checking the generated code into VCS instead of generating it with a build script?

@thomaseizinger thomaseizinger changed the title Migrate away from protoc Migrate from protoc to quick-protobuf and remove the buildscripts Nov 21, 2022
This was referenced Feb 17, 2023
Open
Closed
@thomaseizinger
Copy link
Contributor Author

Our CI is failing a lot recently due to the protoc action getting rate-limited in downloading protoc. I am aware that there are likely solutions to work around that but I am gonna take that as an opportunity to go ahead with this migration at the same time since we also want that unless someone beats me to it!

@dariusc93 dariusc93 mentioned this issue Dec 4, 2022
Closed
@kckeiks
Copy link
Contributor

kckeiks commented Dec 4, 2022

@libp2p/rust-libp2p-maintainers Please voice your opinion on this!

It's been almost 3 weeks so I think we can move forward here! :)

@kckeiks Would you be open to expand #3066 to all crates but with checking the generated code into VCS instead of generating it with a build script?

Hey @thomaseizinger I'm sorry I did not see your message before. Yeah, I would be happy to help out here and expand #3066. Have you started by any chance? I don't want our patches to overlap. I'll start working on this this coming week.

@thomaseizinger
Copy link
Contributor Author

Nope, I haven't started yet, so feel free to go ahead. Thank you! :)

@thomaseizinger thomaseizinger mentioned this issue Dec 6, 2022
Merged
4 tasks
@mxinden mxinden linked a pull request Dec 7, 2022 that will close this issue
Migrate from protoc to quick-protobuf #3066
Closed
4 tasks
@kckeiks kckeiks mentioned this issue Jan 7, 2023
Merged
4 tasks
@sergey-melnychuk sergey-melnychuk mentioned this issue Jan 16, 2023
Merged
@mxinden mxinden mentioned this issue Jan 18, 2023
Merged
4 tasks
@mergify mergify bot closed this as completed in #3312 Mar 2, 2023
mergify bot pushed a commit that referenced this issue Mar 2, 2023
@kckeiks
feat: migrate to quick-protobuf
db82e02 
Instead of relying on `protoc` and buildscripts, we generate the bindings using `pb-rs` and version them within our codebase. This makes for a better IDE integration, a faster build and an easier use of `rust-libp2p` because we don't force the `protoc` dependency onto them.

Resolves #3024.

Pull-Request: #3312.
@thomaseizinger thomaseizinger mentioned this issue Mar 3, 2023
Closed
@thomaseizinger thomaseizinger mentioned this issue Mar 20, 2023
Closed
@thomaseizinger thomaseizinger mentioned this issue May 1, 2023
Merged
4 tasks
mergify bot pushed a commit that referenced this issue May 8, 2023
@thomaseizinger
ci: remove protoc installation
2130923 
With all crates have received a release since #3024, building the baseline rustdoc no longer required `protoc` and we can thus remove it from our CI entirely.

Resolves #3539.

Pull-Request: #3858.
@mxinden mxinden mentioned this issue May 22, 2023
Closed
@mxinden mxinden mentioned this issue Sep 6, 2023
Open
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this issue Mar 8, 2024
@kckeiks
feat: migrate to quick-protobuf
bd1912b 
Instead of relying on `protoc` and buildscripts, we generate the bindings using `pb-rs` and version them within our codebase. This makes for a better IDE integration, a faster build and an easier use of `rust-libp2p` because we don't force the `protoc` dependency onto them.

Resolves libp2p#3024.

Pull-Request: libp2p#3312.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
difficulty:moderate getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted
Projects
None yet
Milestone
No milestone
5 participants
@tomaka @dariusc93 @thomaseizinger @mxinden @kckeiks