-
Notifications
You must be signed in to change notification settings - Fork 149
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
Build darwin/aarch64 and universal binary #203
Conversation
* docs small changes * add arm64 to darwin architectures * docs/comments wee changes * stop using deprecated funcations * it works * now it works * add darwin-binary-arm64 to packageArchMap * add darwin/arm64 to ci packaging * use aarch instead of arm64 for package names
This pull request does not have a backport label. Could you fix it @AndersonQ? 🙏
NOTE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think this looks good.
dev-tools/mage/common.go
Outdated
// ParallelCtx should be used instead and a refactor is need to remove calls | ||
// Parallel? If so, context.TODO() is more appropriated as it makes explicit | ||
// a context should came from the caller and the current context is just a | ||
// temporary solution until the needed refactor is made. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are correct, should be refactored to always take a context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change LGTM, thanks for improving the comments too!
I am currently testing locally
Using this PR and the beats PR and the following command.
EXTERNAL=false PLATFORMS="darwin/arm64" PACKAGES="tar.gz" mage package
@AndersonQ Are we missing an update for osquerybeat in the beats repository PR? Running the above command yield the following error or maybe there is something missing in the osquerybeat build @aleksmaus ?
|
This pull request is now in conflicts. Could you fix it? 🙏
|
I'll take a look, the osquerybeat fetched the osquery distro during the build, looks like needs adjustment for darwin/arm64. |
oh, yes, it'll try to build the beats assuming the beats repo is on |
@AndersonQ
leads to this error:
what is the expectation of the beat repo? anybody is working on adjusting it to run for darwin/arm64? |
I'm on it. It seems the removal of the elastic-agent code from the beats repo broke the M1 PR. |
@aleksmaus @ph Set up the beats repo:
I updated the PR description with these instructions |
Thank you! Will have PR for osquerybeat shortly to support darwin/arm64. |
Opened PR against branch 26683-agent-arm64 |
/test |
1 similar comment
/test |
This pull request is now in conflicts. Could you fix it? 🙏
|
@@ -573,7 +594,7 @@ func ParallelCtx(ctx context.Context, fns ...interface{}) { | |||
// Parallel runs the given functions in parallel with an upper limit set based | |||
// on GOMAXPROCS. | |||
func Parallel(fns ...interface{}) { | |||
ParallelCtx(context.Background(), fns...) | |||
ParallelCtx(context.TODO(), fns...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I've tested the build, I haven't tested on m1.
@ph @AndersonQ it's still a problem with building on M1
I already mentioned before that the docker image tagged with arm64 is actually amd64 arch and qemu can't run that |
hi @aleksmaus, yes, we know that. So far we don't have a fix for that :/ |
@AndersonQ , Thank you! |
@amitkanfer I'm on PTO this whole week, perhaps @jlind23 could help you |
What does this PR do?
It enables the Elastic-Agent and Beats to be built for
darwin/arm64
and generates a universal binary (darwin/universal
).The universal binary is the merge of the
darwin/arm64
withdarwin/amd64
. Therefore it requires to first build thedarwin/arm64
anddarwin/amd64
and them "merge" them together into thedarwin/universal
. Alsodarwin/universal
isn't a valid platform or OS/ARCH pair, therefore it isn't recognised by the Go compiler. Given that, the choice was to adddarwin/arm64
as a new platform for the Elastic-Agent and when ever both platforms (darwin/arm64
anddarwin/amd64
) are built, also generate and package the universal binary.Even though it's
quitean implicit behaviour, the alternative would also require a lot of changes and exceptions to have our build process recognisingdarwin/universal
, but compilingdarwin/arm64
anddarwin/amd64
and merging them. Also the current process makes available all three of them, allowing the user to choose which fits better their needs. The drawback of the universal binary is its size, the double of the single binaries.A new target,
BuildDarwinUniversal
was added on thex-pack
smagefile
s required by the Elastic-Agent.Also fixes some small typos, redundant information on the help command and remove the use of some deprecated functions.
Why is it important?
We need to support Mac with M1 chips
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
Set up the beats repo:
elastic-agent
repo26683-agent-arm64
while [Beats] Build darwin/arm64 and universal binary beats#29585 isn't mergedBuild the elastic-agent for the new platforms:
PLATFORMS="darwin/arm64" PACKAGES="tar.gz" mage package
PLATFORMS="darwin/arm64,darwin/amd64" PACKAGES="tar.gz" mage package
Run the elastic-agent
####Watch out:
The elastic-agent tries to download any binary it might need to comply with the assigned policy. However none of the new binaries from this PR are present on the artifact API and other applications (e.g. fleet-server and elastic-endpoint) also aren't publishing
darwin/arm64
versions. Therefore if the elastic-agent tries to download them, it'll fail. This is the expected behavior and this PR does not address it, see Out of Scope.Out of scope:
darwin-universal
artifacts on the artifact APIdarwin/amd64
agent running under Rosetta2 on a Mac with M1 chip. This will be addressed on another PR.Related issues