Skip to content

Prepend Pybind11Extension flags rather than appending them. - #2808

Merged
henryiii merged 1 commit into
pybind:masterfrom
anntzer:prepend-flags
Jan 19, 2021
Merged

Prepend Pybind11Extension flags rather than appending them.#2808
henryiii merged 1 commit into
pybind:masterfrom
anntzer:prepend-flags

Conversation

@anntzer

@anntzeranntzer commented Jan 19, 2021

Copy link
Copy Markdown
Contributor

Description

This allows users to set e.g. extra_compile_args=["-g"] (temporarily,
for debugging purposes) and not have that get overridden by
Pybind11Extension's default -g0.

In order to minimize "order inversion" (not that it really matters,
though) I chose to collect all flags and add them at once (hence the
dropping of *args). I also dropped flag deduplication, which seems
unneeded.

Suggested changelog entry:

* ``extra_compile_args`` and ``extra_link_args`` automatically set by
Pybind11Extension are now prepended, which allows them to be overridden
by user-set ``extra_compile_args`` and ``extra_link_args``.

This allows users to set e.g. `extra_compile_args=["-g"]` (temporarily,
for debugging purposes) and not have that get overridden by
Pybind11Extension's default `-g0`.
In order to minimize "order inversion" (not that it really matters,
though) I chose to collect all flags and add them at once (hence the
dropping of `*args`). I also dropped flag deduplication, which seems
unneeded.
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
Copy Markdown
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: Python2.7isnotrecommended.
ThisversionisincludedinmacOSforcompatibilitywithlegacysoftware.
FutureversionsofmacOSwillnotincludePython2.7.
Instead, itisrecommendedthatyoutransitiontousing'python3'fromwithinTerminal.
Python2.7.16 (default, Nov232020, 08:01:20)
[GCCAppleLLVM12.0.0 (clang-1200.0.30.4) [+internal-os, ptrauth-isa=sign+striondarwinType"help", "copyright", "credits"or"license"formoreinformation.
>>>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. :)

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

Copy link
Copy Markdown
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?

@anntzeranntzerJan 19, 2021

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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.

@henryiii

henryiii commented Jan 19, 2021

Copy link
Copy Markdown
Collaborator

Question you might be able to answer: do you know how to get setuptools to put the user includes before the system includes? Setuptools adds the main directory in front of the user includes (specifically, the pybind11 one), so if you have a system install of pybind11, it might override the local one if it's in the same places as the Python include. I'm not sure of this, where I've seen the bug is for boost - my system install of boost was overriding the local one I was trying to provide in Pybind11Extension. I fixed this in CMake, but don't know how to fix it in setuptools at the moment.

@henryiiihenryiii added this to the v2.6.2 milestone Jan 19, 2021
@anntzer

Copy link
Copy Markdown
ContributorAuthor

Question you might be able to answer: do you know how to get setuptools to put the user includes before the system includes? Setuptools adds the main directory in front of the user includes (specifically, the pybind11 one), so if you have a system install of pybind11, it might override the local one if it's in the same places as the Python include.

Just to be clear, this is unrelated to this PR? I can have a look, but perhaps we can move the discussion to a separate issue?

In any case I'm not sure what's the behavior you're mentioning, as trying e.g. to compile python_example with PYTHONPATH set to /path/to/pybind11/checkout results for me in

gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -march=x86-64 -mtune=generic -O3 -pipe -fno-plt -fno-semantic-interposition -march=x86-64 -mtune=generic -O3 -pipe -fno-plt -march=x86-64 -mtune=generic -O3 -pipe -fno-plt -fPIC -DVERSION_INFO=0.0.1 -I/path/to/pybind11/checkout/pybind11/include -I/usr/include/python3.9 -c src/main.cpp -o build/temp.linux-x86_64-3.9/src/main.o -fvisibility=hidden -g0 -std=c++17

so here it's indeed the local install that comes first.

@henryiiihenryiii reopened this Jan 19, 2021
@henryiii
henryiii merged commit e8c4f54 into pybind:masterJan 19, 2021
@henryiii

Copy link
Copy Markdown
Collaborator

Yes, it's a separate issue. I'll try to test it a bit more in the near future, and I'll refer back here if I can show it again. Thanks!

@github-actionsgithub-actionsBot added the needs changelog Possibly needs a changelog entry label Jan 19, 2021
@anntzer
anntzer deleted the prepend-flags branch January 19, 2021 23:22
@anntzer

Copy link
Copy Markdown
ContributorAuthor

feel free to ping me as needed.

@henryiiihenryiii removed the needs changelog Possibly needs a changelog entry label Jan 25, 2021
@rwgkrwgk mentioned this pull request Feb 10, 2023
Sign up for freeto 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

@anntzer@henryiii