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

ci: install protoc from repositories where possible #3258

Merged
merged 5 commits into from
Dec 30, 2022

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Dec 19, 2022

Description

With the addition of more CI jobs, we are constantly running into API limits on setting up protoc. For all jobs that run on ubuntu, we can install it from apt instead.

On ubuntu 22.04, which is what ubuntu-latest points to, this installs protoc v3.12.4.

Notes

Links to any relevant issues

Open Questions

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

@MarcoPolo
Copy link
Contributor

I’ve not looked closely, but let’s make sure this installs an up to date version. (Not something a year old)

@thomaseizinger
Copy link
Contributor Author

I’ve not looked closely, but let’s make sure this installs an up to date version. (Not something a year old)

It is only meant to be a temporary fix until we ship #3066.

I just ran the command in a ubuntu:jammy (22.04) docker image and got protoc 3.12.4 which is about 2.5 years old. What are your concerns in installing an older version?

@MarcoPolo
Copy link
Contributor

MarcoPolo commented Dec 19, 2022 via email

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Dec 20, 2022

On my phone, but this has some context: libp2p/specs#465 (comment)

Thanks for linking this. At the moment, I'd just like to ease us from the burden of having constantly red CI jobs, PRs being dequeued from the merge queue and having to baby-sit and restart them constantly until we finally get a window where we don't hit the API limit upon installing protoc.

Long-term ,we are moving away from requiring protoc. I'd assume that by the time this decision is made and enforced across all libp2p implementations, we will have long merged #3066.

@libp2p/rust-libp2p-maintainers Do you agree with this?

@thomaseizinger thomaseizinger marked this pull request as ready for review December 20, 2022 03:44
@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Dec 20, 2022

Funnily enough, our interoperability tests install protoc through the exact same mechanism: https://github.com/libp2p/test-plans/blob/34b6eaeffd9f4a011e41ad8b2d8103229e750c12/ping/rust/Dockerfile#L4

I'd say that is an argument for doing it the same way here as well because it guarantees us that what we test interoperability on is actually the same thing as we write our unit tests etc for.

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.

I am fine moving forward here. Issues due to the protoc version should be detected at compile time, e.g. the proto3 optional issue would result in a compile time failure as our Rust code would expect an Option. libp2p/specs#465 (comment)

@mxinden
Copy link
Member

mxinden commented Dec 30, 2022

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Dec 30, 2022

refresh

✅ Pull request refreshed

@mergify mergify bot merged commit 68d0f88 into master Dec 30, 2022
@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Feb 15, 2023

I am backporting this to v0.49 because the actual backport is otherwise super painful.

@thomaseizinger
Copy link
Contributor Author

@Mergifyio backport rust-v0490-testplan

mergify bot pushed a commit that referenced this pull request Feb 15, 2023
With the addition of more CI jobs, we are constantly running into API limits on setting up protoc. For all jobs that run on ubuntu, we can install it from `apt` instead.

On ubuntu 22.04, which is what `ubuntu-latest` points to, this installs `protoc v3.12.4`.

(cherry picked from commit 68d0f88)

# Conflicts:
#	.github/workflows/cache-factory.yml
#	.github/workflows/ci.yml
@mergify
Copy link
Contributor

mergify bot commented Feb 15, 2023

backport rust-v0490-testplan

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants