Skip to content

ObjC: Update Sample#4108

Merged
jcanizales merged 4 commits intogrpc:masterfrom
bdotdub:objc-sample-update
Nov 12, 2015
Merged

ObjC: Update Sample#4108
jcanizales merged 4 commits intogrpc:masterfrom
bdotdub:objc-sample-update

Conversation

@bdotdub
Copy link
Copy Markdown
Contributor

@bdotdub bdotdub commented Nov 11, 2015

Addresses #3371


  • Uses the new ProtoMethod instead of the missing GRPCMethodName
  • Points the RemoteTest podspec to use the new path for the grpc_objective_c_plugin plugin
  • Update objc sample Podfile to use example projects

@grpc-kokoro
Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

1 similar comment
@grpc-kokoro
Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was unsure whether to change this one. There is no protoc binary at that path for me. I couldn't find a built protoc binary after running make in the root of the project or in the third_party/protobuf directory.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, sorry for that :-/ This file evolved to be used exclusively by the automatic tests (running ./tools/run_tests/run_tests.py -l objc from the root of the repo). Those will make plugins and set $CONFIG before doing pod install, making the podspec work.

As it became inappropriate for a sample, I added a version of it that just uses the installed protoc + plugin inside the examples directory. The Podfile of this sample should just point to that one (as the Podfile for the SwiftSample does). And ideally we should move generated_libraries/RemoteTestClient/* into tests/RemoteTestClient/* to eliminate confusion. (And also eliminate generated_libraries/RouteGuideClient, which isn't used anymore.)

Do you want to do that? Otherwise I'm happy to send a PR to your branch with the changes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The Podfile of this sample should just point to that one (as the Podfile for the SwiftSample does).

I can do this!

And ideally we should move generated_libraries/RemoteTestClient/* into tests/RemoteTestClient/* to eliminate confusion. (And also eliminate generated_libraries/RouteGuideClient, which isn't used anymore.)

I can send this over in a separate PR. Sound good?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds great!

@bdotdub
Copy link
Copy Markdown
Contributor Author

bdotdub commented Nov 11, 2015

@jcanizales ?

@jcanizales
Copy link
Copy Markdown
Contributor

Hi Benny! Thanks for this, it's awesome that you just went ahead and fixed it :)

@jcanizales
Copy link
Copy Markdown
Contributor

The changes in Sample/ViewController.m LGTM.

@bdotdub
Copy link
Copy Markdown
Contributor Author

bdotdub commented Nov 12, 2015

@jcanizales Ok - above issues addressed!

@jcanizales
Copy link
Copy Markdown
Contributor

Excellent; thanks again!

jcanizales added a commit that referenced this pull request Nov 12, 2015
@jcanizales jcanizales merged commit 87ffbba into grpc:master Nov 12, 2015
@bdotdub bdotdub deleted the objc-sample-update branch November 12, 2015 19:09
@bdotdub
Copy link
Copy Markdown
Contributor Author

bdotdub commented Nov 13, 2015

@jcanizales as a separate discussion, what's the priority getting Swift support to work? I'm having a really hard time getting the Swift sample to run. For any meaningful Swift project that uses Cocoapods and ObjC dependencies, you need to use use_frameworks!. I seem to get a lot of errors trying to get the Swift sample to work.

(No need to reply – I imagine you're very busy!)

@jcanizales
Copy link
Copy Markdown
Contributor

Ah, tricky! Let me open a separate issue and track it there.

@jcanizales
Copy link
Copy Markdown
Contributor

WRT priorities, I would say:

  1. Make the library usable from Objective-C (because without this, it isn't usable from Swift either). We still have some work to do here.
  2. Make the library not a pain to use from ObjC, and usable from Swift.
  3. Make it not a pain to use from Swift.

@lock lock Bot locked as resolved and limited conversation to collaborators Jan 29, 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.

4 participants