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

Remove some linux-specific musl headers #17704

Merged
merged 1 commit into from
Aug 24, 2022
Merged

Remove some linux-specific musl headers #17704

merged 1 commit into from
Aug 24, 2022

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Aug 23, 2022

We don't implement any of the functions declared in these headers, so
there is not real use for them, and I think it can be misleading to allow
them to be included at compile time.

See already did this for sendfile.h: #14248

See #17638

@sbc100 sbc100 requested review from kripken and dschuff August 23, 2022 04:40
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.

Changing the arch file for emscripten sounds good, but the other changes are in generic code, which means they'll show up in diffs to upstream musl and make upgrading less simple, won't they?

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 23, 2022

Changing the arch file for emscripten sounds good, but the other changes are in generic code, which means they'll show up in diffs to upstream musl and make upgrading less simple, won't they?

All of the other changes to upstream header are simply removing entire files. I decided it was cleaner just to remove them completely. We could create a list of files to exclude in system/lib/update_musl.py in perhaps, but I'm not sure its necessary right now. Its easy to avoid re-adding them up update I think.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 23, 2022

Changing the arch file for emscripten sounds good, but the other changes are in generic code, which means they'll show up in diffs to upstream musl and make upgrading less simple, won't they?

All of the other changes to upstream header are simply removing entire files. I decided it was cleaner just to remove them completely. We could create a list of files to exclude in system/lib/update_musl.py in perhaps, but I'm not sure its necessary right now. Its easy to avoid re-adding them up update I think.

The only other change in this PR is a change to test file..

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 23, 2022

Add a ChangeLog entry just in case.

@sbc100 sbc100 force-pushed the remove_linux_headers branch from f4fb5d7 to 84bdabc Compare August 23, 2022 20:18
@kripken
Copy link
Member

kripken commented Aug 23, 2022

It's easy to avoid re-adding them, but it is another thing to need to remember to do. Musl adding a new file might be confusing here, also - both would look like a file that only exists in upstream.

I'm not strongly opposed, but this seems like fairly low benefit, and there is some risk of breakage (like happened in our tests, it seems) so I'm not sure this is worth it? How big do you see the benefit as?

We don't implement any of the functions declared in these headers, so
there is not real use them, and I think it can be misleading to allow
them to be included at compile time.

See #17638
@sbc100 sbc100 force-pushed the remove_linux_headers branch from 84bdabc to 562e853 Compare August 23, 2022 21:13
@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 23, 2022

It's easy to avoid re-adding them, but it is another thing to need to remember to do. Musl adding a new file might be confusing here, also - both would look like a file that only exists in upstream.

I'm not strongly opposed, but this seems like fairly low benefit, and there is some risk of breakage (like happened in our tests, it seems) so I'm not sure this is worth it? How big do you see the benefit as?

I updated the update_musl.py script to match these changes. I think it is a lot cleaner/better not to ship headers for random stuff we don't support.

When I run update_musl.py against the emscripten branch of musl it now does not create any new files.

Also, note that we had already pruned at least one file prior to this change "sendfile.h". See #16234

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 23, 2022

And #14248

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.

Ok, fair enough. I don't feel strongly and those are reasonable reasons.

@sbc100 sbc100 merged commit 655ad88 into main Aug 24, 2022
@sbc100 sbc100 deleted the remove_linux_headers branch August 24, 2022 02:05
sbc100 added a commit that referenced this pull request Oct 13, 2022
This the removal of headers that we don't support was started in #17704.

See #18050
sbc100 added a commit that referenced this pull request Oct 18, 2022
This the removal of headers that we don't support was started in #17704.

See #18050
sbc100 added a commit that referenced this pull request Oct 18, 2022
This the removal of headers that we don't support was started in #17704.

See #18050
@sbc100 sbc100 mentioned this pull request Nov 3, 2022
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.

2 participants