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.

Sign up for GitHub

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

Jump to bottom

Import https://github.com/pybind/pybind11_protobuf/pull/73 #108

Closed
wants to merge 0 commits into from
Closed

Import https://github.com/pybind/pybind11_protobuf/pull/73 #108

wants to merge 0 commits into from

Conversation

copybara-service[bot]
Copy link

Import #73

Manual import. The Google-internal repo is the source of truth for pybind11_protobuf. Sorry we didn't get to automating imports from GitHub PRs.

@rwgk rwgk mentioned this pull request Feb 28, 2023
Closed
3 tasks
rwgk
rwgk reviewed Mar 1, 2023
View reviewed changes
Copy link
Contributor

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

These are comments copy-pasted (with slight edits) from an internal review by @mkruskal-google

Sorry it's a bit awkward with me in the middle (knowing very little about cmake). I hope that's ok.

CMakeLists.txt Outdated
set(CMAKE_CXX_EXTENSIONS OFF)

set(CMAKE_CXX_FLAGS
"-fPIC"
Copy link
Contributor

Choose a reason for hiding this comment

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

CMAKE_POSITION_INDEPENDENT_CODE is a more portable way to do this

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated in a3e1bff

CMakeLists.txt Outdated

option(BUILD_pybind11 "Build the pybind11 dependency Library" ON)
message(STATUS "Python: Build pybind11: ${BUILD_pybind11}")

Copy link
Contributor

Choose a reason for hiding this comment

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

This is kindof an anti-pattern for cmake. The typical approach is to assume your callers have already installed the necessary dependencies. by default you should use find_package assuming the dependencies have been installed, and fall back to option only if that fails.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated in 44a9c21.

  • Removed option to build dependencies.
  • Moved dependency settings to top-level CMakeLists.txt
  • Try find_package first then FetchContent if package not found.

CMakeLists.txt Outdated
Comment on lines 42 to 50
include(CMakeFindDependencyMacro)
include(FindProtobuf)
if(NOT protobuf_FOUND
AND NOT PROTOBUF_FOUND
AND NOT TARGET protobuf::libprotobuf)
if(BUILD_protobuf)
find_dependency(protobuf CONFIG REQUIRED)
endif()
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these necessary? You've already either used find_package or FetchContent_Declare

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed in e388b89

pybind11_protobuf/tests/CMakeLists.txt Outdated
# ============================================================================
# tests
# ============================================================================

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look like it's ever getting invoked

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes - the cmake build for tests is not yet complete and integrated. Can look to add if required for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just omit this file in this PR? Would we lose something if we did that?

Copy link
Contributor

@srmainwaring srmainwaring Mar 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes can remove. No we won't lose anything. Removed in 6159ace.

@srmainwaring
Copy link
Contributor

Sorry it's a bit awkward with me in the middle (knowing very little about cmake). I hope that's ok.

No prob. I'm not a cmake expert either and adapted the fetch content usage from another project, so happy to take on board improvements. Will update #73 with the reviewers suggestions.

@copybara-service copybara-service bot force-pushed the cl/512933788 branch 3 times, most recently from d18e856 to d47e377 Compare March 8, 2023 18:35
@copybara-service copybara-service bot closed this Mar 8, 2023
@copybara-service copybara-service bot deleted the cl/512933788 branch March 8, 2023 18:45
@srmainwaring srmainwaring mentioned this pull request Mar 9, 2023
Merged
copybara-service bot pushed a commit that referenced this pull request Mar 9, 2023
@copybara-github
Import #73 (again)
e8bde5d 
Manual import. The Google-internal repo is the source of truth for pybind11_protobuf. Sorry we didn't get to automating imports from GitHub PRs.

* Initially merged via #108
* Rolled back via #109

PiperOrigin-RevId: 515452762
@copybara-service copybara-service bot mentioned this pull request Mar 9, 2023
Closed
copybara-service bot pushed a commit that referenced this pull request Mar 10, 2023
@copybara-github
Import #73 (again)
054ce9e 
Manual import. The Google-internal repo is the source of truth for pybind11_protobuf. Sorry we didn't get to automating imports from GitHub PRs.

* Initially merged via #108
* Rolled back via #109

PiperOrigin-RevId: 515452762
copybara-service bot pushed a commit that referenced this pull request Mar 10, 2023
@copybara-github
Import #73 (again)
57422ba 
Manual import. The Google-internal repo is the source of truth for pybind11_protobuf. Sorry we didn't get to automating imports from GitHub PRs.

* Initially merged via #108
* Rolled back via #109

PiperOrigin-RevId: 515452762
copybara-service bot pushed a commit that referenced this pull request Mar 10, 2023
@copybara-github
Import #73 (again)
7dae0d4 
Manual import. The Google-internal repo is the source of truth for pybind11_protobuf. Sorry we didn't get to automating imports from GitHub PRs.

* Initially merged via #108
* Rolled back via #109

PiperOrigin-RevId: 515452762
copybara-service bot pushed a commit that referenced this pull request Mar 10, 2023
@copybara-github
Import #73 (again)
e92d086 
Manual import. The Google-internal repo is the source of truth for pybind11_protobuf. Sorry we didn't get to automating imports from GitHub PRs.

* Initially merged via #108
* Rolled back via #109

PiperOrigin-RevId: 515452762
copybara-service bot pushed a commit that referenced this pull request Mar 10, 2023
@copybara-github
Import #73 (again)
e1714f0 
Manual import. The Google-internal repo is the source of truth for pybind11_protobuf. Sorry we didn't get to automating imports from GitHub PRs.

* Initially merged via #108
* Rolled back via #109

PiperOrigin-RevId: 515452762
copybara-service bot pushed a commit that referenced this pull request Mar 10, 2023
@srmainwaring @copybara-github
Import #73 (again)
d824311 
Manual import. The Google-internal repo is the source of truth for pybind11_protobuf. Sorry we didn't get to automating imports from GitHub PRs.

* Initially merged via #108
* Rolled back via #109

PiperOrigin-RevId: 515659056
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwgk rwgk rwgk left review comments

@srmainwaring srmainwaring srmainwaring left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@srmainwaring @rwgk