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

Add Musl support #23

Closed
wants to merge 4 commits into from
Closed

Add Musl support #23

wants to merge 4 commits into from

Conversation

razvand
Copy link
Contributor

@razvand razvand commented Oct 29, 2022

Make changes to build and source files for Musl integration.

@razvand razvand added the enhancement New feature or request label Nov 2, 2022
@razvand razvand requested review from StefanJum and kubanrob and removed request for kubanrob November 2, 2022 06:32
@razvand razvand force-pushed the musl-support branch 3 times, most recently from 6a8581d to 74e184c Compare November 6, 2022 17:22
@razvand razvand mentioned this pull request Nov 7, 2022
Closed
4 tasks
craciunoiuc
craciunoiuc reviewed Nov 9, 2022
View reviewed changes
Copy link
Member

@craciunoiuc craciunoiuc left a comment

Choose a reason for hiding this comment

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

I did an initial pass over it. I suppose the PR will include only the last 3 commits.

The changes are fine, I feel like they need a bit more comments.

patches/0007-musl-support.patch Outdated Show resolved Hide resolved
patches/0007-musl-support.patch Outdated Show resolved Hide resolved
uknetdev.c Outdated Show resolved Hide resolved
@razvand razvand force-pushed the musl-support branch 3 times, most recently from e48643f to 0eaf1eb Compare November 20, 2022 15:29
@razvand razvand assigned mschlumpp and unassigned dragosargint Nov 20, 2022
@razvand razvand marked this pull request as ready for review November 20, 2022 15:35
@razvand razvand requested review from craciunoiuc and removed request for StefanJum November 20, 2022 15:35
@razvand
Copy link
Contributor Author

razvand commented Nov 20, 2022

@craciunoiuc , I didn't make updates to the original files. I just created patches for them. I would leave them as they are and, if required, I would create issues to fix them in the future.

@craciunoiuc
Copy link
Member

craciunoiuc commented Nov 20, 2022 •
edited
Loading

Yup, it's fine, I will take another look over it tomorrow

In the meantime: Make sure to add the unikraft.io address to Github, so Github identifies the commits as yours

mschlumpp
mschlumpp requested changes Nov 21, 2022
View reviewed changes
Copy link
Member

@mschlumpp mschlumpp left a comment

Choose a reason for hiding this comment

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

I wasn't able to compile lib-lwip with these changes and musl. I'm also not sure which PRs would be necessary (I tried unikraft/unikraft#644, unikraft/lib-musl#19, and this PR).
Therefore, I just looked over the changes.

patches/0010-src-api-Make-sockets.c-compatible-with-Musl.patch Outdated Show resolved Hide resolved
@razvand
Copy link
Contributor Author

razvand commented Nov 22, 2022

@mschlumpp , I'll be back with instructions on building LWIP + Musl.

@razvand
Copy link
Contributor Author

razvand commented Nov 22, 2022

Yup, it's fine, I will take another look over it tomorrow

In the meantime: Make sure to add the unikraft.io address to Github, so Github identifies the commits as yours

@craciunoiuc , done.

@mschlumpp
Copy link
Member

Okay it seems to work fine. Other than the comment I already left, LGTM.

(I guess I have to approve it after Cezar reviewed it and everything is fixed, right?)

@craciunoiuc
Copy link
Member

I want to do another pass now and also test it properly, then I guess we can approve.

craciunoiuc
craciunoiuc reviewed Nov 24, 2022
View reviewed changes
Copy link
Member

@craciunoiuc craciunoiuc left a comment

Choose a reason for hiding this comment

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

Some Oxford comma for you @razvand

patches/0008-src-include-Make-lwip-inet.h-compatible-with-Musl.patch Outdated Show resolved Hide resolved
patches/0009-src-api-Make-netdb.c-compatible-with-Musl.patch Outdated Show resolved Hide resolved
patches/0010-src-api-Make-sockets.c-compatible-with-Musl.patch Outdated Show resolved Hide resolved
patches/0010-src-api-Make-sockets.c-compatible-with-Musl.patch Outdated Show resolved Hide resolved
@razvand
sockets.c: Remove functions defined in libc
379e2b6 
Functions `if_nametoindex` and `if_indextoname` are to be defined by the
standard C library (Musl, nolibc).

Signed-off-by: Razvan Deaconescu <razvand@unikraft.io>
@razvand
Remove source code files defined by libc
fae2a53 
Remove source code files that are to be defined by the standard C
library (Musl or nolibc).

Signed-off-by: Razvan Deaconescu <razvand@unikraft.io>
@razvand
Remove commented out source code files
3a678a2 
Lines were commented out in `Makefile.uk`. Remove them from
`Makefile.uk` and also fully remove the source code files from the tree.

Signed-off-by: Razvan Deaconescu <razvand@unikraft.io>
@razvand
patches: Add patches for Musl compatibility
16ff033 
Add patch files that remove or update functions, macros and variables in
LWIP original source code to make it compatible to Musl.

Signed-off-by: Razvan Deaconescu <razvand@unikraft.io>
craciunoiuc
craciunoiuc approved these changes Nov 25, 2022
View reviewed changes
Copy link
Member

@craciunoiuc craciunoiuc left a comment

Choose a reason for hiding this comment

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

All good here. Thanks!

Reviewed-by: Cezar Craciunoiu cezar.craciunoiu@unikraft.io

mschlumpp
mschlumpp approved these changes Nov 25, 2022
View reviewed changes
Copy link
Member

@mschlumpp mschlumpp left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks.
Approved-by: Marco Schlumpp marco@unikraft.io

unikraft-bot pushed a commit that referenced this pull request Nov 25, 2022
@razvand @unikraft-bot
Remove source code files defined by libc
30591cd 
Remove source code files that are to be defined by the standard C
library (Musl or nolibc).

Signed-off-by: Razvan Deaconescu <razvand@unikraft.io>
Reviewed-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.io>
Approved-by: Marco Schlumpp <marco@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #23
unikraft-bot pushed a commit that referenced this pull request Nov 25, 2022
@razvand @unikraft-bot
Remove commented out source code files
ac95c7f 
Lines were commented out in `Makefile.uk`. Remove them from
`Makefile.uk` and also fully remove the source code files from the tree.

Signed-off-by: Razvan Deaconescu <razvand@unikraft.io>
Reviewed-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.io>
Approved-by: Marco Schlumpp <marco@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #23
unikraft-bot pushed a commit that referenced this pull request Nov 25, 2022
@razvand @unikraft-bot
patches: Add patches for Musl compatibility
560e66e 
Add patch files that remove or update functions, macros and variables in
LWIP original source code to make it compatible to Musl.

Signed-off-by: Razvan Deaconescu <razvand@unikraft.io>
Reviewed-by: Cezar Craciunoiu <cezar.craciunoiu@unikraft.io>
Approved-by: Marco Schlumpp <marco@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@craciunoiuc craciunoiuc craciunoiuc approved these changes

@mschlumpp mschlumpp mschlumpp approved these changes

Assignees

@mschlumpp mschlumpp

Labels
ci/merged enhancement New feature or request
Projects
Musl
Status: Done
Unikraft Core Roadmap
Status: ✅ Done!
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@razvand @craciunoiuc @mschlumpp @dragosargint @unikraft-bot