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

feat: migrate to quick-protobuf #3312

Merged
merged 79 commits into from
Mar 2, 2023
Merged

feat: migrate to quick-protobuf #3312

merged 79 commits into from
Mar 2, 2023

Conversation

kckeiks
Copy link
Contributor

@kckeiks kckeiks commented Jan 7, 2023 •
edited by thomaseizinger
Loading

Description

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.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@kckeiks kckeiks mentioned this pull request Jan 7, 2023
Closed
4 tasks
thomaseizinger
thomaseizinger approved these changes Jan 8, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Nice, this is what I was looking for, thanks!

Once we delete the other proto files, those should hopefully show up as moved rather than added.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

Dang, just saw CI doesn't actually like this. I am no bash expert but maybe try globbing into a variable first?

mxinden
mxinden reviewed Jan 10, 2023
View reviewed changes
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks to both of you for the work here!

core/src/generated/mod.rs Show resolved Hide resolved
@kckeiks
Copy link
Contributor Author

kckeiks commented Jan 11, 2023

Dang, just saw CI doesn't actually like this. I am no bash expert but maybe try globbing into a variable first?

I'll try that. I'm no expert on that either so I will probably have to push quite a bit to test it so I apologize in advance. Also, will update this PR soon.

@kckeiks
Copy link
Contributor Author

kckeiks commented Jan 12, 2023

Looks like it works https://github.com/libp2p/rust-libp2p/actions/runs/3898831952/jobs/6657940569

@kckeiks
Copy link
Contributor Author

kckeiks commented Jan 12, 2023

Looks like the diff is not working though. This run should have failed. https://github.com/libp2p/rust-libp2p/actions/runs/3898847742/jobs/6657969252

@thomaseizinger
Copy link
Contributor

Looks like the diff is not working though. This run should have failed. libp2p/rust-libp2p/actions/runs/3898847742/jobs/6657969252

Why did you expect it to fail? Did you modify any of the generated code in a commit?

@kckeiks
Copy link
Contributor Author

kckeiks commented Jan 12, 2023

Looks like the diff is not working though. This run should have failed. libp2p/rust-libp2p/actions/runs/3898847742/jobs/6657969252

Why did you expect it to fail? Did you modify any of the generated code in a commit?

I modified the glob so every *.proto file should be getting compiled including the ones outside /generated.

@thomaseizinger
Copy link
Contributor

Looks like the diff is not working though. This run should have failed. libp2p/rust-libp2p/actions/runs/3898847742/jobs/6657969252

Why did you expect it to fail? Did you modify any of the generated code in a commit?

I modified the glob so every *.proto file should be getting compiled including the ones outside /generated.

The problem is that those are untracked files so they don't seem to "fail" the git diff command. I tried by modifying an existing generated file, committing it and git diff fails properly on those.

@kckeiks
Copy link
Contributor Author

kckeiks commented Jan 12, 2023

Looks like the diff is not working though. This run should have failed. libp2p/rust-libp2p/actions/runs/3898847742/jobs/6657969252

Why did you expect it to fail? Did you modify any of the generated code in a commit?

I modified the glob so every *.proto file should be getting compiled including the ones outside /generated.

The problem is that those are untracked files so they don't seem to "fail" the git diff command. I tried by modifying an existing generated file, committing it and git diff fails properly on those.

Thanks for the help!

@thomaseizinger thomaseizinger mentioned this pull request Feb 27, 2023
Open
@mergify
Copy link
Contributor

mergify bot commented Feb 28, 2023

This pull request has merge conflicts. Could you please resolve them @kckeiks? 🙏

@kckeiks kckeiks mentioned this pull request Mar 1, 2023
Closed
@thomaseizinger
Copy link
Contributor

@mxinden This is ready to merge from my end. You will need to merge libp2p/github-mgmt#128 before you can merge here to update the required jobs.

@mergify
Copy link
Contributor

mergify bot commented Mar 1, 2023

This pull request has merge conflicts. Could you please resolve them @kckeiks? 🙏

@obi1kenobi obi1kenobi mentioned this pull request Mar 1, 2023
Merged
mxinden
mxinden previously approved these changes Mar 2, 2023
View reviewed changes
@mxinden mxinden added the send-it label Mar 2, 2023
@mergify mergify bot dismissed stale reviews from mxinden and thomaseizinger March 2, 2023 10:21

Approvals have been dismissed because the PR was updated after the send-it label was applied.

@mergify mergify bot merged commit db82e02 into libp2p:master Mar 2, 2023
@thomaseizinger
Copy link
Contributor

I think some of the changelog diffs have been applied to the wrong version as of the last release now.

@mxinden
Copy link
Member

mxinden commented Mar 2, 2023 •
edited
Loading

Good catch. @thomaseizinger can you send a pull request in the next couple of days?

@mxinden
Copy link
Member

mxinden commented Mar 2, 2023

Thanks @kckeiks and @thomaseizinger for all the work here!

@thomaseizinger
Copy link
Contributor

Good catch. @thomaseizinger can you send a pull request in the next couple of days?

My mind is engineering a semverlog tool as we speak to avoid this.

I can send a PR although it might take a while, have a busy weekend ahead and flying to Europe on Tuesday morning.

@thomaseizinger thomaseizinger mentioned this pull request Mar 3, 2023
Merged
4 tasks
@thomaseizinger
Copy link
Contributor

Sent it here: #3541

mergify bot pushed a commit that referenced this pull request Mar 10, 2023
@thomaseizinger
fix: move changelog entries to correct version
0cad636 
Whilst #3312 was in development, we pushed a new release out and forgot to move the changelog entries to the new version. Unfortunately, this is all still very manual until we have a solution for #2902 so this stuff keeps happening.

Pull-Request: #3541.
@bajtos bajtos mentioned this pull request Mar 30, 2023
Merged
@kckeiks kckeiks mentioned this pull request Apr 26, 2023
Closed
3 tasks
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request 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.
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
@thomaseizinger
fix: move changelog entries to correct version
621fe2f 
Whilst libp2p#3312 was in development, we pushed a new release out and forgot to move the changelog entries to the new version. Unfortunately, this is all still very manual until we have a solution for libp2p#2902 so this stuff keeps happening.

Pull-Request: libp2p#3541.
hrxi added a commit to nimiq/core-rs-albatross that referenced this pull request May 3, 2024
@hrxi
protoc isn't needed by libp2p anymore
ce8530c 
Hasn't been needed since libp2p 0.51.1.

libp2p/rust-libp2p#3312
@hrxi hrxi mentioned this pull request May 3, 2024
Merged
jsdanielh pushed a commit to nimiq/core-rs-albatross that referenced this pull request May 3, 2024
@hrxi @jsdanielh
protoc isn't needed by libp2p anymore
bdbf5e9 
Hasn't been needed since libp2p 0.51.1.

libp2p/rust-libp2p#3312
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomaseizinger thomaseizinger thomaseizinger left review comments

@rkuhn rkuhn rkuhn left review comments

@mxinden mxinden mxinden left review comments

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate from protoc to quick-protobuf and remove the buildscripts
5 participants
@kckeiks @thomaseizinger @obi1kenobi @mxinden @rkuhn