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 missed musl sendfile.h #16200

Closed
wants to merge 3 commits into from
Closed

Add missed musl sendfile.h #16200

wants to merge 3 commits into from

Conversation

VirtualTim
Copy link
Collaborator

@VirtualTim VirtualTim commented Feb 4, 2022

For some reason this file was missing, but is present in the musl repo: https://github.com/emscripten-core/musl/blob/master/include/sys/sendfile.h.
I created a bug on emsdk (emscripten-core/emsdk#983), but it looks like the issue was actually here.

Edit: I realise now that this file was intentionally removed (#14248). I've replaced the contents with a compiler warning linking to the PR that removed.
I don't know if this is a good idea or not, so welcome some input.

Copy link
Member

@kripken kripken 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 this makes sense. A clear compile error sounds good.

Maybe the text could be "sendfile is not supported" instead of "was removed"?

@sbc100 what do you think?

@sbc100
Copy link
Collaborator

sbc100 commented Feb 4, 2022

Why not simply not include it?

@sbc100
Copy link
Collaborator

sbc100 commented Feb 4, 2022

I think if a header file is completely unsupported (e.g. windows.h) is seems reasonable to not include it at all.

@sbc100
Copy link
Collaborator

sbc100 commented Feb 4, 2022

We should probably remove a bunch of other linux-specific headers too I guess.

@VirtualTim
Copy link
Collaborator Author

From my point of view, a project I was compiling with 1.39.6 could no longer compile when upgrading to 3.1.3. As the error was a missing system header I thought something was wrong with my Emscripten setup. I wasted a bunch of time before I released that the file had actually been intentionally removed. I could also see this being useful for people trying to port existing software, as something like this will actually tell them what to do.
But I do get that changing a file to error on include is a bit odd.

Maybe instead of this the removal of sendfile.h should just be mentioned in the changelog?

@sbc100
Copy link
Collaborator

sbc100 commented Feb 9, 2022

Certainly, adding a changelog entry makes sense to me.

@VirtualTim
Copy link
Collaborator Author

Closed in favour of just updating the changelog.

@VirtualTim VirtualTim closed this Feb 11, 2022
@VirtualTim VirtualTim deleted the VirtualTim-sendfile branch February 11, 2022 03:46
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