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

cronet build related changes to build.yaml, and added ifdef guard around objective_c cronet code. #6750

Merged
merged 5 commits into from
Jun 6, 2016

Conversation

makdharma
Copy link
Contributor

@makdharma makdharma commented Jun 1, 2016

changes to build.yaml, and added ifdef guard around objective_c cronet code.

@dgquintas
Copy link
Contributor

target public_headers_must_be_c89 (test/core/surface/public_headers_must_be_c89.c) does not name header grpc/grpc_cronet.h as a dependency

You need to remove the grpc_cronet.h include from test/core/surface/public_headers_must_be_c89.c

@dgquintas
Copy link
Contributor

LGTM. This has already been cherrypicked in the import.

@makdharma
Copy link
Contributor Author

test this please

@makdharma
Copy link
Contributor Author

@jcanizales can you LGTM and merge?

@jcanizales
Copy link
Contributor

Can you describe in the top-level message what the changes are doing?

@dgquintas
Copy link
Contributor

BTW, remember that we are still not merging ourselves. We are still adding the "ready to merge" label.

@makdharma makdharma changed the title cronet build related changes cronet build related changes to build.yaml, and added ifdef guard around objective_c cronet code. Jun 3, 2016
@makdharma
Copy link
Contributor Author

@jcanizales ping?

@jcanizales
Copy link
Contributor

Please add a TODO in GRPCChannel.h to move the Cronet-specific methods into a separate file (a category of GRPCChannel, as we do with GRPCCall).

I still don't know what the changes in build.yaml are for, as they're not self-evident, but I trust @dgquintas already reviewed them.

Other than that, LGTM

@jtattermusch jtattermusch merged commit 87ec3b7 into grpc:master Jun 6, 2016
@makdharma makdharma deleted the cronet branch June 9, 2017 23:39
@lock lock bot locked as resolved and limited conversation to collaborators Jan 23, 2019
@lock lock bot unassigned dgquintas Jan 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants