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

protobuf: add protoc option #1179

Merged
merged 12 commits into from
Jun 24, 2020
Merged

protobuf: add protoc option #1179

merged 12 commits into from
Jun 24, 2020

Conversation

madebr
Copy link
Contributor

@madebr madebr commented Mar 24, 2020

Specify library name and version: protobuf/3.9.1.0

  • This recipe allows protobuf to be used as a build requirement.
  • Add option to build protoc.

fixes #65 #1956

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

This was referenced Mar 24, 2020
@conan-center-bot
Copy link
Collaborator

Some configurations of 'protobuf/3.9.1.0' failed in build 1 (e594974848d15c887147cf2644e65585413745d6):

  • Macos x86_64, Debug, apple-clang 9.1, libc++ . Options: protobuf:shared-True
  • Access to all the logs
  • Windows x86_64, Debug, Visual Studio 14, MTd. Options: protobuf:shared-False
    • Hooks errors detected:
      • [HOOK - conan-center.py] post_package_info(): ERROR: [CMAKE FILE NOT IN BUILD FOLDERS (KB-H019)] The *.cmake files have to be placed in a folder declared as cpp_info.builddirs. Currently folders declared: ['lib\\cmake\\protobuf'] (https://github.com/conan-io/conan-center-index/wiki/Error-Knowledge-Base#KB-H019)
      • [HOOK - conan-center.py] post_package_info(): ERROR: [CMAKE FILE NOT IN BUILD FOLDERS (KB-H019)] Found files:
    • Log download
    • Log error Download
    • Profile
  • Access to all the logs

@madebr
Copy link
Contributor Author

madebr commented Mar 25, 2020

    * `[HOOK - conan-center.py] post_package_info(): ERROR: [CMAKE FILE NOT IN BUILD FOLDERS (KB-H019)] The *.cmake files have to be placed in a folder declared as `cpp_info.builddirs`. Currently folders declared: ['lib\\cmake\\protobuf'] (https://github.com/conan-io/conan-center-index/wiki/Error-Knowledge-Base#KB-H019)`
    * `[HOOK - conan-center.py] post_package_info(): ERROR: [CMAKE FILE NOT IN BUILD FOLDERS (KB-H019)] Found files:`

@uilianries On Windows, the hook should normalize windows paths to cmake paths ("\\" -> "/")

@conan-center-bot
Copy link
Collaborator

Some configurations of 'protobuf/3.9.1.0' failed in build 2 (7b7b66d460066f522c12abc622f9216d82c130ad):

  • Macos x86_64, Debug, apple-clang 9.1, libc++ . Options: protobuf:shared-True
  • Access to all the logs
  • Windows x86_64, Debug, Visual Studio 14, MTd. Options: protobuf:shared-False
    • Hooks errors detected:
      • [HOOK - conan-center.py] post_package_info(): ERROR: [CMAKE FILE NOT IN BUILD FOLDERS (KB-H019)] The *.cmake files have to be placed in a folder declared as cpp_info.builddirs. Currently folders declared: ['lib\\cmake\\protobuf'] (https://github.com/conan-io/conan-center-index/wiki/Error-Knowledge-Base#KB-H019)
      • [HOOK - conan-center.py] post_package_info(): ERROR: [CMAKE FILE NOT IN BUILD FOLDERS (KB-H019)] Found files:
    • Log download
    • Log error Download
    • Profile
  • Access to all the logs

@madebr
Copy link
Contributor Author

madebr commented Mar 25, 2020

@jgsogo This recipe provides some libraries (libprotobuf/libprotoc/..) and an executable (protoc).
So this package can be used as a build requirement and a ordinary requirement.
In package_info I add the bin folder to the PATH environment variable.

Won't this cause problems when cross building (and adding it in both requirements and build_requirements)? Because PATH might be appended by a directory, containing executables for the host architecture..

@conan-center-bot
Copy link
Collaborator

Some configurations of 'protobuf/3.9.1.0' failed in build 3 (8dcff5b4b209f241499a32fdc1cecb43c4d8338c):

  • Linux x86_64, Release, gcc 5, libstdc++ . Options: protobuf:shared-True
    • Hooks errors detected:
      • [HOOK - conan-center.py] pre_export(): ERROR: [TEST PACKAGE - RUN ENVIRONMENT (KB-H029)] The 'RunEnvironment()' build helper is no longer needed. It has been integrated into the self.run(..., run_environment=True) (https://github.com/conan-io/conan-center-index/wiki/Error-Knowledge-Base#KB-H029)
    • Log download
    • Log error Download
    • Profile
  • Access to all the logs

@madebr
Copy link
Contributor Author

madebr commented Mar 25, 2020

    * `[HOOK - conan-center.py] pre_export(): ERROR: [TEST PACKAGE - RUN ENVIRONMENT (KB-H029)] The 'RunEnvironment()' build helper is no longer needed. It has been integrated into the self.run(..., run_environment=True) (https://github.com/conan-io/conan-center-index/wiki/Error-Knowledge-Base#KB-H029) `

@uilianries

The RunEnvironment helper is needed in the build() step of the cmake script.
So either:

  • the cmake helper needs to be improved (as does AutoToolsBuildEnvironment and Meson); or
  • the hook is too stringent.

This is the same problem as @ericLemanissier was seeing while packaging libsndfile.
See #1103 (comment)

@Croydon
Copy link
Contributor

Croydon commented Mar 25, 2020

Maybe the hook should check if RunEnvironment is used inside of test() because there should be no reasons why it should be used there? 🤔

@jgsogo
Copy link
Contributor

jgsogo commented Mar 25, 2020

@jgsogo This recipe provides some libraries (libprotobuf/libprotoc/..) and an executable (protoc).
So this package can be used as a build requirement and a ordinary requirement.
In package_info I add the bin folder to the PATH environment variable.

Won't this cause problems when cross building (and adding it in both requirements and build_requirements)? Because PATH might be appended by a directory, containing executables for the host architecture..

Have a look to the implementation of the feature in the sources: https://github.com/jgsogo/conan/blob/4b977e322c308341003007b5641aadc3df36383b/conans/client/installer.py#L503

  • Recipes in the host context (requirements and build_requires forced to host) propagate only the cpp_info to deps_cpp_info

    conan_file.deps_cpp_info.update(n.conanfile.cpp_info, n.ref.name)
  • Regular build requirements (those will be in the build context) propagate the env_info and the cpp_info to the deps_env_info:

    env_info = EnvInfo()
    env_info._values_ = n.conanfile.env_info._values_.copy()
    # Add cpp_info.bin_paths/lib_paths to env_info (it is needed for runtime)
    env_info.DYLD_LIBRARY_PATH.extend(n.conanfile.cpp_info.lib_paths)
    env_info.DYLD_LIBRARY_PATH.extend(n.conanfile.cpp_info.framework_paths)
    env_info.LD_LIBRARY_PATH.extend(n.conanfile.cpp_info.lib_paths)
    env_info.PATH.extend(n.conanfile.cpp_info.bin_paths)
    conan_file.deps_env_info.update(env_info, n.ref.name)

This is not the same as it was being done before. This is not breaking because using two profiles is an opt-in, but we know this can break some recipes or workflows, we really wan't to know how people is using the propagation in the projects and try to improve it for Conan 2.0. We might change it if needed based on your feedback.

@madebr
Copy link
Contributor Author

madebr commented Mar 25, 2020

Have a look to the implementation of the feature in the sources: https://github.com/jgsogo/conan/blob/4b977e322c308341003007b5641aadc3df36383b/conans/client/installer.py#L503

  ```python
  env_info = EnvInfo()
  env_info._values_ = n.conanfile.env_info._values_.copy()
  # Add cpp_info.bin_paths/lib_paths to env_info (it is needed for runtime)
  env_info.DYLD_LIBRARY_PATH.extend(n.conanfile.cpp_info.lib_paths)
  env_info.DYLD_LIBRARY_PATH.extend(n.conanfile.cpp_info.framework_paths)
  env_info.LD_LIBRARY_PATH.extend(n.conanfile.cpp_info.lib_paths)
  env_info.PATH.extend(n.conanfile.cpp_info.bin_paths)
  conan_file.deps_env_info.update(env_info, n.ref.name)
  ```

@jgsogo
Thanks for the reference code.
Adding these extra fields to cpp_infoseems like a good idea.
Can a recipe detect in its package_info in what context it is used? (build/host context)

@uilianries
Copy link
Member

The RunEnvironment helper is needed in the build() step of the cmake script.
So either:

* the cmake helper needs to be improved (as does `AutoToolsBuildEnvironment` and `Meson`); or

* the hook is too stringent.

This is the same problem as @ericLemanissier was seeing while packaging libsndfile.
See #1103 (comment)

I'll push a new fix for hooks, thanks for reporting.

@jgsogo
Copy link
Contributor

jgsogo commented Mar 25, 2020

@madebr

Adding these extra fields to cpp_info seems like a good idea.

We are not adding extra to cpp_info, with the two-profiles approach we are propagating (for the build requires in the build context) the cpp_info and the env_info to the deps_env_info, and before entering the build() method of the consumers all those variables will populate the environment: DYLD_..., LD_..., PATH_....

But, the deps_cpp_info in the consumers will contain less information.

Can a recipe detect in its package_info in what context it is used? (build/host context)

Right now (v1.24) it can't. This is something we need to investigate and implement for the next releases.

@conan-center-bot
Copy link
Collaborator

Some configurations of 'protobuf/3.9.1' failed in build 4 (404bde744641c80f9002bd8ff2ed836fa343130f):

  • Linux x86_64, Release, gcc 5, libstdc++ . Options: protobuf:shared-True
    • Hooks errors detected:
      • [HOOK - conan-center.py] pre_export(): ERROR: [TEST PACKAGE - RUN ENVIRONMENT (KB-H029)] The 'RunEnvironment()' build helper is no longer needed. It has been integrated into the self.run(..., run_environment=True) (https://github.com/conan-io/conan-center-index/wiki/Error-Knowledge-Base#KB-H029)
    • Log download
    • Log error Download
    • Profile
  • Access to all the logs

@madebr
Copy link
Contributor Author

madebr commented Apr 27, 2020

@uilianries Please re-review.
I've added protoc support to protobuf/3.11.4 as well and force rebased on top of current master.

@conan-center-bot
Copy link
Collaborator

Some configurations of 'protobuf/3.9.1' failed in build 5 (246a00050b410e989b61f4956a4a2d91244821d1):

  • Linux x86_64, Release, gcc 5, libstdc++ . Options: protobuf:shared-True
    • Hooks errors detected:
      • [HOOK - conan-center.py] pre_export(): ERROR: [TEST PACKAGE - RUN ENVIRONMENT (KB-H029)] The 'RunEnvironment()' build helper is no longer needed. It has been integrated into the self.run(..., run_environment=True) (https://github.com/conan-io/conan-center-index/wiki/Error-Knowledge-Base#KB-H029)
    • Log download
    • Log error Download
    • Profile
  • Access to all the logs

@conan-center-bot
Copy link
Collaborator

Some configurations of 'protobuf/3.9.1' failed in build 6 (246a00050b410e989b61f4956a4a2d91244821d1):

@jgsogo
Copy link
Contributor

jgsogo commented Apr 29, 2020

For this add_custom_command we need to modify the .cmake file, it is the same when running tests using cmake --build ... --target test (conan-io/examples#17 (review)). From outside CMake (where Conan lives) we cannot add anything to the environment, it won't be used in the subprocess.

@madebr
Copy link
Contributor Author

madebr commented Apr 29, 2020

For this add_custom_command we need to modify the .cmake file, it is the same when running tests using cmake --build ... --target test (conan-io/examples#17 (review)). From outside CMake (where Conan lives) we cannot add anything to the environment, it won't be used in the subprocess.

Ok, I'll patch the cmake script.

But the RunEnvironment will be needed anyhow for linux (LD_LIBRARY_PATH)

The original code used the cmake imported target protobuf::protoc.
If the new conan components feature would be able to generate this executable target, it can also be used.
Would this be another way to solve this?

@madebr
Copy link
Contributor Author

madebr commented Apr 29, 2020

For the cmake_find_package_multi generator, I think we need to have the INTERFACE_LINK_DIRECTORIES property of the libraries populated.
Then we can use generator expressions to extract the property of the correct build type.

Now I would need to add ${Protobuf_LIB_DIRS_RELEASE}:${Protobuf_LIB_DIRS_DEBUG}:${Protobuf_LIB_DIRS_RELWITHDEBINFO}:${Protobuf_LIB_DIRS_MINSIZEREL}.
And then, this doesn't guarantee success if a user added an extra build type to its settings.yml.

@conan-center-bot
Copy link
Collaborator

All green in build 15 (e688f64750ec13f0553f95b3d39b2d4640b94a36)! 😊

@conan-center-bot
Copy link
Collaborator

All green in build 16 (e688f64750ec13f0553f95b3d39b2d4640b94a36)! 😊

@Croydon
Copy link
Contributor

Croydon commented Jun 23, 2020

I don't understand if there are any issues left, but if they aren't breaking and can't be fixed quickly I would suggest to create an issue to fix it later and finally merge this

This becomes quite a blocker IMO

@uilianries
Copy link
Member

@Croydon Agreed, Protobuf is one of most required packages

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

Agree, we can move this one forward and wait for issues.

@madebr
Copy link
Contributor Author

madebr commented Jun 24, 2020

Agree, we can move this one forward and wait for issues.

I love your optimism 😄

@jgsogo
Copy link
Contributor

jgsogo commented Jun 24, 2020

Agree, we can move this one forward and wait for issues.

I love your optimism 😄

🤣 well, I didn't mean there will be issues.... but, well, we all know there will be. I'll spend some days now experiencing with the cross-building feature, env propagation, settings,... so I'll have time to try this recipe and open the issues myself 😄

homer-seto

@SSE4 SSE4 merged commit 252ad9a into conan-io:master Jun 24, 2020
@madebr madebr deleted the protobuf_protoc branch June 24, 2020 14:21
@Croydon
Copy link
Contributor

Croydon commented Jun 24, 2020

Thanks everyone! 🥳

@madebr madebr mentioned this pull request Jul 5, 2020
4 tasks
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

7 participants