-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Rework the installation examples #8289
Conversation
This was using a very, very old version of ReactiveCocoa from 2015 and hasn't been applicable to any maintained versions of it for a very long time.
7ea724f
to
97d4797
Compare
@@ -17,7 +17,7 @@ target: | |||
- swiftpm-debug | |||
- swiftpm-address | |||
- swiftpm-thread | |||
- swiftpm-ios | |||
- ios-xcode-spm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This renaming is a lazy way to get it to stop installing baas for these tests.
cd examples/installation | ||
sh build.sh "test-osx-swift-xcframework" | ||
REALM_XCODE_VERSION=$REALM_XCODE_LATEST_VERSION ./build.rb osx xcframework |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The XCFramework test now uses the framework sitting in the build
directory if it's not asked to fetch a release, so we can reuse it for this test rather than needing a separate target.
;; | ||
|
||
verify-cocoapods-ios-dynamic) | ||
PLATFORM=$(echo "$COMMAND" | cut -d - -f 3) | ||
# https://github.com/CocoaPods/CocoaPods/issues/7708 | ||
export EXPANDED_CODE_SIGN_IDENTITY='' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bug this was working around was fixed a while ago.
@@ -735,21 +706,6 @@ case "$COMMAND" in | |||
exit 0 | |||
;; | |||
|
|||
"verify-osx-swift") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these verify- targets which just forward to the test- target are now covered by a verify-*
target.
2acd667
to
679e0ab
Compare
The installation examples were originally created when there was just iOS and macOS and cross-platform Apps weren't a thing, and the approach that worked then has turned into a giant mess. Consolidating (almost) all the installation tests into a single codebase with separate project files per installation method cuts the amount of code involved by 90% and makes adding more platforms relatively painles cuts the amount of code involved by 90% and makes adding more platforms relatively painless. Separate obj-c and Swift tests have not proven to have any value. We've never had any bugs caught specifically by the obj-c test which wasn't also caught by the swift test other than ones specifically related to the obj-c release zip, so now everything except for the obj-c only iOS static framework is tested only via Swift. Some of the tests ended up just plain being redundant; we had separate XCFramework and fat framework tests, but then the fat framework tests had to be ported to use XCFrameworks. Running the smoke tests in the simulator has never actually caught a bug and doubles the amount of things to build (and adds the requirement to reset the simulators, which takes a while), so ditch that and only test archiving the app for device. The CocoaPods transitive dependency test is now done as part of the normal cocoapods tests rather than a separate one. All platforms are now tested via Xcode SPM rather than only iOS. tvOS is now tested for all installation methods (previously it was not tested at all due to how much of a hassle adding platforms was). visionOS is now tested when using Xcode 15 (i.e. it still isn't actually tested on CI).
679e0ab
to
7cc3b60
Compare
Passing release pipeline with these changes: https://ci.realm.io/blue/organizations/jenkins/cocoa-pipeline/detail/cocoa-pipeline/2090/pipeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great work, the new build installation script is easier to understand now, and reworking the installation example removes a lot of files which is also nice. Some questions and minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use notxcodeproj
so Carthage cannot see it. But how we later test the projec (installation method)t specifically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build.rb symlinks SwiftPackageManager.notxcodeproj to SwiftPackageManager.xcodeproj, which is gitignored so that we don't accidentally commit it.
@@ -5,30 +5,23 @@ platformNames = ['osx': 'macOS', 'ios': 'iOS', 'watchos': 'watchOS', 'tvos': 'tv | |||
carthageXcodeVersion = '14.3.1' | |||
docsSwiftVersion = '5.8.1' | |||
|
|||
def installationTest(platform, test, language) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you tested this for a release, how do you do it without actually posting a release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of the cocoa-pipeline job, so it happens after building the release but before publishing it, so I just ran it on this branch. There's also post-release testing which validates that the published release works, and that'll probably need some fiddling to fix after merging this since it's hard to validate now.
for (def test in ["dynamic", "cocoapods", "carthage"]) { | ||
parallelBuilds["Installation - ${platformName} Swift ${test}"] = installationTest(platform, test, 'swift') | ||
} | ||
parallelBuilds['Installation - iOS Static'] = installationTest('ios', 'xcframework', carthageXcodeVersion, 'objc', 'static') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this the only installation test we are doing for obj-s, is that what we are supposed to be doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Installing the Swift SDK only works if the obj-c SDK successfully installs, so in practice every time we've had something break the obj-c installation it's been caught by the Swift tests and separately testing obj-c as well hasn't been useful. The static framework needs to test obj-c directly because we don't ship a static Swift framework.
} | ||
parallelBuilds['Installation - iOS Static'] = installationTest('ios', 'xcframework', carthageXcodeVersion, 'objc', 'static') | ||
parallelBuilds['Installation - XCFramework Evolution'] = xcframeworkEvolutionTest() | ||
for (def version in xcodeVersions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this duplicating testing
- installationTest('osx', 'xcframework', version, 'swift', ‘dynamic’)
where version == carthageXcodeVersion
test platform, 'cocoapods', 'static' unless platform == 'visionos' | ||
end | ||
|
||
test 'ios', 'xcframework', 'static' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to test this in only one platform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For historical reasons we ship an iOS-specific static framework (iOS 7 didn't support dynamic frameworks, and we just kept shipping it even after dropping iOS 7). These changes generalize the build so that the static framework isn't iOS-specific but we're still only shipping it for iOS.
- cocoapods-watchos | ||
- cocoapods-tvos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to distinguish which targets are building the framework and which are installation test
The installation examples were originally created when there was just iOS and macOS and cross-platform Apps weren't a thing, and the approach that worked then has turned into a giant mess. Consolidating (almost) all the installation tests into a single codebase with separate project files per installation method cuts the amount of code involved by 90% and makes adding more platforms relatively painles cuts the amount of code involved by 90% and makes adding more platforms relatively painless.
Separate obj-c and Swift tests have not proven to have any value. We've never had any bugs caught specifically by the obj-c test which wasn't also caught by the swift test other than ones specifically related to the obj-c release zip, so now everything except for the obj-c only iOS static framework is tested only via Swift.
Some of the tests ended up just plain being redundant; we had separate XCFramework and fat framework tests, but then the fat framework tests had to be ported to use XCFrameworks.
Running the smoke tests in the simulator has never actually caught a bug and doubles the amount of things to build (and adds the requirement to reset the simulators, which takes a while), so ditch that and only test archiving the app for device.
The CocoaPods transitive dependency test is now done as part of the normal cocoapods tests rather than a separate one.
All platforms are now tested via Xcode SPM rather than only iOS. tvOS is now tested for all installation methods (previously it was not tested at all due to how much of a hassle adding platforms was).
visionOS is now tested when using Xcode 15 (i.e. it still isn't actually tested on CI). Currently only the SPM installation method supports visionOS. Cocoapods is actively working on adding support, and XCFramework support is only blocked on us cutting a release with it.