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 link flags for grpc armv7 wheels (attempt #2) #63521

Merged
merged 1 commit into from
Jan 6, 2022

Conversation

allenporter
Copy link
Contributor

@allenporter allenporter commented Jan 5, 2022

Proposed change

Add link flags, as another attempt of PR #63518 with two differences:

  • Don't quote docker env flags, since the quotes are treated as part of the variable. (My previous attempts at testing this were with pip3 commands and environment variables inside docker build phase, and not using --env-file.)
  • Add grpcio to the skip-binary list so that the armv7 build does not attempt to use an existing wheel, but instead always builds from source (this is why it succeeded). The idea here being that we want to ensure we use a wheel built specifically for alpine rather than a generic arm wheel.

Original PR description:
Add -lexecinfo to the list of GRPC LDFLAGS, which allows the home assistant wheel builder to create wheels for armv7 grpc builds.

This is needed because:

  • Home assistant installs libexecinfo-dev in the builder
  • abseil-cpp seems to conditionally pick this up src
  • musl does not include backtrace as part of the system library, however it can come from `-lexecinfo

This allows building a grpc 1.43.0 wheel on armv7 alpine 3.14 with python 3.9

GRPC currently does not have a way to add a single LDFLAG, so the existing LDFLAGS are copied until an option for that is added, which I can explore.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

Add link flags, as another attempt of PR home-assistant#63518 with two differences:
- Don't quote docker env flags, since the quotes are treated as part of the variable. (My previous attempts at testing this were with pip3 commands and environment variables inside docker build phase, and not using --env-file.)
- Add grpcio to the skip-binary list so that the armv7 build does not attempt to use an existing wheel, but instead always builds from source (this is why it succeeded). The idea here being that we want to ensure we use a wheel built specifically for alpine rather than a generic arm wheel.
@probot-home-assistant probot-home-assistant bot added the small-pr PRs with less than 30 lines. label Jan 5, 2022
allenporter added a commit to allenporter/home-assistant-core that referenced this pull request Jan 5, 2022
Try again PR home-assistant#63493 to kick off grpcio wheel build.

This should not be merged until home-assistant#63521 which sets flags to build grpcio from source with all the required link flags.
@allenporter
Copy link
Contributor Author

allenporter commented Jan 6, 2022

Here is some additional context for how I tested the two changes manually. Instead of using the stripped down builder, I used the environment that invokes the wheel builder. It's not exactly the same as what runs on CI, but much closer. There are a set of commands in https://github.com/allenporter/home-assistant-wheels/tree/issue-grpc/docker that basically set environment variables and attempt to create docker containers than run the wheel builder.

Here are some more details on how I manually tested the two additional changes. I ran these on both amd64 and armv7 and they both had the same result.

For bug 1

I reproduced the failure we saw last attempt with the quotes in the --env-file:

$ cat docker/env_file
GRPC_BUILD_WITH_BORING_SSL_ASM=false
GRPC_PYTHON_BUILD_SYSTEM_OPENSSL=true
GRPC_PYTHON_BUILD_WITH_CYTHON=true
GRPC_PYTHON_DISABLE_LIBC_COMPATIBILITY=true
GRPC_PYTHON_LDFLAGS="-lpthread -Wl,-wrap,memcpy -static-libgcc -lexecinfo"
$ PYTHON_VERSION=3.9 ALPINE_VERSION=3.14 GRPC_VERSION=1.43.0 ARCH=amd64 docker/command create_builder
$ PYTHON_VERSION=3.9 ALPINE_VERSION=3.14 GRPC_VERSION=1.43.0 ARCH=amd64 docker/command build
...
2022-01-06T02:05:10,095   /usr/lib/gcc/x86_64-alpine-linux-musl/10.3.1/../../../../x86_64-alpine-linux-musl/bin/ld: cannot find -lpthread -Wl,-wrap,memcpy -static-libgcc -lexecinfo
...

Then, running that again without the quotes in the --env-file:

$ cat docker/env_file
GRPC_BUILD_WITH_BORING_SSL_ASM=false
GRPC_PYTHON_BUILD_SYSTEM_OPENSSL=true
GRPC_PYTHON_BUILD_WITH_CYTHON=true
GRPC_PYTHON_DISABLE_LIBC_COMPATIBILITY=true
GRPC_PYTHON_LDFLAGS=-lpthread -Wl,-wrap,memcpy -static-libgcc -lexecinfo
$ PYTHON_VERSION=3.9 ALPINE_VERSION=3.14 GRPC_VERSION=1.43.0 ARCH=amd64 docker/command create_builder
$ PYTHON_VERSION=3.9 ALPINE_VERSION=3.14 GRPC_VERSION=1.43.0 ARCH=amd64 docker/command build
...
Successfully built grpcio

So I have higher confidence this will work.

For bug 2

I previously added --skip-binary="grpcio" in my local wheel builder as I saw it attempting to reuse an existing wheel when manually testing, similar to what we saw on CI for armv7. I forgot to add this in the original PR to update the link flags. I also tested this again with --skip-binary="aiohttp,grpcio" to try to more closely mimic what CI will do.

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Alright, lets try again 👍

@frenck frenck merged commit 5edc17a into home-assistant:dev Jan 6, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed small-pr PRs with less than 30 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants