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

Libuv pod and event engine core test suite setup for iOS/ObjC #27563

Merged
merged 2 commits into from
Oct 7, 2021

Conversation

dennycd
Copy link
Contributor

@dennycd dennycd commented Oct 2, 2021

Initial scaffolding to bring in libuv and event engine core test structure for iOS/ObjC stack. Change summary below:

1. Introduce new podspec for libuv-grpc

Creating new podspec for bringing in libuv on iOS/Objc.

  • Libuv-gRPC.podspec: new podspec for libuv. Published as Libuv-gRPC on cocoapod trunk.
  • gRPC-Core.podspec: adding libuv as dependency lib for gRPC-Core pod

Reasons for the change:

  • A separate libuv pod helps modularize gRPC-Core's 3p dependencies on iOS stack. Instead of aggregating all 3p source through gRPC-Core, here we packaging libuv under a standalone pod module
  • OSS Community reuse of Libuv-gRPC for iOS

2. Introducing iOS Core Test pod setup and adding event engine timer test suite

  • Setting up a new iOS test pod (CoreTests) dedicated for unit testing c-core from within iOS build chain & runtime.
    • In the future, CoreTests will be hosting other C-Core/EE test coverage on iOS. These should be light-weight test logic aiming at verifying c-core's functions on iOS.

A few follow up todos after this initial step

  • Auto-generate libuv source & headers via buildgen script using template
    • Currently as 1st step, we hard-coded libuv source in podspec. Ideally these are pulled from libuv.BUILD rules and tunneled through its template files and auto-populated (alongside build flags) via buildgen script. This reduce maintenance overhead.
  • Adding test coverage for EE's timer implementation into EventEngineTimerTests
    • also look at how we can have better reuse of existing test logic from c-core. One possibility is to expose EE's test logic via test util and call from iOS test (similar to cronet e2e test)
  • More follow up test coverage for other EE // libuv components

@dennycd dennycd requested a review from sampajano October 2, 2021 06:47
@dennycd
Copy link
Contributor Author

dennycd commented Oct 2, 2021

@HannahShiSFB

@dennycd dennycd requested a review from drfloob October 2, 2021 06:53
@dennycd dennycd changed the title Add podspec for libuv-grpc Libuv pod and event engine core test suite setup Oct 2, 2021
@dennycd dennycd changed the title Libuv pod and event engine core test suite setup Libuv pod and event engine core test suite setup for iOS/Objc Oct 2, 2021
@dennycd dennycd added lang/ObjC platform/iOS release notes: no Indicates if PR should not be in release notes labels Oct 2, 2021
@dennycd dennycd changed the title Libuv pod and event engine core test suite setup for iOS/Objc Libuv pod and event engine core test suite setup for iOS/ObjC Oct 2, 2021
@drfloob
Copy link
Member

drfloob commented Oct 5, 2021

Great to see progress made on libuv/iOS!

The binary size diff test is broken due to missing libuv dependencies, so I think there's still a piece missing here.

Sat Oct  2 00:21:45 PDT 2021 - In file included from /Volumes/BuildData/tmpfs/src/github/grpc/src/objective-c/examples/Sample/Pods/Libuv-gRPC/src/uv-common.c:22:
Sat Oct  2 00:21:45 PDT 2021 - /Volumes/BuildData/tmpfs/src/github/grpc/src/objective-c/examples/Sample/Pods/Libuv-gRPC/include/uv.h:53:12: fatal error: 'uv/uv/errno.h' file not found
Sat Oct  2 00:21:45 PDT 2021 -   #include <uv/uv/errno.h>
Sat Oct  2 00:21:45 PDT 2021 -            ^~~~~~~~~~~~~~~
Sat Oct  2 00:21:45 PDT 2021 - /Volumes/BuildData/tmpfs/src/github/grpc/src/objective-c/examples/Sample/Pods/Libuv-gRPC/include/uv.h:53:12: note: did not find header 'uv/errno.h' in framework 'uv' (loaded from '/Volumes/BuildData/tmpfs/src/github/grpc/src/objective-c/examples/Sample/Build/Build/Build/Products/Release-iphoneos/Libuv-gRPC')

Also, it looks like the sanity check is failing on a need for a clang_format run.

@dennycd dennycd force-pushed the event_engine branch 13 times, most recently from 55237a4 to 5f4cef8 Compare October 6, 2021 17:17
Julia, pyuv, and others.
DESC

spec.homepage = "https://libuv.org/"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

point to a landing page on cocoapod trunk

Copy link
Contributor

@sampajano sampajano left a comment

Choose a reason for hiding this comment

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

Thanks so much for the change, Denny (And the detailed in person review)!! Great step to unblock the EventEngine migration!

My only major comment is that while Podspec looks to be a nice way to manage sub-dependencies, the way it's done right now does seem to involve some duplicate maintenance overhead (on top of maintaining the build for core, e.g. specifying all the headers / files / build flags).

Maybe you could document a bit more on why you chose this approach (and maybe why it's superior) and how going forward we could minimize its maintenance cost.

Thanks!! :)

(btw It'd also be nice if some other build expert can take a look at this PR too :))

src/objective-c/Libuv-gRPC.podspec Show resolved Hide resolved
src/objective-c/Libuv-gRPC.podspec Show resolved Hide resolved
src/objective-c/Libuv-gRPC.podspec Show resolved Hide resolved
gRPC-Core.podspec Outdated Show resolved Hide resolved
@dennycd
Copy link
Contributor Author

dennycd commented Oct 6, 2021

all build break fixed

@dennycd dennycd force-pushed the event_engine branch 2 times, most recently from ebc9489 to fb3805c Compare October 7, 2021 00:13
Copy link
Member

@drfloob drfloob left a comment

Choose a reason for hiding this comment

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

I'm learning a bit about cocoapods tonight ... do you intend to maintain a public, separately-installable libuv target? I'm not really sure it's our place to do so, especially given that we might not end up using it for iOS ourselves in gRPC (worst case), and that a handful of obj-c/swift platforms are not officially supported by libuv. CC @nicolasnoble.

https://cocoapods.org/pods/Libuv-gRPC

Also, it appears the sanity checks are still failing, possibly from changes in master since you last merged?

templates/src/objective-c/Libuv-gRPC.podspec.template Outdated Show resolved Hide resolved
Copy link
Member

@drfloob drfloob left a comment

Choose a reason for hiding this comment

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

Chatting offline, everything SGTM! Thanks Denny!

Summary: we should look into the possibility of making the libuv-grpc cocoapod private if possible, but I don't want to block this PR if not.

@sampajano
Copy link
Contributor

sampajano commented Oct 7, 2021

Chatting offline, everything SGTM! Thanks Denny!

Summary: we should look into the possibility of making the libuv-grpc cocoapod private if possible, but I don't want to block this PR if not.

Yeah on a similar note.. i kind of prefer how some of our other pods are named, e.g. gRPC-RxLibrary & gRPC-ProtoRPC, as opposed to Libuv-gRPC and BoringSSL-GRPC, as the former seems to be more eco-friendly to the overall cocoapods namespace (unless we really have intentions for 3rd party users to depend on them) :)

@dennycd dennycd force-pushed the event_engine branch 4 times, most recently from dc016d9 to 30d8f28 Compare October 7, 2021 19:15
@dennycd
Copy link
Contributor Author

dennycd commented Oct 7, 2021

Thanks AJ. We will spin out a separate task to move gRPC-only dependency pods into private. For the current PR, we will mark it as intended for gRPC only usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported Specifies if the PR has been imported to the internal repository lang/ObjC platform/iOS 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

3 participants