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

(feat) add support for musl using cross-rs #2536

Merged
merged 7 commits into from
Dec 21, 2023
Merged

Conversation

jprendes
Copy link
Contributor

@jprendes jprendes commented Nov 18, 2023

This PR adds support for building and testing against musl targets.

A summary of changes:

  • Add musl targets to Cross.toml and the corresponding Dockerfile.musl
  • Install dependencies in Dockerfile.gnu so that we can run tests using cross.
  • Add a cargo.sh script that wraps cargo calls, using either cargo or cross based on the target.
  • Fix mock.rs so that it builds with musl targets
  • Use the cargo.sh script in the features_test.sh and justfile
  • Remove now redundant musl_test.sh.
  • Update CI to install cross, use cargo.sh, and force using cross for all targets (including host target).
  • Update CI to relese musl binaries as well as gnu binaries.

Example of a release generated from this PR: https://github.com/jprendes/youki/releases/tag/v0.4.1

Note: This PR builds on top of #2541. I will rebase once that is merged.

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2023

Codecov Report

Merging #2536 (06d8e9f) into main (e3cafbd) will increase coverage by 0.02%.
Report is 5 commits behind head on main.
The diff coverage is 77.77%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2536      +/-   ##
==========================================
+ Coverage   65.86%   65.88%   +0.02%     
==========================================
  Files         133      133              
  Lines       16795    16819      +24     
==========================================
+ Hits        11062    11082      +20     
- Misses       5733     5737       +4     

@jprendes jprendes force-pushed the feat-musl branch 14 times, most recently from 918640b to 01a84bb Compare November 22, 2023 09:45
@jprendes jprendes marked this pull request as ready for review November 22, 2023 09:46
@utam0k
Copy link
Member

utam0k commented Nov 22, 2023

Sorry, I won't be able to find the time this week, so I'll look at it in a few days 🙏

@jprendes jprendes self-assigned this Nov 22, 2023
scripts/cargo.sh Outdated Show resolved Hide resolved
scripts/cargo.sh Outdated Show resolved Hide resolved
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Co-Authored-By: adrianalin <pop.adrian61@gmail.com>
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
@jprendes
Copy link
Contributor Author

Reabsed.
@utam0k @adrianalin @cuisongliu PTAL

An example release generated by this PR is:
https://github.com/jprendes/youki/releases/tag/v0.3.2

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Dec 20, 2023

Self-reminder : need to update merge CI requirements to correct CI names after this is merged.
(The three pending CI seems to be because of renaming of the workflows, so should be ok.)

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

May I ask a favor of the documentation for developers in another PR?
https://containers.github.io/youki/developer/introduction.html

@utam0k
Copy link
Member

utam0k commented Dec 20, 2023

Thanks!! @jprendes

@utam0k
Copy link
Member

utam0k commented Dec 20, 2023

Self-reminder : need to update merge CI requirements to correct CI names after this is merged. (The three pending CI seems to be because of renaming of the workflows, so should be ok.)

Done ✅

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Dec 21, 2023

Thanks @utam0k 💜
Is this ready to merge? The CI checks which are updated in this are keeping other PRs from passing...
I can revert the CI check requirements temp until this is merged, or as we don't have anything urgent, just wait out for this.
Let me know.

Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I have some nitpicks / questions, but nothing that would block merging, so as @utam0k has already approved, I'll wait sometime and merge this.

@jprendes thanks a lot for this, this is really amazing work! Take your time for replying to the comments 👍

.github/workflows/main.yml Show resolved Hide resolved
Cross.toml Outdated Show resolved Hide resolved
scripts/build.sh Outdated Show resolved Hide resolved
@utam0k
Copy link
Member

utam0k commented Dec 21, 2023

Ops sorry I'll merge it

@utam0k utam0k enabled auto-merge (squash) December 21, 2023 11:49
@utam0k utam0k merged commit ff79c54 into youki-dev:main Dec 21, 2023
26 checks passed
@github-actions github-actions bot mentioned this pull request Dec 21, 2023
@adrianalin
Copy link
Contributor

adrianalin commented Dec 27, 2023

Sorry, I could not find where documentation was updated (related to musl build), @jprendes please provide link. Thank you!

@jprendes
Copy link
Contributor Author

Sorry, I could not find where documentation was updated (related to musl build)

I still need to create a PR. I'll do that as soon as I have some bandwidth again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants