Skip to content

fix: remove -stdlib=libc++ from setup helpers, not needed on modern Pythons #4639

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

Merged
merged 2 commits into from
May 1, 2023

Conversation

biergaizi
Copy link
Contributor

@biergaizi biergaizi commented Apr 25, 2023

Description

On macOS, by default, pybind11 currently unconditionally set the compiler flag -stdlib=libc++ in Pybind11Extension.__init__(), regardless of which compiler is used. This flag is required for clang, but is invalid for GCC. If GCC is used, it causes compilation failures in all Python projects that use pybind11, with the error message:

arm64-apple-darwin22-gcc: error: unrecognized command-line option -stdlib=libc++.

This commit uses has_flag() to detect whether -stdlib=libc++ on macOS, and injects this flag from build_ext.build_extensions(), rather than setting it unconditionally.

This PR resolves Issue #4637.

Suggested changelog entry:

No longer inject -stdlib=libc++, not needed for modern Pythons (macOS 10.9+)

@biergaizi biergaizi requested a review from henryiii as a code owner April 25, 2023 15:24
@biergaizi biergaizi force-pushed the master branch 6 times, most recently from e20d6df to e586135 Compare April 25, 2023 15:46
@biergaizi
Copy link
Contributor Author

biergaizi commented Apr 25, 2023

Just tested it on an Apple Silicon macOS development server, but please be sure to double check it, I have no prior experience with pybind11.

…#4637.

On macOS, by default, pybind11 currently unconditionally set the compiler
flag "-stdlib=libc++" in Pybind11Extension.__init__(), regardless of which
compiler is used. This flag is required for clang, but is invalid for GCC.
If GCC is used, it causes compilation failures in all Python projects that
use pybind11, with the error message:

    arm64-apple-darwin22-gcc: error: unrecognized command-line option -stdlib=libc++.

This commit uses has_flag() to detect whether "-stdlib=libc++" on macOS,
and injects this flag from build_ext.build_extensions(), rather than
setting it unconditionally.

Signed-off-by: Yifeng Li <tomli@tomli.me>
@henryiii
Copy link
Collaborator

hasflag is slow and probably not supported correctly when cross-compiling, so not sure I like adding more usages of it. Is there a better way to tell if the target compiler is clang?

Also, do we really need it? I think this was only important when MACOSX_DEPLOYMENT_TARGET was < 10.9, but now that PyPI has updated to 10.10, I don't think there are any platforms left that might set it to < 10.9.

@biergaizi
Copy link
Contributor Author

Is there a better way to tell if the target compiler is clang?

I cannot think of a better idea... Sometimes, even gcc can be an alias of clang.

Also, do we really need it? I think this was only important when MACOSX_DEPLOYMENT_TARGET was < 10.9, but now that PyPI has updated to 10.10, I don't think there are any platforms left that might set it to < 10.9.

If legacy compatibility is unimportant, this flag can be removed entirely.

@biergaizi
Copy link
Contributor Author

New idea: we can run has_flag() only when a legacy macOS is detected, and we skip both the check and the flag entirely otherwise. This way, there's no performance penalty by default, while compatibility is preserved.

@gnaggnoyil
Copy link

gnaggnoyil commented Apr 25, 2023

Isn't self.compiler kept unchanged during the for ext in self.extensions loop? Do we really need to put the has_stdlib_libc(self.compiler) inside the loop?

Also I would like to suggest to rename has_stdlib_libc to has_stdlib_libcxx, because of... obviousity :)

@biergaizi
Copy link
Contributor Author

Isn't self.compiler kept unchanged during the for ext in self.extensions loop? Do we really need to put the has_stdlib_libc(self.compiler) inside the loop?

Yeah, the check is better to be kept out of the loop.

Also I would like to suggest to rename has_stdlib_libc to has_stdlib_libcxx, because of... obviousity :)

It was my intention as well, somehow I forgot about it while editing the file...

@henryiii
Copy link
Collaborator

The only system I'm aware of that a user might run across with <10.9 as a target would be the original python.org Python 3.6 "fat" installer (or an old PyPy which as 10.7, but we do not support older PyPy's). Everything else is 10.9 or 10.10 (PyPy). So I think we are probably safe dropping unless I'm wrong.

@henryiii henryiii changed the title Inject -stdlib=libc++ on macOS only when it's supported, close #4637. fix: remove -stdlib=libc++ from setup helpers, not needed on modern Pythons Apr 28, 2023
Copy link
Collaborator

@Skylion007 Skylion007 left a comment

Choose a reason for hiding this comment

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

@henryiii This PR looks good to go now, right? Any blockers?

@henryiii henryiii merged commit da91926 into pybind:master May 1, 2023
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label May 1, 2023
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants