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

prost version upgrade brakes core build scripts #2848

Closed
divagant-martian opened this issue Aug 25, 2022 · 5 comments
Closed

prost version upgrade brakes core build scripts #2848

divagant-martian opened this issue Aug 25, 2022 · 5 comments

Comments

@divagant-martian
Copy link
Contributor

divagant-martian commented Aug 25, 2022

Summary

the upgrade to prost v0.11.0 includes some breaking changes, including prost no longer installing protoc. As a consequence the last version requires protoc to be explicitely/manually installed. See https://github.com/tokio-rs/prost/releases/tag/v0.11.0 and https://github.com/sigp/lighthouse/runs/8005207211?check_suite_focus=true#step:6:1020 for the error that now we get building libp2p's latests version

Expected behaviour

Either libp2p should continue to build or the new requirement should be noted in the release notes and installation guide

Actual behaviour

upgrading libp2p unexpectedly fails to build. See below copied error from CI

Debug Output

error: failed to run custom build command for `libp2p-core v0.35.1`

Caused by:
  process didn't exit successfully: `/home/runner/work/lighthouse/lighthouse/target/release/build/libp2p-core-730d1467ea8eddc3/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at '
  Could not find `protoc` installation and this build crate cannot proceed without
  this knowledge. If `protoc` is installed and this crate had trouble finding
  it, you can set the `PROTOC` environment variable with the specific path to your
  installed `protoc` binary.

  For more information: https://docs.rs/prost-build/#sourcing-protoc
  ', /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/prost-build-0.11.1/src/lib.rs:1227:10
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...
error: failed to compile `lighthouse v3.0.0 (/home/runner/work/lighthouse/lighthouse/lighthouse)`, intermediate artifacts can be found at `/home/runner/work/lighthouse/lighthouse/target`
make: *** [install] Error 101
Makefile:28: recipe for target 'install' failed

Possible Solution

ideally install protoc, but I assume there is a reason behind prost removing this from their build script. If this is not possible, protoc should be noted as a build dependency in the docs.

Version

v0.47.0

Would you like to work on fixing this bug?

Maybe but I'm not sure why the change was done in prost in the first place

@mxinden
Copy link
Member

mxinden commented Aug 26, 2022

Thanks for calling this out @divagant-martian!

I updated the release notes:

https://github.com/libp2p/rust-libp2p/releases/tag/v0.47.0

Do you think we should also adjust the changelogs? If so, would you mind preparing a pull request?

@divagant-martian
Copy link
Contributor Author

I was about to add the entry to the changelogs in the appropriate versions (those belonging to the 0.47.0 release as well as a patch changelogs entry but that might be unnecessary and prevent from actually patching the changelogs pinned in the tag?

So what I was about to do:

diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md
index 1626a471..b8531957 100644
--- a/core/CHANGELOG.md
+++ b/core/CHANGELOG.md
@@ -1,11 +1,15 @@
 # 0.35.1
 
 - Update to `p256` `v0.11.0`. See [PR 2636].
+- Patch CHANGELOG. See [PR 2851].
 
 [PR 2636]: https://github.com/libp2p/rust-libp2p/pull/2636/
+[PR 2851]: https://github.com/libp2p/rust-libp2p/pull/2851
 
 # 0.35.0
 
+- Update prost requirement from 0.10 to 0.11 which no longer installs the protoc Protobuf compiler.
+  Thus you will need protoc installed locally. See [PR 2788].
 - Drop `Unpin` requirement from `SubstreamBox`. See [PR 2762] and [PR 2776].
 - Drop `Sync` requirement on `StreamMuxer` for constructing `StreamMuxerBox`. See [PR 2775].
 - Use `Pin<&mut Self>` as the receiver type for all `StreamMuxer` poll functions. See [PR 2765].
@@ -19,6 +23,7 @@
 [PR 2776]: https://github.com/libp2p/rust-libp2p/pull/2776
 [PR 2765]: https://github.com/libp2p/rust-libp2p/pull/2765
 [PR 2797]: https://github.com/libp2p/rust-libp2p/pull/2797
+[PR 2788]: https://github.com/libp2p/rust-libp2p/pull/2788
 
 # 0.34.0
 

lmk what you think it's best.

@thomaseizinger
Copy link
Contributor

Patching the changelog itself doesn't need a changelog entry IMO :)

@divagant-martian
Copy link
Contributor Author

yeah agree, it's more the habit at this point

@mxinden
Copy link
Member

mxinden commented Aug 31, 2022

With #2851 merged, I will close here. @divagant-martian thanks for the patch!

@mxinden mxinden closed this as completed Aug 31, 2022
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

No branches or pull requests

3 participants