Skip to content

Prepend Pybind11Extension flags rather than appending them. #2808

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.

Sign up for GitHub

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 1 commit into from
Jan 19, 2021
Merged

Prepend Pybind11Extension flags rather than appending them. #2808

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 25 additions & 18 deletions pybind11/setup_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,14 @@ class Pybind11Extension(_Extension):
this is an ugly old-style class due to Distutils.
"""

def _add_cflags(self, *flags):
for flag in flags:
if flag not in self.extra_compile_args:
self.extra_compile_args.append(flag)
# flags are prepended, so that they can be further overridden, e.g. by
# ``extra_compile_args=["-g"]``.

def _add_lflags(self, *flags):
for flag in flags:
if flag not in self.extra_link_args:
self.extra_link_args.append(flag)
def _add_cflags(self, flags):
self.extra_compile_args[:0] = flags
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow, I did not know this was valid! But it is, even back in 2.7!

WARNING: Python 2.7 is not recommended.
This version is included in macOS for compatibility with legacy software.
Future versions of macOS will not include Python 2.7.
Instead, it is recommended that you transition to using 'python3' from within Terminal.

Python 2.7.16 (default, Nov 23 2020, 08:01:20)
[GCC Apple LLVM 12.0.0 (clang-1200.0.30.4) [+internal-os, ptrauth-isa=sign+stri on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> a = [1,2,3]
>>> a[:0] = [4,5,6]
>>> a
[4, 5, 6, 1, 2, 3]

Even can be used in the middle:

>>> a[5:5] = [10]
>>> a
[4, 5, 6, 1, 2, 10, 3]

Maybe this is common knowledge, but it surprised me. :)


def _add_ldflags(self, flags):
self.extra_link_args[:0] = flags

def __init__(self, *args, **kwargs):

Expand Down Expand Up @@ -139,13 +138,17 @@ def __init__(self, *args, **kwargs):
# Have to use the accessor manually to support Python 2 distutils
Pybind11Extension.cxx_std.__set__(self, cxx_std)

cflags = []
ldflags = []
if WIN:
self._add_cflags("/EHsc", "/bigobj")
cflags += ["/EHsc", "/bigobj"]
else:
self._add_cflags("-fvisibility=hidden", "-g0")
cflags += ["-fvisibility=hidden", "-g0"]
if MACOS:
self._add_cflags("-stdlib=libc++")
self._add_lflags("-stdlib=libc++")
cflags += ["-stdlib=libc++"]
ldflags += ["-stdlib=libc++"]
self._add_cflags(cflags)
self._add_ldflags(ldflags)

@property
def cxx_std(self):
Expand Down Expand Up @@ -174,7 +177,8 @@ def cxx_std(self, level):
if not level:
return

self.extra_compile_args.append(STD_TMPL.format(level))
cflags = [STD_TMPL.format(level)]
ldflags = []

if MACOS and "MACOSX_DEPLOYMENT_TARGET" not in os.environ:
# C++17 requires a higher min version of macOS. An earlier version
Expand All @@ -186,18 +190,21 @@ def cxx_std(self, level):
desired_macos = (10, 9) if level < 17 else (10, 14)
macos_string = ".".join(str(x) for x in min(current_macos, desired_macos))
macosx_min = "-mmacosx-version-min=" + macos_string
self.extra_compile_args.append(macosx_min)
self.extra_link_args.append(macosx_min)
cflags += [macosx_min]
ldflags += [macosx_min]

if PY2:
if WIN:
# Will be ignored on MSVC 2015, where C++17 is not supported so
# this flag is not valid.
self.extra_compile_args.append("/wd5033")
cflags += ["/wd5033"]
elif level >= 17:
self.extra_compile_args.append("-Wno-register")
cflags += ["-Wno-register"]
elif level >= 14:
self.extra_compile_args.append("-Wno-deprecated-register")
cflags += ["-Wno-deprecated-register"]

self._add_cflags(cflags)
self._add_ldflags(ldflags)
Comment on lines +206 to +207
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason these are prepended? This may happen at the beginning (if it is manually specified), at an arbitrary point (if assigned to, in Python 3), or at the end (if left to "automatic"). The order of these particular flags shouldn't really matter, in theory, I'd expect?

Copy link
Contributor Author

@anntzer anntzer Jan 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I chose to just prepend everything to simplify the explanation ("pybind11 puts all its flags at the beginning") (also, perhaps someone really wants to override one of the -Wflags...), but admittedly the only thing I personally really care about is -g, so I can revert the changes on this specific part if you prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is fine, it's the most consistent (I think) if this gets triggered at different times.



# Just in case someone clever tries to multithread
Expand Down