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 arm64 static build binary #239

Merged
merged 1 commit into from
Feb 17, 2021
Merged

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Feb 9, 2021

This adds an arm64 derivation to the static binary build.

Manual test

> nix-build nix/arm64.nix
> file result/bin/conmon
result/bin/conmon: ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), statically linked, for GNU/Linux 2.6.32, stripped

Test build: conmon.tar.gz
Refers to cri-o/cri-o#4552

@saschagrunert saschagrunert force-pushed the arm branch 4 times, most recently from 73bb36a to 238baea Compare February 16, 2021 12:33
@saschagrunert
Copy link
Member Author

The cache is 2131Mb large and the build took ~2 hours. I think if the cache only invalidates when updating the pinned nixpkgs then we're probably fine with that change, right?

PTAL @cevich

@saschagrunert saschagrunert changed the title WIP: Add arm64 static build binary Add arm64 static build binary Feb 16, 2021
@cevich
Copy link
Member

cevich commented Feb 16, 2021

cache only invalidates when updating the pinned nixpkgs then we're probably fine with that change, right?

I'm nearly always amazed by how deceptively complex caching can be. A 2-hour build puts us right up there against a hard-coded Cirrus-CI max job runtime (no setting can override it). Considering our experience on the podman repo, would it make sense to go straight to the cachix solution here as well?

Copy link
Member

@cevich cevich left a comment

Choose a reason for hiding this comment

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

Good first-draft, I made a few comments/questions for discussion. Bonous points for getting it working on your own!

.cirrus.yml Outdated Show resolved Hide resolved
.cirrus.yml Outdated Show resolved Hide resolved
.cirrus.yml Outdated Show resolved Hide resolved
.cirrus.yml Outdated Show resolved Hide resolved
.cirrus.yml Outdated Show resolved Hide resolved
@saschagrunert saschagrunert force-pushed the arm branch 2 times, most recently from 74a678a to 0faa223 Compare February 17, 2021 09:07
@saschagrunert
Copy link
Member Author

Considering our experience on the podman repo, would it make sense to go straight to the cachix solution here as well?

Yeah I think using cachix is a good move forward here. I'll pre-populate the conmon cache and come back to the CI if that is done. 👍

@saschagrunert saschagrunert force-pushed the arm branch 5 times, most recently from fd8f430 to 70b6811 Compare February 17, 2021 12:32
@saschagrunert
Copy link
Member Author

saschagrunert commented Feb 17, 2021

The job is down to 3 minutes with the cache, and the binary artifacts looks fine, too:

> file conmon
conmon: ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), statically linked, for GNU/Linux 2.6.32, stripped
> qemu-aarch64 ./conmon --version
conmon version 2.0.27-dev
commit: 70b68113db9745369071c9d1e4f876cb31259abb-dirty

@cevich
Copy link
Member

cevich commented Feb 17, 2021

Great! Thanks for all your work on this. Do you want to e-mail me so I can create an encrypted $CACHIX_AUTH_TOKEN?

(apologies if you already did and I didn't see it yet for some reason)

@saschagrunert
Copy link
Member Author

(apologies if you already did and I didn't see it yet for some reason)

Yes, I already did it :)

@cevich
Copy link
Member

cevich commented Feb 17, 2021

Well go figure, now that I go looking for it...there it is! 🤣

Use ENCRYPTED[4c3b8d82b0333abf048c56a71f2559ddb1c9ed38f0c28916eca13f79affa5904cf90c76a5bd8686680c89f41079ef341]

@saschagrunert saschagrunert force-pushed the arm branch 2 times, most recently from 784419d to d29b965 Compare February 17, 2021 14:34
@cevich
Copy link
Member

cevich commented Feb 17, 2021

It looks to me like there's a lot of duplication between static_binary_task and static_binary_arm64_task. Just by eyeball, is the only difference default.nix vs default-arm64.nix?

Assuming so, we can easily simplify this to one task, using an env. var. and a matrix...

@saschagrunert
Copy link
Member Author

It looks to me like there's a lot of duplication between static_binary_task and static_binary_arm64_task. Just by eyeball, is the only difference default.nix vs default-arm64.nix?

Assuming so, we can easily simplify this to one task, using an env. var. and a matrix...

Alright changed to a matrix, let's see how that works.

@cevich
Copy link
Member

cevich commented Feb 17, 2021

Lol, I was just about to post a proposed diff but you posted almost the exact same change 😄

I think the only secret-sauce (it's not obvious) in the task is alias: static_binary (that will cause the name: value to work properly)

This adds an arm64 derivation to the static binary build.

Signed-off-by: Sascha Grunert <[email protected]>
@saschagrunert
Copy link
Member Author

saschagrunert commented Feb 17, 2021

Lol, I was just about to post a proposed diff but you posted almost the exact same change

I think the only secret-sauce (it's not obvious) in the task is alias: static_binary (that will cause the name: value to work properly)

Ah thank you for the hint, are those names fine now? Both binaries are looking good, the cache works as intended.

@TomSweeneyRedHat
Copy link
Member

I'm not at all an expert in this arena, but the changes LGTM.
@cevich and @saschagrunert thank you both for your great work here!

@cevich
Copy link
Member

cevich commented Feb 17, 2021

Ah thank you for the hint, are those names fine now?

Hmmm, something is still wrong...oh, you removed the 'name' attr. from matrix, that's why. No matter, I think it's probably fine as-is...it's a small detail not worth fretting over. Important thing is you got it all working 😀

/lgtm

@cevich
Copy link
Member

cevich commented Feb 17, 2021

Woops...no bot here.

@cevich cevich merged commit 7310bf1 into containers:master Feb 17, 2021
@saschagrunert saschagrunert deleted the arm branch February 17, 2021 15:17
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.

3 participants