Skip to content

Conversation

ssiano
Copy link
Contributor

@ssiano ssiano commented Dec 22, 2020

When pybind11 is included as a submodule, the user needs to update their
Python module search path. Otherwise, the first c++ compilation command
in docs/basics.rst will fail.

When pybind11 is included as a submodule, the user needs to update their
Python module search path.  Otherwise, the first c++ compilation command
in docs/basics.rst will fail.
@ssiano
Copy link
Contributor Author

ssiano commented Dec 22, 2020

This bit of documentation may be helpful to new users. It would have been helpful to me when I first went through the instructions in the doc.

If you believe so, feel free to rewrite it as you see fit and discard this pull request.

@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Dec 22, 2020

I believe the idea is that if you use pybind11 as a git submodule, that you don't install it through pip. So you don't need to call python3 -m pybind11 --includes, because you know where the pybind11 includes are (i.e., PATH_TO/extern/pybind11/include or so, as noted in the paragraph above.)

I don't even know if python -m pybind11 is supposed to work when pybind11 is not pip-installed. I don't believe so, at least. @henryiii?

@henryiii
Copy link
Collaborator

henryiii commented Dec 22, 2020

Yes, it is supposed to work, though I would do it this way: https://github.com/scikit-hep/boost-histogram/blob/ca298e7c9008025191e94a927fbd0d1bd012b97f/setup.py#L11-L14

Or, better yet, refer to the docs here under "Suggested usage if you have pybind11 as a submodule": https://pybind11.readthedocs.io/en/stable/compiling.html#copy-manually :)

You should never be setting PYTHONPATH if you are building a library, and an install step must be self contained and not depend on the users environment.

@henryiii
Copy link
Collaborator

henryiii commented Dec 22, 2020

Ahh, you are looking at building it by hand, but using it as a submodule? You generally should always be using setuptools or CMake if you are working with a library containing a submodule, not building it by hand. That's just a demo to show you how it works and prove it is simple. If you do want to build it by hand but you don't want to install it, then sure, but I don't think it needs to be in the docs. An end-user can use PYTHONPATH, though I still recommend staying away from it.

@ssiano
Copy link
Contributor Author

ssiano commented Dec 22, 2020

That's a good point @YannickJadoul. Maybe it would be better to update docs/basics.rst to show how to do the c++ compilation when using pybind11 as a Git submodule.

Currently that command is

c++ -O3 -Wall -shared -std=c++11 -fPIC `python3 -m pybind11 --includes` example.cpp -o example`python3-config --extension-suffix`

python -m pybind11 does work when pybind11 is not pip-installed as long as you use PYTHONPATH. But as you mention this may not be necessary if the user knows that the includes can be specified such as -I/usr/include/python3.8 -IPATH_TO/extern/pybind11/include. However, the user may not know the first part (the -I/usr/include/python3.8).

@henryiii
Copy link
Collaborator

the user knows that the includes can be specified

It's best written as python3-config --includes, I think.

@henryiii
Copy link
Collaborator

Or python3-config --cflags to get all the flags and such.

@henryiii
Copy link
Collaborator

I also prefer $() to backticks, it's much easier to avoid missing, but have never proposed the change.

@henryiii
Copy link
Collaborator

henryiii commented Dec 22, 2020

c++ -shared -std=c++11 -fPIC $(python3-config --cflags) <path to pybind11/include> example.cpp \
    -o example$(python3-config --extension-suffix)

I think is the full, correct way to do it all in one line. For me, it expands to:

c++ -shared -std=c++11 -fPIC -I/usr/local/Cellar/[email protected]/3.9.1/Frameworks/Python.framework/Versions/3.9/include/python3.9 \
  -I/usr/local/Cellar/[email protected]/3.9.1/Frameworks/Python.framework/Versions/3.9/include/python3.9 \
  -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG \
  -g -fwrapv -O3 -Wall -I/usr/local/include \
  -I/Library/Developer/CommandLineTools/SDKs/MacOSX11.0.sdk/usr/include \
  -I/Library/Developer/CommandLineTools/SDKs/MacOSX11.0.sdk/System/Library/Frameworks/Tk.framework/Versions/8.5/Headers \
  <path to pybind11/include> example.cpp -o example.cpython-39-darwin.so

But why are we doing this by hand instead in CMake or setuptools? And why are you expecting python -m pybind11 to work without installing pybind11?

@ssiano
Copy link
Contributor Author

ssiano commented Dec 22, 2020

That command looks good Henry. You would just need to put the -I before the <path to pybind11/include>.

I'm not sure why python3-config --cflags list the first include path twice, but it does the same for me on three different platforms.

Regarding doing it by hand, this is just what a first-time visitor to the doc might try when following the demo.

@henryiii
Copy link
Collaborator

Maybe we could just add a note that pybind11 should be installed for these python -m pybind11 commands to work?

@ssiano
Copy link
Contributor Author

ssiano commented Dec 22, 2020

Or maybe a note to use $(python3-config --includes) -Iextern/pybind11/include in place of $(python3 -m pybind11 --includes) if you are doing this demo with pybind11 included as a submodule.

This note shows how to modify the compilation command for the example
when the pybind11 source has been included as a Git submodule.
@ssiano ssiano changed the title docs: mention PYTHONPATH in installing.rst docs: add a note about compiling the example Dec 23, 2020
Added an internal link to the docs
Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

Looks much better. Let's get at least one more approval before merging, though.

docs/basics.rst Outdated
.. note::

If you used :ref:`include_as_a_submodule` to install the pybind11 source,
then use ``$(python3-config --includes) -Iextern/pybind11/include``
Copy link
Collaborator

Choose a reason for hiding this comment

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

-I should really be -isystem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, but the pybind11-config and the Python-config both use -I I believe, so it would be a little odd if this causes pybind11 to be system but not Python.h.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
then use ``$(python3-config --includes) -Iextern/pybind11/include``
then use ``$(python3-config --includes) -isystem extern/pybind11/include``

For a simple example, not sure this is best, but it's also better to show the correct thing if it's visible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm... You're right that pybind11-config and python-config both use -I. I'd argue that both of those are wrong. I personally use -isystem /usr/include/python3.9 and my cmake adds both with SYSTEM.

Anyway, I'm not super determined to change this -I to -isystem.

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

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

Thanks, @ssiano! This seems both more correct, and in a better place (i.e., under manual compiling, rather than seemingly suggesting to manually compile a submodule; CMake should still be preferred, if possible, I think) :-)

@YannickJadoul YannickJadoul added this to the v2.6.2 milestone Dec 23, 2020
@ssiano
Copy link
Contributor Author

ssiano commented Dec 24, 2020

Now I'm thinking that any new note in "First steps" should reference the comments in https://pybind11.readthedocs.io/en/latest/compiling.html#building-manually.

So please don't merge this for now.

@henryiii
Copy link
Collaborator

Those probably should be $() too.

@henryiii
Copy link
Collaborator

So please don't merge this for now.

Then convert to draft. :)

Also updated the command substitution syntax for consistency
@ssiano ssiano marked this pull request as draft December 24, 2020 02:28
@ssiano ssiano marked this pull request as ready for review December 24, 2020 03:06
@henryiii henryiii merged commit 6f66e76 into pybind:master Dec 24, 2020
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Dec 24, 2020
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Dec 24, 2020
@henryiii
Copy link
Collaborator

Thanks!

Sign up for free to 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.

4 participants