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

Add build for armv7 buf binaries #3450

Merged
merged 9 commits into from
Nov 7, 2024

Conversation

nimish-ks
Copy link
Contributor

This PR adds coss-compiled build support for linux arm LE linux armv7l architecture, used by some of the older ARM devices such as the raspberry pi.

      - name: setup-qemu
        uses: docker/setup-qemu-action@v3
        id: qemu
        with:
          # alpine image doesn't support linux/riscv64
          platforms: linux/386,linux/amd64,linux/arm64,linux/arm/v7,linux/arm/v6,linux/ppc64le,linux/s390x

I can see that linux armv7 support already exists in the ci.yml file. Do I need to make changes elsewhere?

This will also fix some downstream issues such as: usememos/memos#3306

@CLAassistant
Copy link

CLAassistant commented Nov 6, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@doriable doriable left a comment

Choose a reason for hiding this comment

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

Thank you submitting these changes! I left some comments for changes that would be needed. I applied these changes locally and tested out the build, and got armv7 binaries to build.

If these changes are merged, this will not be in effect until the next release and would also not be applied to old releases, so in order for downstream services like memos to pick this up, they would need to update to the release when it becomes available.

And just making sure I've addressed any open questions:

I can see that linux armv7 support already exists in the ci.yml file. Do I need to make changes elsewhere?

This is for our Docker images and should not affect your use-case. However, based on what I'm seeing in memos, they use buf to manage the generated code for their proto API definitions and they manage buf through npm/pnpm. So we'll also need to update the binaries available to our NPM packages if this merges.

make/buf/scripts/release.bash Outdated Show resolved Hide resolved
make/buf/scripts/release.bash Outdated Show resolved Hide resolved
make/buf/scripts/release.bash Outdated Show resolved Hide resolved
make/buf/scripts/release.bash Outdated Show resolved Hide resolved
make/buf/scripts/release.bash Outdated Show resolved Hide resolved
make/buf/scripts/release.bash Outdated Show resolved Hide resolved
@nimish-ks
Copy link
Contributor Author

nimish-ks commented Nov 7, 2024

If these changes are merged, this will not be in effect until the next release and would also not be applied to old releases, so in order for downstream services like memos to pick this up, they would need to update to the release when it becomes available.

I understand. Looking at the pace of the last few release cycles (few weeks to a month), I don't believe It would be a long wait. :)

This is for our Docker images and should not affect your use-case.

Cool.

However, based on what I'm seeing in memos, they use buf to manage the generated code for their proto API definitions and they manage buf through npm/pnpm. So we'll also need to update the binaries available to our NPM packages if this merges.

Yes, thought so too. It it this one? https://www.npmjs.com/package/@bufbuild/protobuf

Copy link
Member

@doriable doriable left a comment

Choose a reason for hiding this comment

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

Yes, thought so too. It it this one? https://www.npmjs.com/package/@bufbuild/protobuf

Yep, that's the one! Once we merge this, I'll update the NPM package publishing process to incorporate these changes for the next release, and then memos and similar projects should be able to update seamlessly.

Before I approve, could you please pull in the latest main and merge to the PR?

@nimish-ks
Copy link
Contributor Author

Before I approve, could you please pull in the latest main and merge to the PR?

I believe I already have in - 1633130. Please correct me if I am mistaken.

@doriable doriable changed the title feat: build armv7l buf binaries Add build for armv7 buf binaries Nov 7, 2024
Copy link
Member

@doriable doriable left a comment

Choose a reason for hiding this comment

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

Thank you!

@doriable doriable merged commit b4916b4 into bufbuild:main Nov 7, 2024
3 of 4 checks passed
@nimish-ks nimish-ks deleted the feat--add-linux-armv7l-support branch November 14, 2024 19:36
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.

4 participants