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

Make cross-compiling great again #11476

Closed
wants to merge 1 commit into from

Conversation

jynnantonix
Copy link

Fixes #11475

@grpc-testing
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

10 similar comments
@grpc-testing
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-testing
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-testing
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-testing
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-testing
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-testing
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

2 similar comments
@grpc-testing
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@grpc-testing
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@nicolasnoble
Copy link
Member

This is okay to test.

@nicolasnoble
Copy link
Member

Several comments. First you would need to update the template file so it regenerates properly when someone changes the list of files in the project.

Second, most of the change isn't actually really needed. I can get behind the AR one, but for the rest, you can actually override these values from make's command line. At best you should use ?= instead of = so they become overridable from the environment variables too.

@jynnantonix
Copy link
Author

Second, most of the change isn't actually really needed. I can get behind the AR one, but for the rest, you can actually override these values from make's command line. At best you should use ?= instead of = so they become overridable from the environment variables too.

That was my first approach (setting them on the make command line so that they cannot get set by the Makefile) but it doesn't really work because there are some non arch-specific includes and defines that are necessary to actually compile the plugins which don't get added.

Ideally there would be some way to just turn off building the plugins and use the ones already installed on the build machine. Then you wouldn't have to treat cross-compiling in any special way at all.

@nicolasnoble
Copy link
Member

nicolasnoble commented Jun 14, 2017 via email

@jynnantonix
Copy link
Author

However I am confused about the rest of the comment. Are you suggesting
using the system's plug-ins to build the tests? Because aside from tests, I
am not aware of the reason you'd need the plug-ins to build anything.

Even if I remove plugins from the all target, something still attempts to use them. Look for $(PROTOC_PLUGINS) starting from here. I don't think I'm building any tests since the way I invoke the build is to just run make with no target.

It appears that PROTOC_PLUGINS automatically gets set to the ones from the grpc tree if $(HAS_SYSTEM_PROTOBUF) is true (see here). This also seems like a bug. I would think that if $(HAS_SYSTEM_PROTOBUF) is true, then we should check to see if the protoc plugins are also installed and use those instead of building them out of the tree.

So to clarify my comment what I'd like is for the Makefile to not attempt to handle multiple toolchains at all and instead use the same toolchain to build everything while providing knobs to turn off parts of the build (like the plugins). I guess I'm a bit frustrated because it's clear that cross-compiling isn't something that gets tested on every commit (otherwise the AR/HOST_AR issue would have been caught earlier) so it seems better to just drop the support rather than let it bit-rot until someone finds a problem.

@grpc-testing
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@nicolasnoble
Copy link
Member

Aah, the health checking now uses the plugins, indeed. Okay, that's a good reason to have a cause for concern when doing cross compilation. Originally, one could ask to only build the library and no host target would've been touch, that's not the case anymore, which indeed increases the complexity.

However, I'm a bit weary of the "using the plugin from the system" part, because this isn't something that's supposed to be stable and backward-compatible. The code generated by the plugin isn't subject to semantic versioning, and there has been cases in the past where code generated by a previous version of the plugin wouldn't be compatible anymore with the latest version of the library, from a pure compilation perspective. So I'm not sure doing what you're suggesting is a good idea, since it would become impossible to upgrade grpc with a simple overwrite. Basically you'd need to uninstall the former version before even compiling, which seems odd.

On another subject, testing cross compilation on every commit isn't something we're really able to do at the moment for two big reasons:

  1. There are lots of potential cross compilation targets. Be it ARM on GNU Linux, Android, ChromeOS, WebOS, ... or even Intel Alpine Linux. We'd need to find a narrow list of targets we're willing to support first, then expand.
  2. The maintenance cost for this isn't trivial. Actually going in and say we're actively going to fully support cross compilation for all times now and forever isn't a decision we should take on lightly.

Alternatively, finding interested active maintainers for some items in 1) would alleviate 2).

@jynnantonix
Copy link
Author

Re: cross-compile being a huge commitment. I fully understand, cross-compiling is a hairy beast. I guess what is was trying to get at is that (from a chrome os perspective) cross-compiling should either be fully-supported or not supported at all. The reason is that we have a lot of tools (like the previously mentioned cross-compile aware wrappers around pkg-config) to deal with packages that don't support cross-compiling. It's the packages that provide halfway (or best-effort) support that cause the most headaches for us because they don't integrate nicely with any of our existing infrastructure.

Re: building the plugins. What I'm suggesting is to provide the following instructions for cross-compiling:

  • Build and install grpc natively for the build machine and then
  • Use the plugins from the previous step when cross-compiling

Rather than making it a default, maybe we can gate it behind the GRPC_CROSS_COMPILE option so that nothing changes for the average user. If you're still worried about backwards compatibility then I won't try to make this change and I'll just stick to cleaning up the host flags and applying those changes to the template.

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@thelinuxfoundation
Copy link

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
CLA GitHub bot

@lock lock bot locked as resolved and limited conversation to collaborators Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants