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

Refactor builds #536

Merged
merged 44 commits into from
Nov 14, 2022
Merged

Conversation

austinlparker
Copy link
Member

This fixes the problems with multi-arch builds -- the context for each build is not consistent.

@austinlparker austinlparker requested a review from a team as a code owner October 23, 2022 22:09
@austinlparker
Copy link
Member Author

Ok, good news/bad news.

Good news, this is actually building things!
Bad news, it's slow as heck because of qemu emulation.

I believe we could fix this by using https://www.docker.com/blog/faster-multi-platform-builds-dockerfile-cross-compilation-guide/ to build amd64 base images and arm64 binaries, but I'm not entirely sure if that'll help for stuff like PHP. I'd intuit that it'd help in C++, but I'm not sure if it'd work to build amd64 grpc and otel then link to an arm64 binary.

Would love if @brettmc and/or @DebajitDas could chime in with ideas as well.

@cartersocha
Copy link
Contributor

Cc @lalitb on the above question too

@lalitb
Copy link
Member

lalitb commented Oct 24, 2022

but I'm not sure if it'd work to build amd64 grpc and otel then link to an arm64 binary.

That won't work for C++. Tagging @esigo as he has done some work on qemu/ARM CI builds earlier.

@esigo
Copy link
Member

esigo commented Oct 24, 2022

I know only two ways to address the slow build issue.

  1. The dependencies being stored in some image and hosted in some registry (not possible for us I think).
  2. Saving the dependencies image in GitHub actions artifact and loading the artifact before builds similar to what I was trying to do here.

I can work on the second option after C++ metric GA.

@austinlparker austinlparker added in-progress This issue is actively being worked on and removed v1-frozen labels Oct 31, 2022
@austinlparker
Copy link
Member Author

Please see #458 (comment) for the current state of work here.

@austinlparker austinlparker changed the title Support multi-arch builds Refactor builds Nov 13, 2022
@austinlparker
Copy link
Member Author

Ok, I'm willing to call this good. I've renamed the PR to reflect what it currently does (which is just refactor the build rather than support multi-arch) but it does lay the foundation for multi-arch in the future. As a benefit, this significantly decreases build time due to parallelizing each service. Since it can do that, and run on free instances, I'm inclined to keep the per-PR build check in there to ensure that we aren't getting compilation errors.

Copy link
Contributor

@cartersocha cartersocha left a comment

Choose a reason for hiding this comment

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

lgtm

@austinlparker
Copy link
Member Author

Doing a bit more work to try and speed up CPP by installing https://github.com/rui314/mold to speed up linking.

Pre-mold: gRPC build 1043s
Post-mold: gRPC build 813s

Pre-mold: OpenTelemetry build 172s
Post-mold: OpenTelemetry build 133s

... Unfortunately, building mold costs 520s, thus obliviating the speedup 🙃

However, I suspect greater returns on end-user hardware so it's probably worth keeping?

@mviitane
Copy link
Member

Unfortunately, I don’t see improvements in local builds:

Ryzen 5600G:

docker compose build currencyservice --no-cache | Original 400.9s | This PR 505.5s
docker compose build featureflagservice --no-cache | Original 73.8s | This PR 85.0s
docker compose build shippingservice --no-cache | Original 182.9s | This PR 177.6s

Mac M1 Pro:

docker compose build currencyservice --no-cache   | Original 472.4s | This PR 639.5s
docker compose build featureflagservice --no-cache | Original 94.1s | This PR 102.9s
docker compose build shippingservice --no-cache   | Original 254.9 | This PR 248.0s

currencyservice and featureflagservice build times increased. The mold linker gave some linking improvements on x86, but the mold install takes way too long to be beneficial overall.

@mviitane
Copy link
Member

I think the GitHub workflow changes should be delivered separately.

@austinlparker austinlparker merged commit fd515c2 into open-telemetry:main Nov 14, 2022
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
* switch to matrix build strategy

Co-authored-by: Carter Socha <43380952+cartersocha@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-progress This issue is actively being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants