Skip to content

Upgrade dependencies and bundle protoc.exe #146

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

Merged
merged 4 commits into from
May 17, 2022
Merged

Upgrade dependencies and bundle protoc.exe #146

merged 4 commits into from
May 17, 2022

Conversation

rkusa
Copy link
Collaborator

@rkusa rkusa commented May 10, 2022

Upgrade dependencies. This includes major upgrades. Unfortunately, the dependency that generates code out of proto files isn't bundling protoc anymore, so I am afraid we have to bundle it ourselves (or keep using the older version). Anyone against bundling protoc.exe in the repo?

@rkusa rkusa marked this pull request as draft May 10, 2022 16:46
@rkusa
Copy link
Collaborator Author

rkusa commented May 10, 2022

Converted back to draft as I have to figure out a way to bundle protoc.exe for both Windows and Linux (CI) ... any ideas? @rurounijones @justin-lovell

@justin-lovell
Copy link
Collaborator

For CI, perhaps use -

apt install -y protobuf-compiler

I would anticipate that for the local development experience, have a section for requiring the tool to be installed - I would encourage with Chocolately https://community.chocolatey.org/packages/protoc

@rurounijones
Copy link
Contributor

Agree with the above suggesions by @justin-lovell

@rkusa
Copy link
Collaborator Author

rkusa commented May 12, 2022

The protoc version for ubuntu is unfortunately too outdated, so I've ended up downloading a binary for linux from Github. Sorry for the CI try-and-error commit spam.

@rkusa rkusa marked this pull request as ready for review May 12, 2022 16:12
@rkusa
Copy link
Collaborator Author

rkusa commented May 12, 2022

I would anticipate that for the local development experience, have a section for requiring the tool to be installed - I would encourage with Chocolately https://community.chocolatey.org/packages/protoc

I am personally not a huge fan of any additional step to simply running cargo build. It additionally has the drawback that we don't control the version. So if we don't want to bundle the protoc.exe in the repo, the only alternative I can think of is to automatically download it as part of the cargo build.

@rurounijones
Copy link
Contributor

rurounijones commented May 12, 2022

I am personally not a huge fan of any additional step to simply running cargo build. It additionally has the drawback that we don't control the version. So if we don't want to bundle the protoc.exe in the repo, the only alternative I can think of is to automatically download it as part of the cargo build.

In rubyland there is a convention that you can run a setup command that does the first-time installation of stuff like this. Can we do the same here? Have a cargo setup that sets up the project dependencies so the full first-time steps are

cargo install
cargo setup
cargo build

Otherwise adding it to the cargo build where it checks for protoc.exe and downloads it if it is missing would be my preferred choice. Alternatively (and I know this will probably be too much work): Create our own rust Crate that does include the dependency and depend on that crate.

@justin-lovell
Copy link
Collaborator

the only alternative I can think of is to automatically download it as part of the cargo build.

I like that alternative. Is your thinking similar to this build.rs? It looks neat.

@rkusa
Copy link
Collaborator Author

rkusa commented May 14, 2022

Thanks for the feedback!

@rurounijones Such a setup convention does not exist in the Rust community (AFAIK).
@justin-lovell Yes, the download would be done in a build.rs. I'd add it to https://github.com/DCS-gRPC/rust-server/blob/main/stubs/build.rs.

I looked into downloading it, but after adding 5 dependencies just to download and extract protoc, I didn't like the idea too much anymore.

Alternatively (and I know this will probably be too much work): Create our own rust Crate that does include the dependency and depend on that crate.

Which made this look like a way better/easier solution. I've updated the PR accordingly. I've created the dependency under my own Github user for now https://github.com/rkusa/protoc-bundled. We could also move it to DCS-gRPC if we like.

I'd actually tend to keeping it as a Git dependency and not publish it to https://crates.io/. Instead of rolling our own dependency, we could also use something like https://lib.rs/crates/protoc-bin-vendored, but I am not too keen on pulling executables from yet another stranger.

In the future we could move to building protoc instead of bundling/downloading it. This currently requires cmake to be available, which brings us back to the same issue. However, a removal of the cmake dependencies for building seems to be in the work (tokio-rs/prost#620).

What do you think?

rurounijones
rurounijones previously approved these changes May 14, 2022
Copy link
Contributor

@rurounijones rurounijones left a comment

Choose a reason for hiding this comment

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

The current dependency in a separate dependency only accessible via git sounds like a good solution.

justin-lovell
justin-lovell previously approved these changes May 14, 2022
Copy link
Collaborator

@justin-lovell justin-lovell left a comment

Choose a reason for hiding this comment

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

learned a lot from this PR. cheers.

@rkusa rkusa dismissed stale reviews from justin-lovell and rurounijones via a1b5ff4 May 16, 2022 17:41
@rkusa rkusa merged commit 3a52d14 into main May 17, 2022
@rkusa rkusa deleted the upgrade-deps branch May 17, 2022 08:40
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.

3 participants