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

Allow CMake to use Protobuf compiler from Conan path #4012

Merged
merged 2 commits into from
Jan 12, 2021
Merged

Allow CMake to use Protobuf compiler from Conan path #4012

merged 2 commits into from
Jan 12, 2021

Conversation

avantgardnerio
Copy link
Contributor

Specify library name and version: protobuff/all_versions

  • 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.

@CLAassistant
Copy link

CLAassistant commented Dec 28, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@conan-center-bot
Copy link
Collaborator

Sorry, the build is only launched for Early Access Program users. You can request access writing in this issue.

@uilianries
Copy link
Member

Hi @avantgardnerio !

Thank you for your PR, please sign the CLA: #4012 (comment)
And, Comment your named on #4. Otherwise, we can't approve your PR.

@avantgardnerio
Copy link
Contributor Author

Hi @avantgardnerio !

Thank you for your PR, please sign the CLA: #4012 (comment)
And, Comment your named on #4. Otherwise, we can't approve your PR.

Thanks, I added my name (Brent Gardner) to the issue linked above. I've also signed the CLA.

@justinmcbride justinmcbride mentioned this pull request Dec 30, 2020
Closed
@justinmcbride
Copy link
Contributor

For reference, the purpose of this PR is to enable the use of the conan-installed protoc compiler within the CMake protobuf_generate function. There was a comment in the recipe file as such: #FIXME: use conan binary component.

We fixed this by using the CMake function find_program to find the binary of protoc. The conan-installed protoc binary is added to the CMAKE_PREFIX_PATH as part of the conanbuildinfo.cmake. Then, that stored location is used in the custom command that gets added as part of the protobuf_generate function. If for any reason the find_program call fails to find the bianry, we default to using the generic protoc command, as has been done in the past.

@avantgardnerio
Copy link
Contributor Author

I get a 403 when I try to read the build result. Not sure why. Do I need to fix something?

@uilianries
Copy link
Member

Hi @avantgardnerio ! First, you have to be approved on EAP, which occurs weekly (on next week in your case). Happy new year!

@justinmcbride
Copy link
Contributor

Hi @uilianries . It looks like we've both been accepted to the EAP. What is the next step to allow this PR to be reviewed?

Thank you!

@github-actions github-actions bot mentioned this pull request Jan 6, 2021
Open
@conan-center-bot
Copy link
Collaborator

An unexpected error happened and has been reported. Help is on its way! 🏇

@uilianries uilianries closed this Jan 6, 2021
@uilianries uilianries reopened this Jan 6, 2021
@uilianries uilianries closed this Jan 6, 2021
@uilianries uilianries reopened this Jan 6, 2021
@conan-center-bot
Copy link
Collaborator

All green in build 4 (1541bd3f4c717485fed725cf15edffda4bb75656)! 😊

@avantgardnerio
Copy link
Contributor Author

Hi, know you folks are busy and the world is on fire, but just a gentle reminder for code review :)

@SSE4 SSE4 requested a review from uilianries January 12, 2021 16:57
Croydon
Croydon reviewed Jan 12, 2021
View reviewed changes
recipes/protobuf/all/conanfile.py
@@ -109,8 +119,8 @@ def _patch_sources(self):
"""COMMAND protobuf::protoc
ARGS --${protobuf_generate_LANGUAGE}_out ${_dll_export_decl}${protobuf_generate_PROTOC_OUT_DIR} ${_plugin} ${_protobuf_include_path} ${_abs_file}
DEPENDS ${_abs_file} protobuf::protoc""",
"""COMMAND "${CMAKE_COMMAND}" #FIXME: use conan binary component
Copy link
Contributor

Choose a reason for hiding this comment

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

Too late now. But you should not have removed this comment. It is a reminder to eventually use an upcoming Conan feature which makes the usage of executables easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the note! Only from my curiosity, can you point towards documentation on that upcoming feature?

Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a start:

conan-io/conan#7240

conan-io/conan#7324

@conan-center-bot conan-center-bot merged commit 4116c93 into conan-io:master Jan 12, 2021
@justinmcbride justinmcbride deleted the find_protoc branch January 12, 2021 21:29
@SpaceIm SpaceIm mentioned this pull request Mar 4, 2021
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmcbride justinmcbride justinmcbride left review comments

@Croydon Croydon Croydon left review comments

@SpaceIm SpaceIm SpaceIm approved these changes

@prince-chrismc prince-chrismc prince-chrismc approved these changes

@SSE4 SSE4 SSE4 approved these changes

@uilianries uilianries Awaiting requested review from uilianries

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

Successfully merging this pull request may close these issues.
9 participants
@avantgardnerio @CLAassistant @conan-center-bot @uilianries @justinmcbride @SSE4 @Croydon @prince-chrismc @SpaceIm