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

chore: Bump Prost to 0.11 #98

Merged
merged 2 commits into from
Oct 4, 2022

Conversation

adamchalmers
Copy link
Contributor

@adamchalmers adamchalmers commented Sep 21, 2022

I noticed the Dependabot PRs (#89 #90 #92), but you really have to update all three Prost crates simultaneously because otherwise you'll get weird errors between versions. So, here's one PR to bump prost's family of crates.

Copy link
Contributor

@phyber phyber left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@adamchalmers
Copy link
Contributor Author

Looks good to me.

Thanks! As a first-time contributor, I can't merge yet, I'll need someone to allow CI jobs to run on this PR.

@mxinden
Copy link
Member

mxinden commented Sep 22, 2022

@adamchalmers thanks for the patch. This is missing the protoc installation step. See additional commits on #92.

Unfortunately #92 is currently failing on armv7-unknown-linux-gnu. I am guessing due to cargo expecting an arm version of protoc. Haven't investigated enough. Help appreciated.

@adamchalmers
Copy link
Contributor Author

adamchalmers commented Sep 22, 2022 via email

@adamchalmers
Copy link
Contributor Author

@mxinden Can you pls grant me permission to run workflows? "First-time contributors need a maintainer to approve running workflows. Learn more."

@mxinden
Copy link
Member

mxinden commented Sep 22, 2022

Approved on the latest changes. Is there a way to auto-approve your pull requests to run on CI? If not, I think you can just run the pipelines on your fork, right?

@adamchalmers
Copy link
Contributor Author

Yeah, I can run them on my fork.

@adamchalmers
Copy link
Contributor Author

adamchalmers commented Sep 22, 2022

I think the root cause is that the GH action for installing protoc doesn't support ARM. There's an issue open for ARM support: arduino/setup-protoc#25 -- notice that in the logs from this CI job, you can see it's downloading an x86-64 binary instead of an ARM binary.

Downloading archive: https://github.com/protocolbuffers/protobuf/releases/download/v3.20.2/protoc-3.20.2-linux-x86_64.zip

I think the erroneous line of their action is here:

const arch: string = osArch == "x64" ? "x86_64" : "x86_32";

So we'll need a PR to that repo to add ARM support, or fork it, or write our own action to install the arm target.

AFAICT prost isn't revealed to users, and it's not part of the public API for this crate. So upgrading to 0.11 isn't a big priority, and shouldn't block any other PRs/releases.

@mxinden
Copy link
Member

mxinden commented Sep 26, 2022

Thanks @adamchalmers for your work on arduino/setup-protoc#25!

@adamchalmers
Copy link
Contributor Author

My pleasure. I am a bit confused why we even need this action though -- can't we just install protoc from the package managers on windows/macos/ubuntu? Why do we need to install a particular version from GitHub release?

@adamchalmers
Copy link
Contributor Author

adamchalmers commented Sep 27, 2022

OK, I opened a PR to fix the issue on the setup-protoc action. In the meantime, I switched to my fork of the action on this branch, so we can see if CI passes. @mxinden could you please approve running CI on this branch?

@mxinden
Copy link
Member

mxinden commented Sep 27, 2022

@adamchalmers I triggered CI.

@adamchalmers
Copy link
Contributor Author

Looks like it doesn't quite work yet. I'll do more testing in my personal fork and figure out why.

@adamchalmers
Copy link
Contributor Author

adamchalmers commented Sep 28, 2022

So, the reason my PR to setup-protoc doesn't work: each of this project's 5 matrix jobs (named "Cross compile") are all actually running on Ubuntu x86_64. The various CPU architectures are only passed to the cargo build step, they don't actually affect the action runner. So the "install protoc" step correctly recognizes the environment as Linux x86_64.

I guess the solution is would have three parts:

  1. setup-protoc needs another configurable input for target OS arch
  2. The "Cross compile" job sets that input to ${{ matrix.target }}
  3. Ensure that protoc gets used for the cross-compile

EDIT: The other problem is that the cross compile targets include three targets for which there is no protoc binary:

  • mipsel-unknown-linux-gnu
  • powerpc-unknown-linux-gnu
  • armv7-unknown-linux-gnueabihf

I don't see any protoc releases for mipsel, and there's only 64-bit releases for powerpc and arm (but your target powerpc and arm7 are both 32-bit). So I guess using a static protoc for those architectures won't work.

Is it really important to test protocol buffer support on those architectures? If so, I guess we have to compile protoc from source. If not, maybe we just disable the protocol buffer feature on those architectures?

@mxinden
Copy link
Member

mxinden commented Sep 29, 2022

Thanks for all the investigation @adamchalmers.

Is it really important to test protocol buffer support on those architectures?

I think it is a nice-to-have. In other words, I am fine with not testing the protobuf feature on these platforms for now.

@adamchalmers
Copy link
Contributor Author

No worries! I'm a little disappointed I couldn't get it all to work how I wanted, but at least I learned a lot about GitHub actions along the way. Finally, this PR passes all the CI.

Prost 0.11 doesn't build a protobuf compiler from source anymore, instead requiring that one is available on the filesystem. Because Google's protobuf compiler is only available in 64-bit Linux, and most of the cross-compile targets use 32-bit Linux, I've disabled the 'protobuf' feature in cross-compilation.

Signed-off-by: Adam Chalmers <adam.s.chalmers@gmail.com>
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.

Again, thank you for your help here @adamchalmers.

.github/workflows/rust.yml Show resolved Hide resolved
Signed-off-by: Max Inden <mail@max-inden.de>
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.

None yet

3 participants