Fix Lib/posixmodule.c compilation with clang on OSX #12038
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
| @@ -20,6 +20,8 @@ | |||
| # pragma weak statvfs | |||
| # pragma weak fstatvfs | |||
|
|
|||
| #include <util.h> /* needed for forkpty() and openpty(). */ | |||
There was a problem hiding this comment.
This portion of the change is duplicative/wrong.
`AC_GNU_SOURCE` (legacy) and `AC_USE_SYSTEM_EXTENSIONS` (new) should be used when conditionally compiling with `_GNU_SOURCE`. Best case scenario, defining the variable to 1 on other platforms can mislead software that relies on these portability constants and result in incorrectly specified `#define`s. Worst case scenario, this will cause some software to break when combined, e.g., libevent2's util.h header getting picked up before the system util.h header on OSX, which will result in a build failure from clang as the `#define` is inconsistently defined between cpython and libevent. This discovery was made in combination with: libevent/libevent#773 . Credit goes to Kevin Bowling (@kev009) for the pattern, obtained from the libevent project [1]. 1. libevent/libevent@ea8fa4c Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>
clang complains about `forkpty` and `openpty` not being defined on OSX. This was occurring because util.h was not being included. Add an explicit util.h #include and while here, fix a potentially ambiguous `sigemptyset` test. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>
The previous code hardcoded `SEEK_SET`, etc. While it's very unlikely that these values will change, it's best to use the definitions to avoid there being mismatches in behavior with the code in the future. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>
ngie-eign
force-pushed
the
fix-Lib-posixmodule-osx-compilation
branch
from
2fe85e7 to
e225130
Compare
|
@ngie-eign, please open an issue on the bug tracker to raise the visibility for this issue. Issues tend to be more discoverable on the bug tracker than if they just have a pull request. Thanks! EDIT: Please also address the merge conflicts. |
This commit fixes building Lib/posixmodule.c with clang on OSX.
Due to a combination of bugs and environment quirks, I was unable to compile posixmodule.c for 2 reasons.
_GNU_SOURCEonly when necessary and in a manner consistent with the autoconf definition to avoid conflicts between projects.forkpty(..)/openpty(..)'s definitions not being found. I suspect that what was happening was that it was finding it via configure, but failing because the header was not included for util.h. Regardless, for correctness, it's best to include util.h on OSX. This should no doubt be extended to other platforms where util.h is present, e.g., NetBSD and OpenBSD, but I will do that in another future commit.