Skip to content
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

Bump the minimum pybind11 that we need #2453

Merged
merged 2 commits into from
Jan 7, 2020

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Jan 4, 2020

Recent changes (2347) seem to rely on some pybind declarations that
are newer than the minimum version we checked for. Bump the pybind11
required version.

Recent changes (2347) seem to rely on some pybind declarations that
are newer than the minimum version we checked for. Bump the pybind11
required version.
@lgritz
Copy link
Collaborator Author

lgritz commented Jan 4, 2020

Did this work, @sobotka ?

@sobotka
Copy link
Contributor

sobotka commented Jan 4, 2020

Checking now. Hold tight.

@sobotka
Copy link
Contributor

sobotka commented Jan 4, 2020

@lgritz clean cmake directly to your lg-oiio repository using the lg-pybind11 branch still seems to bomb out.

Topmost error:

[ 95%] Building CXX object src/python/CMakeFiles/PyOpenImageIO.dir/py_colorconfig.cpp.o
In file included from /lg-oiio/src/python/py_colorconfig.cpp:31:
/lg-oiio/src/python/py_oiio.h: In function ‘bool PyOpenImageIO::py_buffer_to_stdvector(std::vector<T>&, const pybind11::buffer&)’:
/lg-oiio/src/python/py_oiio.h:267:36: error: no matching function for call to ‘pybind11::buffer::request() const’
     oiio_bufinfo binfo(obj.request());

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 4, 2020

Can you send me the full verbose output of a fresh build from scratch? If you use the makefile wrapper:

make nuke ; make | tee build.log

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 4, 2020

Sorry, I meant:

make nuke ; make VERBOSE=1 | tee build.log

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 4, 2020

It's not enough just to install the patch, you need to start from the initial cmake config step in order to get it to download the right pybind11 version.

@sobotka
Copy link
Contributor

sobotka commented Jan 4, 2020

Trying another clean directory cmake build. Will ship log if error.

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 4, 2020

That can't be the whole thing, it doesn't show the cmake config or generation steps at all.

@sobotka
Copy link
Contributor

sobotka commented Jan 4, 2020

Replacing

    if ((BUILD_MISSING_PYBIND11 AND NOT PYBIND11_INCLUDES) OR BUILD_PYBIND11_FORCE)

with

    if ((BUILD_MISSING_PYBIND11 AND NOT PYBIND11_FOUND) OR BUILD_PYBIND11_FORCE)

in pythonutils.cmake fixes this, as per @lgritz

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 4, 2020

Yes, Troy and I had a long off-ticket conversation. There was a bug, as cited above, where if you had pybind11 on your system but it was too old a version, it wouldn't do the auto-download like it was supposed to (and like it did, when it didn't find any pybind11 on your system).

I will amend the PR with that fix.

@lgritz
Copy link
Collaborator Author

lgritz commented Jan 5, 2020

Amended...

Fix logic in pybind11 auto-download:

If no pybind11 was found on the system, it would correctly auto-download.
I botched the test where, if a pybind11 was found but was an insufficient
version, it would not auto-download as intended. This was just an incorrect
test of PYBIND11_INCLUDES when it should have tested PYBIND11_FOUND.

I also augmented the logic to print a more helpful message pybind11
is found but is insufficient version, and furthermore if it's found
in the ext/ area (the auto-downloaded one) but still too old, it will
remind you that it needs to be blown away and refreshed. You'll see
something like this in the cmake configure output:

-- Pybind11 was found in /Users/lg/code/oiio/oiio.lg/ext/pybind11/include
-- ... but was version 2.4.0 (minimum is 2.4.2)
-- Try removing ext/pybind11 and let me download a newer version.

If no pybind11 was found on the system, it would correctly auto-download.
I botched the test where, if a pybind11 was found but was an insufficient
version, it would not auto-download as intended. This was just an incorrect
test of PYBIND11_INCLUDES when it should have tested PYBIND11_FOUND.

I also augmented the logic to print a more helpful message pybind11
is found but is insufficient version, and furthermore if it's found
in the ext/ area (the auto-downloaded one) but still too old, it will
remind you that it needs to be blown away and refreshed. You'll see
something like this in the cmake configure output:

    -- Pybind11 was found in /Users/lg/code/oiio/oiio.lg/ext/pybind11/include
    -- ... but was version 2.4.3 (minimum is 2.4.4)
    -- Try removing ext/pybind11 and let me download a newer version.
@lgritz lgritz merged commit adc3fa4 into AcademySoftwareFoundation:master Jan 7, 2020
@lgritz lgritz deleted the lg-pybind11 branch January 7, 2020 07:40
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Jan 7, 2020
Recent changes (2347) seem to rely on some pybind declarations that
are newer than the minimum version we checked for. Bump the pybind11
required version.

If no pybind11 was found on the system, it would correctly auto-download.
I botched the test where, if a pybind11 was found but was an insufficient
version, it would not auto-download as intended. This was just an incorrect
test of PYBIND11_INCLUDES when it should have tested PYBIND11_FOUND.

I also augmented the logic to print a more helpful message pybind11
is found but is insufficient version, and furthermore if it's found
in the ext/ area (the auto-downloaded one) but still too old, it will
remind you that it needs to be blown away and refreshed. You'll see
something like this in the cmake configure output:

    -- Pybind11 was found in /Users/lg/code/oiio/oiio.lg/ext/pybind11/include
    -- ... but was version 2.4.3 (minimum is 2.4.4)
    -- Try removing ext/pybind11 and let me download a newer version.
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Jan 7, 2020
Recent changes (2347) seem to rely on some pybind declarations that
are newer than the minimum version we checked for. Bump the pybind11
required version.

If no pybind11 was found on the system, it would correctly auto-download.
I botched the test where, if a pybind11 was found but was an insufficient
version, it would not auto-download as intended. This was just an incorrect
test of PYBIND11_INCLUDES when it should have tested PYBIND11_FOUND.

I also augmented the logic to print a more helpful message pybind11
is found but is insufficient version, and furthermore if it's found
in the ext/ area (the auto-downloaded one) but still too old, it will
remind you that it needs to be blown away and refreshed. You'll see
something like this in the cmake configure output:

    -- Pybind11 was found in /Users/lg/code/oiio/oiio.lg/ext/pybind11/include
    -- ... but was version 2.4.3 (minimum is 2.4.4)
    -- Try removing ext/pybind11 and let me download a newer version.
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Jan 7, 2020
Recent changes (2347) seem to rely on some pybind declarations that
are newer than the minimum version we checked for. Bump the pybind11
required version.

If no pybind11 was found on the system, it would correctly auto-download.
I botched the test where, if a pybind11 was found but was an insufficient
version, it would not auto-download as intended. This was just an incorrect
test of PYBIND11_INCLUDES when it should have tested PYBIND11_FOUND.

I also augmented the logic to print a more helpful message pybind11
is found but is insufficient version, and furthermore if it's found
in the ext/ area (the auto-downloaded one) but still too old, it will
remind you that it needs to be blown away and refreshed. You'll see
something like this in the cmake configure output:

    -- Pybind11 was found in /Users/lg/code/oiio/oiio.lg/ext/pybind11/include
    -- ... but was version 2.4.3 (minimum is 2.4.4)
    -- Try removing ext/pybind11 and let me download a newer version.
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Jan 9, 2020
Recent changes (2347) seem to rely on some pybind declarations that
are newer than the minimum version we checked for. Bump the pybind11
required version.

If no pybind11 was found on the system, it would correctly auto-download.
I botched the test where, if a pybind11 was found but was an insufficient
version, it would not auto-download as intended. This was just an incorrect
test of PYBIND11_INCLUDES when it should have tested PYBIND11_FOUND.

I also augmented the logic to print a more helpful message pybind11
is found but is insufficient version, and furthermore if it's found
in the ext/ area (the auto-downloaded one) but still too old, it will
remind you that it needs to be blown away and refreshed. You'll see
something like this in the cmake configure output:

    -- Pybind11 was found in /Users/lg/code/oiio/oiio.lg/ext/pybind11/include
    -- ... but was version 2.4.3 (minimum is 2.4.4)
    -- Try removing ext/pybind11 and let me download a newer version.
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.

None yet

2 participants