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

[DO NOT MERGE] Introduce swift version testing #513

Draft
wants to merge 37 commits into
base: trunk
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
ec040d9
Revert swift-tools-version support to 5.9
andrewdmontgomery Oct 16, 2024
7c3bdac
Use a build matrix to validate on multiple versions of Xcode
andrewdmontgomery Oct 16, 2024
daefb44
Specify the correct IMAGE_ID
andrewdmontgomery Oct 16, 2024
c5f84a6
Try including a separate `[email protected]`
andrewdmontgomery Oct 16, 2024
7d0a0e3
Update "Build and Test" label to include matrix build name
andrewdmontgomery Oct 16, 2024
4aeaf93
Revert "pins" version from 3 to 2 in Package.resolved
andrewdmontgomery Oct 16, 2024
a280108
Revert "Try including a separate `[email protected]`"
andrewdmontgomery Oct 16, 2024
4260fb5
Refactor testing to support passing the device to the test
andrewdmontgomery Oct 16, 2024
6d0dbe6
Fix matrix setup, references
andrewdmontgomery Oct 16, 2024
495b8c1
Use env variables instead of passing parameters
andrewdmontgomery Oct 16, 2024
7877527
Attempt fix for Swift 5.9 build error
andrewdmontgomery Oct 16, 2024
0f95d01
Add support for validating podspecs for Swift 5.9 and Swift 5.10
andrewdmontgomery Oct 16, 2024
05025f3
Group matrix builds
andrewdmontgomery Oct 17, 2024
c51d45a
Pass the device as an argument instead of using ENV
andrewdmontgomery Oct 17, 2024
9132be5
TRY: remove `matrix.setup` and specify all configs with `matrix.adjus…
andrewdmontgomery Oct 17, 2024
5f8e661
TRY: adding `matrix.setup` with empty values
andrewdmontgomery Oct 17, 2024
a4acef4
TRY: setting null values in `matrix.setup`
andrewdmontgomery Oct 17, 2024
e34d6e0
TRY: setting values to empty strings
andrewdmontgomery Oct 17, 2024
b2c7460
Revert "TRY: setting values to empty strings"
andrewdmontgomery Oct 17, 2024
a90ec77
Revert "TRY: setting null values in `matrix.setup`"
andrewdmontgomery Oct 17, 2024
c7d7437
Revert "TRY: adding `matrix.setup` with empty values"
andrewdmontgomery Oct 17, 2024
fdd0a52
Revert "TRY: remove `matrix.setup` and specify all configs with `matr…
andrewdmontgomery Oct 17, 2024
c094892
Clean up the `matrix.setup` instances, include clarifying comments
andrewdmontgomery Oct 17, 2024
5c7946c
TRY: setting a blank `matrix.setup` set to null
andrewdmontgomery Oct 17, 2024
d6eba0c
TRY: setting a blank `matrix.setup` by setting to `{}`
andrewdmontgomery Oct 17, 2024
612b281
Revert "TRY: setting a blank `matrix.setup` by setting to `{}`"
andrewdmontgomery Oct 17, 2024
6b75175
Revert "TRY: setting a blank `matrix.setup` set to null"
andrewdmontgomery Oct 17, 2024
f14bc5b
Refactor fastlane `test` to use defaults where possible
andrewdmontgomery Oct 17, 2024
070180d
Remove unused properties in matrix for pod validation
andrewdmontgomery Oct 17, 2024
fb4efe8
Fix logic for generating the `device` string
andrewdmontgomery Oct 17, 2024
98b21fa
Add support for testing Xcode 16.0
andrewdmontgomery Oct 17, 2024
541a4c4
Fix Xcode 16 string
andrewdmontgomery Oct 17, 2024
3388ffe
Use Xcode 15.2 instead of 15.1 for the Swift 5.9 checks
andrewdmontgomery Oct 18, 2024
15fbebf
Add swift version to matrix builds, use in label
andrewdmontgomery Oct 18, 2024
b00cc5d
Move source of truth for Xcode version to `shared-pipeline-vars`
andrewdmontgomery Oct 18, 2024
4482a62
Extract Swift versions into `shared-pipeline-vars`
andrewdmontgomery Oct 18, 2024
abb0aec
Use '_' instead of '.' for strings that appear in step labels
andrewdmontgomery Oct 18, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 47 additions & 11 deletions .buildkite/pipeline.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
agents:
queue: "mac"
env:
IMAGE_ID: $IMAGE_ID
IMAGE_ID: $CURRENT_IMAGE_ID

steps:
#################
Expand All @@ -21,20 +21,56 @@ steps:
#################
# Build and Test
#################
- label: "📦 Build and Test Swift Package"
key: "test"
command: |
validate_swift_package
plugins: [$CI_TOOLKIT]
- group: "📦 Build and Test Swift Package"
steps:
- label: "📦 Build and Test Swift Package (Swift {{matrix.swift_version}})"
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that changing the label of those jobs will have the side-effect of changing the name of the commit status event reported on the GitHub PR.

This PR targets release/3.0.0 which is not a protected branch, so doesn't have any required check, so this doesn't show here. But for PRs targeting trunk, the branch protection settings requires the buildkite/gravatar-sdk-ios/build-and-test-swift-package check to pass… and with the new names of those CI jobs, such a check with that exact name won't exist anymore.
image

So this means that once this PR lands and then is propagated to trunk, you'll have to remember to update the branch protection settings of the trunk branch in GitHub to stop requiring the old check names and instead add the new ones instead.

Copy link
Contributor

@AliSoftware AliSoftware Oct 18, 2024

Choose a reason for hiding this comment

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

Note that the names of those checks emitted to GitHub are derived by Buildkite because we have the following settings set on the gravatar-ios-sdk pipeline in Buildkite:
image

If you don't like the names generated by Buildkite and find they are too verbose, we can disable this option (via our Infrastructure-as-Code setup for this pipeline and instead add the appropriate notify: { github_commit_status: { context: "custom name" } } entry to each of the step we want a GitHub status to be reported for.
The only "drawback" is that if we disable that setting for Buildkite to generate the commit statuses for us, we'll have to add the notify entry for every single step manually (i.e. we can't just customize some of them but let buildkite generate the status for the others). But that's still the approach we've tended to go towards in other pipelines lately (see: paaHJt-78h-p2), because custom status names are so much nicer to read in the UI of a Github PR than the generated one

Let us know if you want to go that route. You can already start adding the notify: … blocks for each step to provide custom status names, and we can take care of disabling the ones generated by Buildkite once the custom ones from notify: land in trunk and we update the branch protection settings in GitHub accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But that's still the approach we've tended to go towards in other pipelines lately (see: paaHJt-78h-p2), because custom status names are so much nicer to read in the UI of a Github PR than the generated one

Oooh, thank you!

You can already start adding the notify: … blocks for each step...

Excellent, good idea.

key: "test"
command: |
validate_swift_package test device_model:"{{matrix.device_model}}" os:"{{matrix.device_os}}"
plugins: [$CI_TOOLKIT]
env:
IMAGE_ID: "{{matrix.image_id}}"
matrix:
setup: # Specify the current version of Xcode, Swift, a device, and the required version of iOS
image_id: $CURRENT_IMAGE_ID
swift_version: $CURRENT_SWIFT_VERSION
device_model: $CURRENT_DEVICE
device_os: $CURRENT_OS
adjustments: # Specify additional versions of Xcode, Swift, a device, and the required version of iOS
- with: # Swift 5.9
image_id: $SWIFT_5_9_IMAGE_ID
swift_version: $SWIFT_5_9_VERSION
Copy link
Contributor

@AliSoftware AliSoftware Oct 18, 2024

Choose a reason for hiding this comment

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

Given how YAML works, I expect that if $SWIFT_5_9_VERSION has a value that resembles a float value (swift_version: 5.9), the YAML parser might interpret the value of that attribute as a float instead of a string… and Buildkite might then complain that this is not of the expected type? (wondering if that's the reason why you pushed abb0aec?)

If that's the case, and to prevent YAML from interpreting this as a float even if it looks like one and instead force it to interpret it as a string, you have two options:

  1. Quote the value, i.e. swift_version: "$SWIFT_5_9_VERSION"
  2. Add a !!str tag in front of the value to force specifying the type of the value to be string, i.e. swift_version: !!str $SWIFT_5_9_VERSION

The first option is probably easier to understand (and less surprising to people who don't know some advanced YAML techniques likes !! tags) so I'd probably just go with it.

I'd suggest to apply the same rule (i.e. quoted strings) for other values as well, especially device_os: "$SWIFT_*_OS" which might be subject to a similar misinterpretation as a Float instead of a String?

device_model: $SWIFT_5_9_DEVICE
device_os: $SWIFT_5_9_OS
- with: # Swift 6.0
image_id: $SWIFT_6_0_IMAGE_ID
swift_version: $SWIFT_6_0_VERSION
device_model: $SWIFT_6_0_DEVICE
device_os: $SWIFT_6_0_OS

###################
# Validate Podspec
###################
- label: "🔬 Validate Podspecs"
key: "validate"
command: |
.buildkite/commands/validate-pods.sh
plugins: [$CI_TOOLKIT]
- group: "🔬 Validate Podspecs"
steps:
- label: "🔬 Validate Podspecs (Swift {{matrix.swift_version}})"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, changing the name of the label will impact the name of the status check reported in GitHub PR that the trunk branch protection settings expects, and will thus require an update of the branch protection settings in GitHub once this lands in trunk.

key: "validate"
command: |
.buildkite/commands/validate-pods.sh
plugins: [$CI_TOOLKIT]
env:
IMAGE_ID: "{{matrix.image_id}}"
matrix:
setup: # Specify the current version of Xcode and Swift
image_id: $CURRENT_IMAGE_ID
swift_version: $CURRENT_SWIFT_VERSION
adjustments: # Specify additional versions of Xcode and Swift
- with:
image_id: $SWIFT_5_9_IMAGE_ID
swift_version: $SWIFT_5_9_VERSION
- with:
image_id: $SWIFT_6_0_IMAGE_ID
swift_version: $SWIFT_6_0_VERSION

#######################
# Publish the Podspecs (if we're building a tag)
Expand Down
27 changes: 25 additions & 2 deletions .buildkite/shared-pipeline-vars
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,29 @@
# This file is `source`'d before calling `buildkite-agent pipeline upload`, and can be used
# to set up some variables that will be interpolated in the `.yml` pipeline before uploading it.

export IMAGE_ID=$(echo "xcode-$(cat .xcode-version)")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only place that uses the .xcode-version. Some day we may want to have Fastlane use it to enforce the version of Xcode, just to guarantee that when developers run make test, they're running in the same basic environment as CI.

But for now, we don't do that, so we don't need this abstraction.


export CI_TOOLKIT="automattic/a8c-ci-toolkit#3.2.2"

## Supported versions of Swift
# Swift 6.0
export SWIFT_6_0_IMAGE_ID="xcode-16.0-v7"
export SWIFT_6_0_VERSION="6.0"
export SWIFT_6_0_DEVICE="default" # Use the default value in Fastlane
export SWIFT_6_0_OS="18.0"

# Swift 5.10
export SWIFT_5_10_IMAGE_ID="xcode-15.4"
export SWIFT_5_10_VERSION="5.10"
export SWIFT_5_10_DEVICE="default"
export SWIFT_5_10_OS="17.5"

# Swift 5.9
export SWIFT_5_9_IMAGE_ID="xcode-15.2-xl"
export SWIFT_5_9_VERSION="5.9"
export SWIFT_5_9_DEVICE="default" # Use the default value in Fastlane
export SWIFT_5_9_OS="17.2"

# Current Development Environment
export CURRENT_IMAGE_ID=$SWIFT_5_10_IMAGE_ID
export CURRENT_SWIFT_VERSION=$SWIFT_5_10_VERSION
export CURRENT_DEVICE=$SWIFT_5_10_DEVICE
export CURRENT_OS="default" # Use the default value in Fastlane
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels a little more self-documenting and clear. I also added the Swift version, and used it in the label fields for these steps, which makes the intetion clearer.

1 change: 0 additions & 1 deletion .xcode-version

This file was deleted.

2 changes: 1 addition & 1 deletion Package.resolved
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,5 @@
}
}
],
"version" : 3
Copy link
Contributor Author

@andrewdmontgomery andrewdmontgomery Oct 18, 2024

Choose a reason for hiding this comment

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

This change (to "version" : 3) snuck into a PR where I updated the SwiftFormat plugin dependency. Reverting to version 2 doesn't seem to cause any issues. Leaving it on 3 cause the Swift 5.9 builds to fail since it doesn't recognize that version.

Copy link
Contributor Author

@andrewdmontgomery andrewdmontgomery Oct 18, 2024

Choose a reason for hiding this comment

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

Note: In addition to the version update, there was one other change in that Package.resolved commit:

{
+  "originHash" : "85a761808437c26b26a29368f9cc9aa509cdd039f95eff656309c72fa6ff2557"
# ...
+  "version" : 3
}

That might be meaningful only in version 3. I left it as a test, and it doesn't seem to cause any issues.

"version" : 2
}
2 changes: 1 addition & 1 deletion Package.swift
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// swift-tools-version: 5.10
// swift-tools-version: 5.9
// The swift-tools-version declares the minimum version of Swift required to build this package.

import PackageDescription
Expand Down
6 changes: 4 additions & 2 deletions Sources/GravatarUI/Base/AssociatedObject.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ import Foundation

func getAssociatedObject<T>(_ object: Any, _ key: UnsafeRawPointer) -> T? {
if #available(iOS 14, *) { // swift 5.3 fixed this issue (https://github.com/apple/swift/issues/46456)
objc_getAssociatedObject(object, key) as? T
// swiftformat:disable:next --redundantReturn
return objc_getAssociatedObject(object, key) as? T
} else {
objc_getAssociatedObject(object, key) as AnyObject as? T
// swiftformat:disable:next --redundantReturn
return objc_getAssociatedObject(object, key) as AnyObject as? T
}
Comment on lines +5 to 10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixed an issue with Swift 5.9. With the implicit return, Swift 5.9 throws a build error.

}

Expand Down
10 changes: 7 additions & 3 deletions fastlane/Fastfile
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
default_platform(:ios)

OS = '17.5'
IPHONE_DEVICE = "iPhone SE (3rd generation) (#{OS})".freeze
IPHONE_MODEL = 'iPhone SE (3rd generation)'
IPHONE_DEVICE = "#{IPHONE_MODEL} (#{OS})".freeze
Comment on lines +6 to +7
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I broke this up into two constants because we often share the model across multiple versions of Xcode (and therefore, across multiple versions of iOS).


PROJECT_ROOT_FOLDER = File.join(File.dirname(File.expand_path(__dir__)), 'Demo')
BUILD_FOLDER = File.join(__dir__, '.build')
Expand Down Expand Up @@ -50,12 +51,15 @@ end

platform :ios do
desc 'Builds the project and runs tests'
lane :test do
lane :test do |device_model: "default", os: "default"|
device_model = IPHONE_MODEL if device_model == "default"
os = OS if os == "default"
Comment on lines +54 to +56
Copy link
Contributor Author

@andrewdmontgomery andrewdmontgomery Oct 18, 2024

Choose a reason for hiding this comment

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

I don't love having to use "default" strings like this. The problem is that matrix values (which are passed into this when CI calls this lane) need to be strings, they can't be null. I thought about using empty strings (""), which would work. But this at least has the benefit of describing what's actually happening: "use the default value".

I'm open to suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah tbh when I first read the default value in the pipeline.yml I thought this was some built-in string that fastlane would understand (e.g. that you'd pass run_tests(…, device: 'default') to fastlane and fastlane would be the one to interpret that value as "use the default simulator"). It's only when I saw the code on these lines 55–56 in your Fastfile that I connected the dots.

I'd also love if it was possible to provide a null value to those parameters in the matrix, but alas, if that's not technically possible then I think default is fine.


run_tests(
package_path: '.',
scheme: 'Gravatar-Package',
xcargs: COMMON_XCARGS,
device: IPHONE_DEVICE,
device: "#{device_model} (#{os})",
prelaunch_simulator: true,
clean: true,
buildlog_path: LOGS_FOLDER,
Expand Down