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

pybind11 cmakedeps #9348

Closed
wants to merge 15 commits into from
Closed

Conversation

lasote
Copy link
Contributor

@lasote lasote commented Feb 11, 2022

Specify library name and version: pybind11/all

I'm submitting some changes in the properties and including a new test_package_cmakedeps to make sure the package is correctly consumed with CMakeDeps.


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@lasote lasote changed the title Feature/pybin11 cmakedeps pybind11 cmakedeps Feb 11, 2022
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@lasote lasote force-pushed the feature/pybin11_cmakedeps branch from 0b2f817 to 5f88d7c Compare February 14, 2022 09:06
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

enable_testing()

add_test(run_example
${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/test.py ${CMAKE_CURRENT_BINARY_DIR}/lib)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the same project files (CMakeLists.txt) from test_package, even if you need some if to skip the conanbuildinfo.cmake. It is important to check that both generators use the same find_package call, same target names,...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that is a good practice:

  1. Tricking the layout to include a ../test_package/CMakeLists.txt is very dirty and not something you will do except for this situation.
  2. Introducing an IF in the original CMakelists.txt to skip the conanbuildinfo.cmake would be very very weird, so you edit that file and you are supposed to know that is shared between several test_packages.
  3. When Conan 2.0 is introduced here, we want to remove/rename the test_package folder and it will require extra effort.
  4. I get that using the same target name is important, and that can be checked in the review of the PR.

Overall I think that code reuse, far from helping, will cause more trouble and dirty code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank God it is more or less easy with the layout, just:

    def layout(self):
        cmake_layout(self)
        self.folders.source = os.path.join("..", "test_package")

I find it important, but let's wait for other reviewers, they are the ones that will maintain this recipe and possible inconsistencies between the two files.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@CPickens42
Copy link

I'm pretty sure this is related to #9343 (comment). Is there any news on this PR @jgsogo? 🙂

@ghost ghost mentioned this pull request Apr 27, 2022
@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

Failure in build 6 (7f4738fcc25f635633edd750f945997205638f77):

  • pybind11/2.9.1@:
    Didn't run or was cancelled before finishing

  • pybind11/2.6.0@:
    CI failed to create some packages (All logs)

    Logs for packageID 5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9:
    [settings]
    arch=x86_64
    build_type=Release
    compiler=gcc
    compiler.libcxx=libstdc++11
    compiler.version=5
    os=Linux
    
    [...]
    -- Detecting CXX compiler ABI info - done
    -- Check for working CXX compiler: /usr/bin/g++ - skipped
    -- Detecting CXX compile features
    -- Detecting CXX compile features - done
    -- Conan: Adjusting default RPATHs Conan policies
    -- Conan: Adjusting language standard
    -- Conan: Compiler GCC>=5, checking major version 5
    -- Conan: Checking correct version: 5
    -- Conan: C++ stdlib: libstdc++11
    -- Conan: Using autogenerated Findpybind11.cmake
    -- Found pybind11: 2.6.0 (found version "2.6.0") 
    -- Found PythonInterp: /opt/pyenv/versions/3.7.13/bin/python3.7 (found version "3.7.13") 
    -- Found PythonLibs: /opt/pyenv/versions/3.7.13/lib/libpython3.7m.so
    -- Performing Test HAS_FLTO
    -- Performing Test HAS_FLTO - Success
    -- Configuring done
    -- Generating done
    -- Build files have been written to: /home/conan/w/prod/BuildSingleReference/conan-center-index/recipes/pybind11/all/test_package/build/2da8b83836e27aacd43916b404a0e9d98f21239f
    
    ----Running------
    > cmake --build '/home/conan/w/prod/BuildSingleReference/conan-center-index/recipes/pybind11/all/test_package/build/2da8b83836e27aacd43916b404a0e9d98f21239f' '--' '-j3'
    -----------------
    Scanning dependencies of target test_package
    [ 50%] Building CXX object CMakeFiles/test_package.dir/test_package.cpp.o
    [100%] Linking CXX shared module lib/test_package.cpython-37m-x86_64-linux-gnu.so
    [100%] Built target test_package
    pybind11/2.6.0 (test package): Running test()
    
    ----Running------
    > /opt/pyenv/versions/3.7.13/bin/python3.7 /home/conan/w/prod/BuildSingleReference/conan-center-index/recipes/pybind11/all/test_package/test.py
    -----------------
    Adding 2 + 3 = 5
    Message: 'Hello from the C++ world!'
    CMake Warning:
      Manually-specified variables were not used by the project:
    
        CMAKE_EXPORT_NO_PACKAGE_REGISTRY
        CMAKE_INSTALL_BINDIR
        CMAKE_INSTALL_DATAROOTDIR
        CMAKE_INSTALL_INCLUDEDIR
        CMAKE_INSTALL_LIBDIR
        CMAKE_INSTALL_LIBEXECDIR
        CMAKE_INSTALL_OLDINCLUDEDIR
        CMAKE_INSTALL_SBINDIR
        CONAN_EXPORTED
        CONAN_IN_LOCAL_CACHE
    
    
    pybind11/2.6.0 (test package): WARN: !!! INTERPRETER: sys.executable: /opt/pyenv/versions/3.7.13/bin/python3.7
    pybind11/2.6.0 (test package): WARN: !!! INTERPRETER: sys.executable: /opt/pyenv/versions/3.7.13/bin/python3.7
    
  • pybind11/2.4.3@:
    Didn't run or was cancelled before finishing

  • pybind11/2.5.0@:
    Didn't run or was cancelled before finishing

  • pybind11/2.6.1@:
    Didn't run or was cancelled before finishing

  • pybind11/2.7.0@:
    Didn't run or was cancelled before finishing

  • pybind11/2.7.1@:
    Didn't run or was cancelled before finishing

  • pybind11/2.6.2@:
    Didn't run or was cancelled before finishing

  • pybind11/2.8.1@:
    Didn't run or was cancelled before finishing


Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.

@stale
Copy link

stale bot commented Jun 18, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@Ahajha
Copy link
Contributor

Ahajha commented Jul 30, 2022

I just pulled this locally, and the fix to the CI is simple, in test_cmakedeps/conanfile.py, the line

from conan.tools.cross_building import cross_building

should be updated to

from conan.tools.build.cross_building import cross_building

@stale
Copy link

stale bot commented Sep 4, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 4, 2022
@ghost ghost mentioned this pull request Oct 3, 2022
4 tasks
@stale
Copy link

stale bot commented Oct 8, 2022

This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions.

@stale stale bot closed this Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants