Compile SocketRocket directly into the SDK #1090
Conversation
This is the first step to compiling SocketRocket directly into the ably-cocoa SDK.
This only adds the files into the project - it doesn’t yet add anything to a target.
ARTSRDelegateController.h and ARTSRError.h have the following line: #import <SocketRocketAblyFork/ARTSRWebSocket.h> This works fine when compiling these files inside the SocketRocketAblyFork framework, but when compiling them directly inside the SDK this path can’t be resolved. So, we create a fake `include` directory containing a symlink called SocketRocketAblyFork. When we add this `include` directory to the header search paths, this makes the import work without needing to modify the SocketRocket code. This feels a bit nasty and there may be a better way to solve it. We could try and remove the angle-bracket #imports from the SocketRocket code, but once I’d found the current solution I didn’t fancy spending extra time exploring that option. It may be a better one, though.
We remove it as a dependency in Carthage and CocoaPods, and deal with the fallout. There is some further work needed to fix the CocoaPod, which I’ll do in a separate commit. For future reference, we are doing this as part of a wider effort to reduce the number of transitive dependencies that we impose on users of the SDK. See issue #949 for more details. I manually removed the framework copy steps. I don’t _think_ there is a more official Carthage-provided way of doing this, but I could be quite wrong; I don’t know much about Carthage.
Similarly to 7c18fe2, SocketRocket’s angle-bracket #imports are a thorn in our side. We use a prepare_command in the podspec to replace them with double-quoted #imports.
We need to make sure we’ll still be complying with the conditions under which SocketRocket is licensed, when we start to bundle it into our SDK. I took this guidance from the “Bundling permissively-licensed dependencies” section on the Apache Infrastructure “Assembling LICENSE and NOTICE files” document [1]. Our SDK is licensed under the Apache License version 2.0, and SocketRocket is licensed under a 3-clause BSD license. If there is someone with more understanding of either the law or software licenses than me, it would be great if we could try and figure out if this change has any implications for users of our SDK. [1] https://infra.apache.org/licensing-howto.html
This appears to be a requirement of SocketRocket’s BSD license: > Redistributions in binary form must reproduce the above copyright > notice, this list of conditions and the following disclaimer in the > documentation and/or other materials provided with the distribution. I believe that including our LICENSE and SocketRocket’s LICENSE, the former containing a pointer to the latter as of commit 75bc4e6, will satisfy this requirement. The unzipped directory (named “Carthage” same as it currently is) looks like this: Carthage ├── Build │ ├── Mac │ │ └── (framework and dSYM) │ ├── iOS │ │ └── (framework and dSYM) │ └── tvOS │ │ └── (framework and dSYM) ├── LICENSE └── SocketRocket └── LICENSE
lawrence-forooghian
requested review from
ricardopereira and
QuintinWillison
|
Yep, we could certainly do that if we wanted to, but that would be a future enhancement. This piece of work is just to hide the SocketRocket dependency from consumers of the SDK. |
# Conflicts: # Ably.xcodeproj/project.pbxproj # LICENSE # Makefile # Scripts/run_examples_tests.sh
|
I had a quick run at making this pull request viable as it had been left to languish for far too long, however it's not immediately obvious to me why the build would be failing now (in something seemingly unrelated). |
This reverts commit b4fe73e.
…ile-socketrocket-into-sdk # Conflicts: # Cartfile.private # Cartfile.resolved
There was a problem hiding this comment.
Thanks for removing the superfluous files, @maratal. 👍
However (and apologies for only just noticing this) but can you please remove the unnecessary layer of folder indirection for socket rocket. From root I would expect to be seeing source files at SocketRocket/ rather than SocketRocket/SocketRocket/ as is currently the case.
It was done not to face possible “include” issues since the whole thing is to be replaced soon. The less we touch it, the better. But probably I shouldn’t cut corner there… 🤪 |
|
Oh, I see. That's a fair point, @maratal. Also, we need to get checks passing. FYI, @lukasz-szyszkowski |
# Conflicts: # Ably.podspec # Ably.xcodeproj/project.pbxproj
|
Since this is a small subset of what @lukasz-szyszkowski has done in #1136 |
I'm closing this one, changes are in #1136 |
RTN11c: Explicit connect should keep trying to recover connection state
What’s new
This removes SocketRocket as a dependency from the
Cartfileand podspec, and compiles SocketRocket directly into the SDK. It uses a submodule to pull the SocketRocket code in.We're doing this as part of a wider effort to reduce the number of transitive dependencies that we impose on users of the SDK. See #949 for more details.
The commit messages explain what I’ve done in lot more detail, and I'd recommend reviewing this commit-by-commit.
Questions for reviewers
I've done a fair bit of testing myself - mainly to check that the Carthage and CocoaPods builds are compiling correctly, and that the LICENCE is correctly included in the zip file that we generate for upload to GitHub. However, I'm not too sure how to test that everything is working functionally. I have a handful of test failures when running the unit tests, but my understanding is that this is to be expected as we do have some intermittent failures. What can we do to increase our confidence in merging these changes?