Update vendored gRPC-core to v1.19.1

[timburks]

This commit also fixes a few minor problems in the vendoring
scripts and updates a call in shim/channel.c to use a modified API.

[timburks]

@MrMage Do the failures on the Linux tests mean anything to you? So far I've I only directly tested on my Mac; I can try on a Linux VM next.

[MrMage]

@timburks thank you for looking into this! The only test failure I can find on Linux CI is this:

/home/travis/build/grpc/grpc-swift/Tests/SwiftGRPCTests/ClientCancellingTests.swift:72: error: ClientCancellingTests.testServerStreaming : XCTAssertEqual failed: ("cancelled") is not equal to ("ok") - 

That does not ring a bell I'm afraid. It might be that the server is simply too fast at sending all responses. You could try working around that by using a string with way more words in

let call = try! client.expand(Echo_EchoRequest(text: "foo bar baz")) { callResult in

or replacing the provider with one that sleeps for, say, 10ms before starting to send out replies. (Example for more strings: (0..<1000).map { String(describing: $0) }.joined(separator: " "))

In terms of https://github.com/grpc/grpc/blob/master/src/objective-c/BoringSSL-GRPC.podspec, I think that file is generated with parameters obtained from running one of the templated gRPC vendoring script generator. I think we are running a similar generator to generate our vendoring script, so in theory the complexity of that file should not deter us, given that the codegen creating that file should also be able to generate our vendoring scripts. (I might be missing something.)

[MrMage]

P.S.: When Assets/roots.pem is updated, I think we should also re-run swift run RootsEncoder > Sources/SwiftGRPC/Core/Roots.swift.

[MrMage]

I think https://github.com/grpc/grpc-swift/blob/master/SwiftGRPC.podspec will also need to be updated, as the new shim would be incompatible with old versions of gRPC-Core.

[timburks]

@MrMage after I slowed the server by sending more strings (100 instead of three), the tests now pass, but I notice 99 new error messages "expand error: unknown" which I think are coming from the server after the client canceled. I think these need to be fixed/cleaned up, but maybe in a separate PR to keep this one from getting too complicated. What do you think?

[timburks]

@MrMage @rebello95 Could we update the podspec in a separate PR?

[MrMage]

@MrMage after I slowed the server by sending more strings (100 instead of three), the tests now pass, but I notice 99 new error messages "expand error: unknown" which I think are coming from the server after the client canceled. I think these need to be fixed/cleaned up, but maybe in a separate PR to keep this one from getting too complicated. What do you think?

In that case, it might simply be easier to implement a custom EchoProvider for this whole test suite that waits 10 ms before starting to send responses, and that avoids logging errors it encounters during sending (because those are expected in this case).

grpc-swift/Tests/SwiftGRPCTests/ServerThrowingTests.swift

Line 25 in 7958369

fileprivate class StatusThrowingProvider: Echo_EchoProvider {

demonstrates a similar custom provider implementation, but note that ours would actually need to be more similar to the "regular" EchoProvider, just with a short delay and without send error logging.

@MrMage @rebello95 Could we update the podspec in a separate PR?

I think that should be fine; users will only be affected once we push a new Podspec. It's just a two-line change, though.

[MrMage]

@timburks as far as I can tell, everything looks good, and I trust that you did the vendoring correctly. However, I know that @rebello95 had issues vendoring gRPC-Core v1.19, so it might be useful to elaborate on why it worked for you with seemingly only minor changes to our scripts.

I would also still suggest creating a custom provider subclass as outlined in #417 (comment).

[timburks]

@MrMage Line 81 of @rebello95's updated scripts/vendor-boringssl.sh contained an incorrect path that caused the script to fail to update a header file. The dropped update caused the boringssl build to expect a large number of functions to be linked in from assembly language source files, so resulting builds would fail at link time with a large number of unresolved symbols. The vendor-grpc script also needed to be updated to make the nanopb headers visible to Swift Package Manager; when they couldn't be found, a missing "pb.h" error was breaking builds.

I wouldn't mind adding the custom EchoProvider, but expect it might still produce at least some superfluous error messages if it started sending after the client had closed the connection early. If we look closer, we might decide that these error messages shouldn't be printed because they could be part of normal operation.

[MrMage]

@MrMage Line 81 of @rebello95's updated scripts/vendor-boringssl.sh contained an incorrect path that caused the script to fail to update a header file. The dropped update caused the boringssl build to expect a large number of functions to be linked in from assembly language source files, so resulting builds would fail at link time with a large number of unresolved symbols. The vendor-grpc script also needed to be updated to make the nanopb headers visible to Swift Package Manager; when they couldn't be found, a missing "pb.h" error was breaking builds.

Thanks for the elaboration, much appreciated!

I wouldn't mind adding the custom EchoProvider, but expect it might still produce at least some superfluous error messages if it started sending after the client had closed the connection early. If we look closer, we might decide that these error messages shouldn't be printed because they could be part of normal operation.

The error logging is in the EchoProvider subclass the test is currently using:

grpc-swift/Sources/Examples/Echo/EchoProvider.swift

Line 36 in 7958369

print("expand error: \(error)")

If we use a (simpler) custom subclass for these tests that does not contain these logging calls, no errors should show up in the log.

[timburks]

@MrMage If you're satisfied with my change to the tests, please merge this. Thanks for reviewing!

[MrMage]

@timburks, sorry but I don't think changing EchoProvider is the correct approach to squelching these error. Instead, I think we should simply create a dedicated Echo_EchoProvider implementation for that test suite. I'm happy to do that; just let me know before I start messing with your PR ;-)

[timburks]

@MrMage OK, I'm fine with your preference. Please go ahead and do that if you have time.

[MrMage]

@timburks I've pushed 04d2d1f to your branch; please check if you are satisfied with that change, then merge.

[rebello95]

@MrMage @rebello95 Could we update the podspec in a separate PR?

I think that should be fine; users will only be affected once we push a new Podspec. It's just a two-line change, though.

Sorry I missed this message. I think we should update it as part of this PR and make sure it builds/passes lints (pod spec lint) prior to merging. We probably shouldn't have Swift PM and CocoaPods running different versions of gRPC Core on master.

[MrMage]

Sorry I missed this message. I think we should update it as part of this PR and make sure it builds/passes lints (pod spec lint) prior to merging. We probably shouldn't have Swift PM and CocoaPods running different versions of gRPC Core on master.

I think this does not matter too much, provided the two do not get tagged. However, I would definitely suggest updating the Podspec as close to updating the vendored code as possible.

@timburks what would be your reasoning to not update the Podspec in this PR? Could it be included?

[timburks]

@MrMage @rebello95 OK, I've also updated the podspec.

pod spec lint has one failure which we'll fix when we tag and push v0.8.2.

[timburks]

I updated the podspec with this command:
pod trunk push SwiftGRPC.podspec --swift-version=4.0 --allow-warnings

The warnings are in the vendored gRPC-Core code (all in grpc_security.h).

From the following warning (produced during validation), we will need to add swift_version to the podspec soon:

- WARN  | swift: The validator used Swift 3.2 by default because no Swift version was specified. To specify a Swift version during validation, add the `swift_version` attribute in your podspec. Note that usage of the `--swift-version` parameter or a `.swift-version` file is now deprecated.

[rebello95]

Sounds good, thanks @timburks!

[MrMage]

Sounds good! I think we can simply specify a Swift version of 4.2 with the next Podspec update.

[MrMage]

@timburks the "Release" on GitHub looks different from the other ones: https://github.com/grpc/grpc-swift/releases. @rebello95, is there a shortcut to including the commit log on the GitHub release?

[rebello95]

@MrMage generally I use something like this:

git log --pretty=format:"- %s - %an (%h)" 0.7.0...master