Skip to content

Ship pybind11 v2.2.3 - #281

Merged
anokfireball merged 6 commits into
openPMD:devfrom
ax3l:topic-pybind11internal
Jun 21, 2018
Merged

Ship pybind11 v2.2.3#281
anokfireball merged 6 commits into
openPMD:devfrom
ax3l:topic-pybind11internal

Conversation

@ax3l

@ax3lax3l commented Jun 20, 2018

Copy link
Copy Markdown
Member

Add an internal version of pybind11, version 2.2.3.

Modifications from the release:

Increase requirement from pybind 2.2.1+ to 2.2.3+ with latest fixes.

@ax3lax3l added install third party third party libraries that are shipped and/or linked frontend: Python3 labels Jun 20, 2018
@ax3l
ax3l requested a review from anokfireballJune 20, 2018 13:01
@ax3l
ax3lforce-pushed the topic-pybind11internal branch from 5fa6d45 to eb36627CompareJune 20, 2018 13:02
@ax3lax3l mentioned this pull request Jun 20, 2018
Comment threadCHANGELOG.rst
- CMake:

- add ``openPMD::openPMD`` alias for full-source inclusion #277
- include internally shipped pybind11 v2.2.3 #281

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should probably note somewehere that the (currently) included version includes modifications w.r.t. the actual pybind11 v2.2.3. This would be a good place.

@ax3lax3lJun 20, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

in principle, absolutely yes.

the modifications I did are only relevant for direct inclusion via add_subdirectories (and to save some space: tests). if one switches to openPMD_USE_INTERNAL_PYBIND11=OFF with the exact same release, they are not needed.

Therefore, I think the note in the commit message is good enough if one wants to see what was added when.

Comment threadCMakeLists.txt
set(openPMD_HAVE_PYTHON TRUE)
else()
set(openPMD_HAVE_PYTHON FALSE)
find_package(pybind11 2.2.3 CONFIG) # 2.3.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we require the changes from pybind/pybind11#1434?
If so, this is actually a lie.

@ax3lax3lJun 20, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

no, we don't need those, every public release in the allowed version range will work :-)
that's the beauty of it, see above :)

Comment thread.travis.yml
python@$TRAVIS_PYTHON_VERSION
$COMPILERSPEC &&
spack load --dependencies py-pybind11@2.2.1 ^python@$TRAVIS_PYTHON_VERSION $COMPILERSPEC;
spack load python@$TRAVIS_PYTHON_VERSION $COMPILERSPEC;

@anokfireballanokfireballJun 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't need to "install" pybind11 beforehand now.

True, but for a reasons similar to #213, we should consider abusing one entry in the build matrix where we do.

@ax3lax3lJun 20, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

maybe we should abuse one to install the latest pybind11 version (since we ship the oldest supported version)?

Still, here it's a bit less severe, since we ship a supported version and the ones getting into trouble are packagers (well, and spack users if we keep the upper version range open).

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Have to find a creative idea how to add extra cmake flags without being super-verbose.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@C0nsultant fixed :)

@ax3l
ax3lforce-pushed the topic-pybind11internal branch 2 times, most recently from a76090e to 1d55e59CompareJune 20, 2018 14:35
Comment thread.travis.yml Outdated
spack load python@$TRAVIS_PYTHON_VERSION $COMPILERSPEC;
if [ "$USE_INTERNAL_PYBIND11" == "OFF" ]; then
travis_wait spack install
py-pybind11@2.2.1 ^python@$TRAVIS_PYTHON_VERSION

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the heat of the moment, you probably forgot that you bumped the minimum.
py-pybind11@2.2.3

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

copy-pasted from the diff when re-adding it xD

Comment thread.travis.yml Outdated
travis_wait spack install
py-pybind11@2.2.1 ^python@$TRAVIS_PYTHON_VERSION
$COMPILERSPEC &&
spack load py-pybind11@2.2.1 ^python@$TRAVIS_PYTHON_VERSION $COMPILERSPEC;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

heat of the... day!

@ax3l
ax3lforce-pushed the topic-pybind11internal branch from 1d55e59 to 1c5646cCompareJune 20, 2018 14:39
@ax3l

ax3l commented Jun 21, 2018

Copy link
Copy Markdown
MemberAuthor

@C0nsultant ok to be merged? :) update: ah.. changelog.. argh.

wjakoband others added 6 commits June 21, 2018 10:53
Add an internal version of pybind11, version 2.2.3.
Modifications from the releases:
- add fixpybind/pybind11#1434
- remove test dirs
- remove wheel packaging (setup.py/cfg)
- remove github templates
Add an internal copy of pybind11 for convenient installs.
Increase requirement from pybind 2.2.1+ to 2.2.3+ with latest
fixes.
We don't need to "install" pybind11 beforehand now.
@ax3l
ax3lforce-pushed the topic-pybind11internal branch from 1c5646c to bc59b40CompareJune 21, 2018 08:54
@anokfireball
anokfireball merged commit 9cad2d2 into openPMD:devJun 21, 2018
@ax3l
ax3l deleted the topic-pybind11internal branch June 21, 2018 11:45
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend: Python3installthird partythird party libraries that are shipped and/or linked

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ax3l@anokfireball@wjakob