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

[physfs] Patches to fix behavior on macOS / Linux #15962

Merged
merged 5 commits into from Jul 23, 2021

Conversation

past-due
Copy link
Contributor

Describe the pull request

Adds two patches to fix behavior (and runtime failure handling) on macOS / Linux / other POSIX-compatible.

  • Handle EINTR (fixes runtime failures - more common on macOS Big Sur, and depending on system configuration and many other factors)
  • Adds missing O_CLOEXEC / FD_CLOEXEC (recommended behavior for libraries)

All triplets should be supported.

I have reached out to upstream about these patches. Will update when there's a response.

Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

Can you report this to the upstream?

Thanks.

@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support requires:author-response labels Feb 1, 2021
@past-due
Copy link
Contributor Author

past-due commented Feb 1, 2021

Can you report this to the upstream?

Thanks.

@JackBoosY:
I have reported this to the maintainer (no public link though, as unfortunately there doesn't appear to be a public bug-tracker so I had to message directly). Will post a comment here with updates whenever I hear back.

@past-due past-due marked this pull request as ready for review February 1, 2021 21:23
@JackBoosY JackBoosY added depends:upstream-changes Waiting on a change to the upstream project and removed requires:author-response labels Feb 2, 2021
@ras0219-msft
Copy link
Contributor

I'm uneasy about adding these code changes without some guidance from upstream -- I'm not familiar enough with the specific semantics and codebase to properly review it.

Assuming upstream gives the OK, then this LGTM

@JackBoosY
Copy link
Contributor

Ping @past-due for response, does the upstream accept this changes?

@past-due
Copy link
Contributor Author

Ping @past-due for response, does the upstream accept this changes?

Haven't heard back - I will ping them again.

@past-due
Copy link
Contributor Author

@JackBoosY I'm pleased to note that upstream has now accepted these patches:

icculus/physfs@a9cb207
icculus/physfs@b8fa8fd

(They also recently moved to GitHub, which made this a bit easier.)

ports/physfs/portfile.cmake Outdated Show resolved Hide resolved
ports/physfs/portfile.cmake Outdated Show resolved Hide resolved
@JackBoosY JackBoosY removed the depends:upstream-changes Waiting on a change to the upstream project label Jul 15, 2021
versions/p-/physfs.json Outdated Show resolved Hide resolved
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jul 22, 2021
@vicroms vicroms merged commit aaeca7b into microsoft:master Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants