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

Makefile.uk.musl.exit: Patch for building on case insensitive file systems #70

Closed
wants to merge 1 commit into from

Conversation

Starnox
Copy link
Contributor

@Starnox Starnox commented Aug 5, 2023

Add exit variant to __Exit.c in order to avoid symbol conflicts on case insensitive file systems

@razvand razvand requested a review from StefanJum August 9, 2023 10:01
@razvand razvand self-assigned this Aug 9, 2023
@razvand razvand added the enhancement New feature or request label Aug 9, 2023
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

I can't test this on MacOS, but it works fine on linux. @Starnox Please update the commit message to state why the name conflict appears on MacOS and not on Linux.

Copy link
Member

@skuenzer skuenzer left a comment

Choose a reason for hiding this comment

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

Hi,
the PR looks good. Just a minor comment: In the commit message can you replace "Darwin does not support case sensitive"? It actually supports it but the support depends on how a volume got formatted. Case-insensitive is just the default setting. Maybe rephrase slightly your commit and title that the is done to support case-insensitive filesystems. It could probably even happen on Linux environments if the underlying file system is restricted. No need to mention Darwin.

Another minor thing: Can you rename the _Exit.c file to _exit.c (git mv _Exit.c _exit.c and adopt Makefile.uk accordingly: _exit.c|unikraft). This removes any leftovers of case sensitivity. Our build system is anyways also not good in handling case sensitive filenames.

Thanks,
Simon

@skuenzer
Copy link
Member

Hi, the PR looks good. Just a minor comment: In the commit message can you replace "Darwin does not support case sensitive"? It actually supports it but the support depends on how a volume got formatted. Case-insensitive is just the default setting. Maybe rephrase slightly your commit and title that the is done to support case-insensitive filesystems. It could probably even happen on Linux environments if the underlying file system is restricted. No need to mention Darwin.

Another minor thing: Can you rename the _Exit.c file to _exit.c (git mv _Exit.c _exit.c and adopt Makefile.uk accordingly: _exit.c|unikraft). This removes any leftovers of case sensitivity. Our build system is anyways also not good in handling case sensitive filenames.

Thanks, Simon

Ups, sorry, the conflict actually occurs because:

Makefile.uk.musl.unistd
22:LIBMUSL_UNISTD_SRCS-y += $(LIBMUSL)/src/unistd/_exit.c

Makefile.uk.musl.exit
20:LIBMUSL_EXIT_SRCS-y += $(LIBMUSL)/src/exit/_Exit.c

However, both source files are part of the extracted musl archive, so please do not rename any files. I'd actually name the variant |exit instead of |unikraft because the file is - if you want - from musl's exit subsystem. So far we used the |unikraft variant for glue code that had a conflicting source file name.

Thanks, Simon

@Starnox Starnox changed the title Makefile.uk.musl.exit: Patch for building on MacOS Makefile.uk.musl.exit: Patch for building on case insensitive file systems Aug 11, 2023
Add `exit` variant to _Exit.c to prevent symbol conflicts. This
is necessary since some file systems are case insensitive, and it could
otherwise lead to a conflict with the object resulted from _exit.c

Signed-off-by: Eduard-Florin Mihailescu <mihailescu.eduard@gmail.com>
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Thank you @Starnox
Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

Copy link
Member

@skuenzer skuenzer left a comment

Choose a reason for hiding this comment

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

Reviewed-by: Simon Kuenzer simon@unikraft.io
Approved-by: Simon Kuenzer simon@unikraft.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/merged enhancement New feature or request
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

5 participants