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

Make CI cross-compile lf for every supported platform; support darwin-arm64 #1298

Merged
merged 3 commits into from
Jul 3, 2023

Conversation

ilyagr
Copy link
Collaborator

@ilyagr ilyagr commented Jun 10, 2023

This reworks the gen/xbuild.sh script to exit with a failure if any of the targets don't compile. It also adds a CI job to try cross-compiling for every platform on every pull request.

The new check is a little slow and compute-expensive, but I am hoping it's well within the quota.

See #1299 for a demo of how the CI would have failed after #1288 and before #1295.

Example of a specific CI failure for #1299:

screenshot


Also added support for darwin-arm64, as that has at least one user and it seems to work fine.

@ilyagr ilyagr marked this pull request as ready for review June 10, 2023 23:46
@ilyagr ilyagr force-pushed the xbuild branch 5 times, most recently from 4b6e624 to 580e433 Compare June 11, 2023 00:02
@ilyagr ilyagr changed the title Make CI cross-compile on all supported platforms Make CI cross-compile lf for every supported platform Jun 11, 2023
Copy link
Collaborator

@joelim-work joelim-work 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 working on this, apart from #1288, this would have also caught the df errors introduced in #1168 that broke the build for a few platforms when releasing r29.

BTW I happened to be playing around with this myself, you can have a look at my version and see if you want to salvage from there.

gen/xbuild.sh Outdated Show resolved Hide resolved
gen/xbuild.sh Outdated Show resolved Hide resolved
gen/xbuild.sh Show resolved Hide resolved
gen/xbuild.sh Outdated Show resolved Hide resolved
gen/xbuild.sh Outdated Show resolved Hide resolved
gen/xbuild.sh Outdated Show resolved Hide resolved
@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 11, 2023

Thank you for looking this over! It's past my bedtime; I'll look at your version and your other comments at some later point.

🌃💤

Update: I did glance at your version. I like the function you call repeatedly much better than my for loop.

@joelim-work
Copy link
Collaborator

BTW I forgot to mention but you may also want to run this through shellcheck. I usually use it to see if there's any variables I forgot to quote, or other mistakes that are easy to make when writing shell scripts.

@ilyagr ilyagr force-pushed the xbuild branch 4 times, most recently from c693e24 to 2f9610b Compare June 11, 2023 22:20
@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 11, 2023

OK, I updated this and borrowed quite a few of your ideas. Thank you very much!

I am also a fan of shellcheck. It demanded a bit too much quoting for my taste, but it's probably still a good idea to make scripts shellcheck-clean.

@ilyagr ilyagr force-pushed the xbuild branch 5 times, most recently from fae2cfc to 69f77ed Compare June 11, 2023 22:41
@ilyagr ilyagr changed the title Make CI cross-compile lf for every supported platform Make CI cross-compile lf for every supported platform; support darwin-arm64 Jun 11, 2023
@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 11, 2023

I added one more commit making the script a little more complex but decreasing repetition. Let me know if I should get rid of it.

…ossible

This reworks the gen/xbuild.sh script to exit with a failure if any of the targets don't compile. It also adds a CI job to try cross-compiling for every platform on every pull request.

The new check is a little slow and compute-expensive, but I am hoping it's well within the quota.

See gokcehan#1299 for a demo of how the CI would have failed after gokcehan#1288 and before gokcehan#1295.

Screenshot: https://github.com/gokcehan/lf/assets/4123047/3d7a6b7c-8642-4d4c-a98a-c8b886c0b184

Thanks to @joelim-work for numerous improvements and an alternate
implementation I heavily borrowed from
(https://github.com/joelim-work/lf/blob/ci/gen/xbuild.sh)
@ilyagr ilyagr force-pushed the xbuild branch 4 times, most recently from e8787c4 to 49b9aad Compare June 12, 2023 03:46
gen/xbuild.sh Outdated Show resolved Hide resolved
gen/xbuild.sh Outdated Show resolved Hide resolved
I can remove this commit if people prefer.
Arm Macs are becoming popular. We have at least one
user: gokcehan#1294. It
seems lf works fine there.
Copy link
Collaborator

@joelim-work joelim-work left a comment

Choose a reason for hiding this comment

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

I think the changes are OK, but it's probably best to have @gokcehan look at this as a secondary reviewer. Shell scripts are notorious for being error prone and subject to stylistic preferences.

Thanks once again for the changes!

@gokcehan
Copy link
Owner

gokcehan commented Jul 3, 2023

Thanks @ilyagr for the patch and @joelim-work for the review. gen/xbuild.sh looks a lot cleaner now.

I wasn't sure about the additional computation that comes with cross compiling but I guess this is getting necessary these days. I found the quotes for github actions here and it says:

GitHub Actions usage is free for standard GitHub-hosted runners in public repositories, and for self-hosted runners. For private repositories, each GitHub account receives a certain amount of free minutes and storage for use with GitHub-hosted runners, depending on the product used with the account. Any usage beyond the included amounts is controlled by spending limits.

So hopefully it wouldn't be a problem since this is a public repo. The only disadvantage might be the additional delay for each change but I think that is acceptable.

@gokcehan gokcehan merged commit c11931d into gokcehan:master Jul 3, 2023
@ilyagr ilyagr deleted the xbuild branch July 5, 2023 20:23
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