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.

Sign up for GitHub

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

Jump to bottom

[ci] Cross build for architectures #5142

Merged
merged 3 commits into from
Apr 5, 2022
Merged

[ci] Cross build for architectures #5142

merged 3 commits into from
Apr 5, 2022

Conversation

hickeyma
Copy link
Contributor

@hickeyma hickeyma commented Apr 4, 2022 •
edited
Loading

Add support to CI to build collector for architectures: Linux 386 and ARM64, and Windows 386. The binaries for these architectures are released in collector releases. This GitHub Actions job will help to see if any issues are introduced to cross binary builds when a commit is pushed.

Fix #5031

Signed-off-by: Martin Hickey martin.hickey@ie.ibm.com

@hickeyma hickeyma requested review from a team and bogdandrutu April 4, 2022 10:09
@hickeyma hickeyma marked this pull request as draft April 4, 2022 10:14
@hickeyma hickeyma force-pushed the ci/add-build-os-archs branch from eaba443 to aa15164 Compare April 4, 2022 10:48
@hickeyma hickeyma mentioned this pull request Apr 4, 2022
Closed
@hickeyma hickeyma force-pushed the ci/add-build-os-archs branch from aa15164 to f8d9624 Compare April 4, 2022 14:57
@codecov
Copy link

codecov bot commented Apr 4, 2022 •
edited
Loading

Codecov Report

Merging #5142 (3b25547) into main (ff6a8f6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5142   +/-   ##
=======================================
  Coverage   90.34%   90.34%           
=======================================
  Files         182      182           
  Lines       11031    11031           
=======================================
  Hits         9966     9966           
  Misses        840      840           
  Partials      225      225

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff6a8f6...3b25547. Read the comment docs.

@hickeyma hickeyma force-pushed the ci/add-build-os-archs branch 2 times, most recently from 95cdb88 to 557531e Compare April 4, 2022 15:26
@hickeyma hickeyma changed the title [WIP][ci] Add supported os architectures [ci] Cross build for architectures Apr 4, 2022
@hickeyma hickeyma marked this pull request as ready for review April 4, 2022 15:47
@hickeyma
Copy link
Contributor Author

hickeyma commented Apr 4, 2022

Change log not required.

@codeboten codeboten added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Apr 4, 2022
codeboten
codeboten reviewed Apr 4, 2022
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on, any thoughts on how this could be kept in sync w/ the releases repo? @jpkrohling any ideas?

mx-psi
mx-psi reviewed Apr 5, 2022
View reviewed changes
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @hickeyma! I left a suggestion to make this more aligned with the 'releases' repo, otherwise it looks good to me.

any thoughts on how this could be kept in sync w/ the releases repo?

My 2 cents is that the os-arch combinations we release will be updated very infrequently and thus it would probably be fine to just keep these in sync manually.

.github/workflows/build-and-test.yml
strategy:
fail-fast: false
matrix:
include:
Copy link
Member

Choose a reason for hiding this comment

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

What about having the same matrix as in opentelemetry-collector-releases? See here. This would help us keep them in sync.

Granted, this will duplicate some testing (if you go test on linux+amd64 you are already building for that), but I think that's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I have updated the matrix to match the opentelemetry-collector-releases matrix. There is one difference and that is the Darwin 386 build is no longer supported by Go. This is because it dropped support for 32-bit binaries on macOS in Go 1.15.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for catching that, created open-telemetry/opentelemetry-collector-releases/issues/97 to track this

@hickeyma
[ci] Do cross architecture build of collector
0af2172 
This job will help to see if any issues introduced to cross binary builds,
when a commit is pushed.

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@hickeyma hickeyma force-pushed the ci/add-build-os-archs branch from 557531e to 0af2172 Compare April 5, 2022 11:33
hickeyma added 2 commits April 5, 2022 12:48
@hickeyma
Sync cross build list with opentelemetry-collector-releases
aee37da 
Update following review comment:
open-telemetry#5142 (comment)

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@hickeyma
Disable darwin 386 build
3b25547 
Go 1.15 dropped support for 32-bit binaries on macOS:
https://go.dev/doc/go1.15

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
@mx-psi mx-psi mentioned this pull request Apr 5, 2022
Closed
codeboten
codeboten approved these changes Apr 5, 2022
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

👍

@codeboten codeboten merged commit 9ca2c42 into open-telemetry:main Apr 5, 2022
@hickeyma hickeyma deleted the ci/add-build-os-archs branch April 5, 2022 15:30
@hickeyma
Copy link
Contributor Author

hickeyma commented Apr 5, 2022

Thanks for the reviews folks.

@hickeyma hickeyma mentioned this pull request Apr 8, 2022
Closed
6 tasks
@mx-psi mx-psi mentioned this pull request May 2, 2022
Open
Nicholaswang pushed a commit to Nicholaswang/opentelemetry-collector that referenced this pull request Jun 7, 2022
@hickeyma @Nicholaswang
[ci] Cross build for architectures (open-telemetry#5142)
b310106 
* [ci] Do cross architecture build of collector

This job will help to see if any issues introduced to cross binary builds,
when a commit is pushed.

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>

* Sync cross build list with opentelemetry-collector-releases

Update following review comment:
open-telemetry#5142 (comment)

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>

* Disable darwin 386 build

Go 1.15 dropped support for 32-bit binaries on macOS:
https://go.dev/doc/go1.15

Signed-off-by: Martin Hickey <martin.hickey@ie.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeboten codeboten codeboten approved these changes

@mx-psi mx-psi mx-psi approved these changes

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu

Assignees
No one assigned
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CI] Build on 386 architecture
3 participants
@hickeyma @codeboten @mx-psi