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

CMake: add separate export for plugin targets #22498

Conversation

herbrechtsmeier
Copy link
Contributor

Add a separate export for the plugin targets to separate binaries and libraries into their own target export files. Replace the cross binary plugin targets with native binary targets during cross
compile to allow the usage of the plugin targets for native and cross compile builds.

@markdroth

Add a separate export for the plugin targets to separate binaries
and libraries into their own target export files. Replace the cross
binary plugin targets with native binary targets during cross
compile to allow the usage of the plugin targets for native and
cross compile builds.
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 27, 2020

CLA Check
The committers are authorized under a signed CLA.

@herbrechtsmeier
Copy link
Contributor Author

I signed it

@markdroth markdroth added area/build kind/enhancement lang/core release notes: no Indicates if PR should not be in release notes labels Mar 27, 2020
@markdroth
Copy link
Member

Looks like the CLA bot is still failing. If you're having problems getting your signature to register, please file a support ticket using the link shown above. Thanks!

@jtattermusch
Copy link
Contributor

I'm not sure I understand how this is useful and what's the use case. Can you give an example?

@jtattermusch jtattermusch self-assigned this Mar 30, 2020
@@ -279,38 +279,38 @@ endfunction()
# These options allow users to enable or disable the building of the various
# protoc plugins. For example, running CMake with
# -DgRPC_BUILD_GRPC_CSHARP_PLUGIN=OFF will disable building the C# plugin.
set(_gRPC_PLUGIN_LIST)
set(gRPC_PLUGIN_LIST)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Names of variables used internally should start with an underscore. Because this variable is used inside the gRPCConfig.cmake.in and the name use the project namespace I have remove it.

@herbrechtsmeier
Copy link
Contributor Author

During cross compile you have to combine the cross compiled libraries with the native binaries. At the moment every downstream project have to find the program by itself:

if(CMAKE_CROSSCOMPILING)
    find_program(_GRPC_CPP_PLUGIN_EXECUTABLE grpc_cpp_plugin)
  else()
    set(_GRPC_CPP_PLUGIN_EXECUTABLE $<TARGET_FILE:grpc_cpp_plugin>)
  endif()

After this patch you could always use $<TARGET_FILE:grpc_cpp_plugin>.

@herbrechtsmeier
Copy link
Contributor Author

@markdroth According to the contributing guidelines we have sign the Contributor License Agreement but this isn't respected by the EasyCLA.

@herbrechtsmeier
Copy link
Contributor Author

@jtattermusch At the moment the gRPC CMake support doesn't work in Yocto because Yocto doesn't install the target binaries into the target sysroot and this is required by the gRPCConfig.cmake. The current solution is to remove this file from the target sysroot but this breaks other projects which use the library targets because CMake finds the gRPCConfig.cmake in the native sysroot and thereby links against the native libraries instead of the target libraries.

@herbrechtsmeier
Copy link
Contributor Author

We signed the EasyCLA in addition to the CLA from the CONTRIBUTING.md.

@stale
Copy link

stale bot commented May 6, 2020

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 30 days. It will be closed automatically if no further update occurs in 7 day. Thank you for your contributions!

@herbrechtsmeier
Copy link
Contributor Author

What can I do to get this reviewed?

@herbrechtsmeier
Copy link
Contributor Author

@jtattermusch Yes. You have to apply this changes and remove the SYSROOT_DIRS_BLACKLIST_append_class-target hack.

@mmurrian
Copy link

@jtattermusch Yes. You have to apply this changes and remove the SYSROOT_DIRS_BLACKLIST_append_class-target hack.

Thanks for the quick response. I deleted my post because I realized I still had the hack you mentioned in place.

This has been on my back-burner for a while. I circled back around and you already fixed it for me!

@herbrechtsmeier
Copy link
Contributor Author

@jtattermusch Yes. You have to apply this changes and remove the SYSROOT_DIRS_BLACKLIST_append_class-target hack.

Thanks for the quick response. I deleted my post because I realized I still had the hack you mentioned in place.

This has been on my back-burner for a while. I circled back around and you already fixed it for me!

The hack isn't from me and it doesn't work correct. You couldn't use the grpc:: targets for linking because they point to the native libraries.

Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

Based on my review, it seems that this PR doesn't actually fix anything, it just adds some syntactic sugar.
I have some doubts whether what the PR tries to do is correct and because it's not a bugfix, i'll close.
Sorry.

@@ -10,3 +10,16 @@ list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_LIST_DIR}/modules)

# Targets
include(${CMAKE_CURRENT_LIST_DIR}/gRPCTargets.cmake)
if(CMAKE_CROSSCOMPILING)
Copy link
Contributor

Choose a reason for hiding this comment

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

this really seems just as a convenience, I don't think it enables anything that wasn't possible before.
As such, I don't think we want this improvement due to its complexity.

It looks like all the other changes have been added just to be able to add this snippet, so IMHO also the other changes from this PR changes aren't really needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this really seems just as a convenience, I don't think it enables anything that wasn't possible before.

At the moment every project inclusive the grpc examples have to check the variable CMAKE_CROSSCOMPILING to find the plugin grpc_cpp_plugin or to use the target grpc::grpc_cpp_plugin:

if(CMAKE_CROSSCOMPILING)
  find_program(_GRPC_CPP_PLUGIN_EXECUTABLE grpc_cpp_plugin)
else()
  set(_GRPC_CPP_PLUGIN_EXECUTABLE $<TARGET_FILE:grpc_cpp_plugin>)
endif()

After the patch every project could simple use the target.

This

As such, I don't think we want this improvement due to its complexity.

This pull request moves the complexity from every project into the source project and thereby remove code duplication.

It looks like all the other changes have been added just to be able to add this snippet, so IMHO also the other changes from this PR changes aren't really needed.

Which changes do you mean?

The PR segregate the host tools from the libraries and search for the host tools during a cross compile.

Please check your examples. At the moment you force everybody to copy code into its CMakeLists.txt.

@mmurrian
Copy link

mmurrian commented Jun 22, 2020

One such "convenience" is the ability to compile and use gRPC in Yocto.

Cross-compiling with gRPC is already unique given the need for both "native" and "target" phases of compilation.

Other libraries (say, Qt) go so far as to have special functionality (AUTOMOC) added to cmake in support of code generation.

gRPC can't even accommodate one additional build target?

@jtattermusch
Copy link
Contributor

One such "convenience" is the ability to compile and use gRPC in Yocto.

Cross-compiling with gRPC is already unique given the need for both "native" and "target" phases of compilation.

Other libraries (say, Qt) go so far as to have special functionality (AUTOMOC) added to cmake in support of code generation.

gRPC can't even accommodate one additional build target?

As said before, Yocto is not a platform we officially support. That said, given the nature of the proposed change I think there should still be a way to build with gRPC under yocto (maybe not in the most straighforward way, but none of the changes you're proposing seem to be strictly necessary).

gRPC can accomodate one additional build target, but the need for such target needs to be clearly established and there needs to be a good way of testing this - otherwise increasing the complexity of the build system (which is already pretty complex) is harmful.

@herbrechtsmeier
Copy link
Contributor Author

One such "convenience" is the ability to compile and use gRPC in Yocto.
Cross-compiling with gRPC is already unique given the need for both "native" and "target" phases of compilation.
Other libraries (say, Qt) go so far as to have special functionality (AUTOMOC) added to cmake in support of code generation.
gRPC can't even accommodate one additional build target?

As said before, Yocto is not a platform we officially support.

You don't support cross compiling. For example the run_distrib_test_raspberry_pi.sh only works because you install the native grpc tools into your root system.

That said, given the nature of the proposed change

What do you mean by this?

I think there should still be a way to build with gRPC under yocto (maybe not in the most straighforward way, but none of the changes you're proposing seem to be strictly necessary).

The problem is that every project need to find the plugin during cross compile. Instead of using the target grpc::grpc_cpp_plugin every project need the code from above.

gRPC can accomodate one additional build target, but the need for such target needs to be clearly established and there needs to be a good way of testing this

I could add a test if this is the problem. If we remove the above code from the example we could use them to test the changes.

  • otherwise increasing the complexity of the build system (which is already pretty complex) is harmful.

It moves the complexity from the downstream projects to the root project.

@herbrechtsmeier
Copy link
Contributor Author

The current solution with the find_program only supports cross compiling if the native program is installed inside the base system. It doesn't work if you have a sysroot for the native tools and cross libraries. CMake only support one list of roots (CMAKE_FIND_ROOT_PATH). A solution is to remove all binaries from the cross sysroot and list the native sysroot after the cross sysroot inside the variable CMAKE_FIND_ROOT_PATH. But this breaks the gRPCConfig.cmake because gRPC exports binaries and the gRPCTargets.cmake checks if all targets exist.

@jtattermusch What is the grpc solution for cross compiling with a native sysroot?

@clementperon
Copy link
Contributor

@jtattermusch I get the same issue on my side.

My rootfs host is different from the rootfs target.

This make an error with the actual gRPCTargets.cmake because the cmake can't found the grpc_cpp_plugin in the "rootfs_target" because it's in the "rootfs_host".

IMO this is a proper solution to split the Tools/Plugins required for gRPC compilation vs the libs required for execution.

Regards,
Clement

@clementperon
Copy link
Contributor

clementperon commented Apr 23, 2021

@veblush @gnossen @ashithasantosh,

At the moment the gRPC CMake doesn't support proper Cross compilation, what is done actually is to install both cross compiled libraries with the native binaries. Which is a big HACK! Merging different architecture in the same Rootfs doesn't work! If you want to have both host gRPC compiler and target gRPC compiler you can't!

I test this patch with host compilation (tools and bin are on x86_64), cross compilation (tools on x86_64 target on ARM64) and generating an SDK then cross-compiling gRPC files from host to a target (1.36.0).

All of them works properly, and splitting the gRPC targets from the gRPC lib/bin is the only proper way to make this cross compilation works properly.

Can this be review again, Please?

Thanks,
Clement

herbrechtsmeier added a commit to weidmueller/grpc that referenced this pull request Apr 6, 2022
Add a separate CMake export for the plugin targets to separate binaries
and libraries into their own CMake target export files. Skip the cross
compiled binary plugin targets during cross compile because they are not
usable and not always available.

The Yocto build system doesn't install cross compiled binaries into the
target sysroot. This makes the CMake gRPC config useless as it checks
the existent of binaries and fails without the binaries.

This is a strip down version of grpc#22498 and related to grpc#26148 and grpc#26857.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
herbrechtsmeier added a commit to weidmueller/grpc that referenced this pull request Apr 8, 2022
Add a separate CMake export for the plugin targets to separate binaries
and libraries into their own CMake target export files. Skip the cross
compiled binary plugin targets during cross compile because they are not
usable and not always available.

The Yocto build system doesn't install cross compiled binaries into the
target sysroot. This makes the CMake gRPC config useless as it checks
the existent of binaries and fails without the binaries.

This is a strip down version of grpc#22498 and related to grpc#26148 and grpc#26857.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
clementperon pushed a commit to clementperon/grpc that referenced this pull request Nov 2, 2022
Add a separate CMake export for the plugin targets to separate binaries
and libraries into their own CMake target export files. Skip the cross
compiled binary plugin targets during cross compile because they are not
usable and not always available.

The Yocto build system doesn't install cross compiled binaries into the
target sysroot. This makes the CMake gRPC config useless as it checks
the existent of binaries and fails without the binaries.

This is a strip down version of grpc#22498 and related to grpc#26148 and grpc#26857.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
veblush pushed a commit that referenced this pull request Nov 15, 2022
* cmake: add separate export for plugin targets

Add a separate CMake export for the plugin targets to separate binaries
and libraries into their own CMake target export files. Skip the cross
compiled binary plugin targets during cross compile because they are not
usable and not always available.

The Yocto build system doesn't install cross compiled binaries into the
target sysroot. This makes the CMake gRPC config useless as it checks
the existent of binaries and fails without the binaries.

This is a strip down version of #22498 and related to #26148 and #26857.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
Signed-off-by: Clément Péron <peron.clem@gmail.com>

* regenerate cmake file

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
Co-authored-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build kind/enhancement lang/core release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants