[DO NOT MERGE] Introduce swift version testing

[andrewdmontgomery]

Closes #

Description

Testing Steps

[AliSoftware]

The image ID is the name of our VMs, which by convention are named xcode-<xcodeversion>.

Suggested change

	- "15.1"
	- "xcode-15.1"

As of today, we still have the VM templates for the following version in our S3 bucket (not listing betas or rcs but only final ones):

• xcode-15.0.1
• xcode-15.1
• xcode-15.2-xl (-xl suffix because this VM has larger disk space to fix issue we had with out-of-space error on the non-XL VM we created initially)
• xcode-15.3-v3 (-v3 suffix because we had some issues with previous attempts at creating the VM and only got it right on the 3rd iteration)
• xcode-15.4
• xcode-16.0-v7 (-v7 suffix because we had some issues with previous attempts at creating the VM and only got it right on the 7th iteration)

[AliSoftware]

💡 You might want to also include the name of the Xcode version in the label too btw, to differentiate those more easily in the Buildkite UI:

  - label: "📦 Build and Test Swift Package ({{matrix}})"

[mokagio]

This could be DRY-ed via:

Suggested change

	iphone_device = ENV['BUILDKITE_DEVICE'] ? ENV['BUILDKITE_DEVICE'].freeze : IPHONE_DEVICE
	iphone_device = ENV.fetch('BUILDKITE_DEVICE', IPHONE_DEVICE)

Which does the same check with fallback.

[AliSoftware]

One should never use env vars prefixed with BUILDKITE_; see comment above for suggested alternative.

[andrewdmontgomery]

Oof, of course. 🤦

[mokagio]

Interesting, I'd never seen matrix used this way. Usually, I'd expect all value to be specified, e.g.

- $IMAGE_ID
- xcode-15.1

But looks like Buildkite is smart enough (or dynamic enough) to add the image_id from the adjustments to the matrix even though it is not specified. 💡

[andrewdmontgomery]

But looks like Buildkite is smart enough (or dynamic enough) to add the image_id from the adjustments to the matrix even though it is not specified

Yep. Adjustments are added individually, as is. So if you have a matrix with 2 properties (which gives you 2 x 2 permutations), and one "adjustment", you'll have:

2 x 2 + 1 permutations that will run.

I'm not entirely sure I love using matrix like this. It's not exactly how it was intended to be used. Each xcode version has a specific OS version. So the one thing we don't want from a matrix is the actual "matrix" of permutations:

+ xcode-15.4 + 17.5
- xcode-15.4 + 17.2
- xcode-15.1 + 17.5
+ xcode-15.1 + 17.2

Matrix supports skipping some instances. But the more versions of Xcode we need to support, the bigger the matrix will be, the more "skips" we will need to add.

So instead, I went with:

• Matrix of 1 for the "current" version of Xcode (and swift)
• Individual adjustments for each additional Xcode + OS combination

I don't love misusing this feature. If there's another relatively DRY way to handle this, I'm all ears.

[AliSoftware]

I wonder if Buildkite would support not having any entry for setup and everything set via adjustments only?

    matrix:
      adjustments:
        - with:
            image_id: "xcode-15.1"
            device: "iPhone SE (3rd generation) (17.2)"
        - with:
            image_id: "$IMAGE_ID"
            device: "iPhone SE (3rd generation) (17.5)"

Not sure that will work, but worth a try, to make things more "symmetric" in the YAML?

And even if it requires the setup key to be present and wouldn't take adjustments into account if not, keeping everything in adjustments and having an empty setup key (setup: ~¹ or setup: {}) might at least work?

Footnotes

1. ~ represents the null value in YAML ↩

[andrewdmontgomery]

Let me try!

[andrewdmontgomery]

matrix:
  adjustments:
    - with:
        image_id: $IMAGE_ID
        device: "iPhone SE (3rd generation) (17.5)"
    - with:
        image_id: "xcode-15.1"
        device: "iPhone SE (3rd generation) (17.2)"

fatal: Failed to upload and process pipeline: Pipeline upload rejected: 'matrix.setup' can't be blank

matrix:
  setup:
    image_id:
    device:
  adjustments:
    - with:
        image_id: $IMAGE_ID
        device: "iPhone SE (3rd generation) (17.5)"
    - with:
        image_id: "xcode-15.1"
        device: "iPhone SE (3rd generation) (17.2)"

fatal: Failed to upload and process pipeline: Pipeline upload rejected: Each item within a 'matrix' must be either a string, boolean or integer

I know it says "must be either a string, boolean or integer", but just in case I tried setting to null. Same error.

Finally, I tried setting those matrix.setup values to empty strings: "". As you would expect, it has no problem using empty strings to add another permutation to the matrix.

[andrewdmontgomery]

... having an empty setup key (setup: ~ or setup: {}) might at least work?

I missed this. Let me try.

[andrewdmontgomery]

matrix:
  setup: ~
  adjustments: # Specify additional versions of Xcode and devices
  - with:
        image_id: $IMAGE_ID
        device: "iPhone SE (3rd generation) (17.5)"
    - with:
        image_id: "xcode-15.1"
        device: "iPhone SE (3rd generation) (17.2)"

fatal: pipeline parsing of "pipeline.yml" failed: line 35: did not find expected key
Line 35 is the line after setup: ~

matrix:
  setup: {}
  adjustments: # Specify additional versions of Xcode and devices
  - with:
        image_id: $IMAGE_ID
        device: "iPhone SE (3rd generation) (17.5)"
    - with:
        image_id: "xcode-15.1"
        device: "iPhone SE (3rd generation) (17.2)"

fatal: pipeline parsing of "pipeline.yml" failed: line 35: did not find expected key

[andrewdmontgomery]

It doesn't look like there is any version of leaving the matrix.setup blank and including everything in the adjustments. So I added some comments that should at least clarify how to use it:

matrix:
  setup: # Specify the current version of Xcode and device
    image_id: $IMAGE_ID
    device: "iPhone SE (3rd generation) (17.5)"
  adjustments: # Specify additional versions of Xcode and devices
    - with:
        image_id: "xcode-15.1"
        device: "iPhone SE (3rd generation) (17.2)"

[AliSoftware]

😢 How unfortunate.

I guess your initial approach will have to do then.

You should be able to get away with not making the image_id and device entries of setup be Arrays though, but instead pass them as simple strings instead.
I just checked with buildkite-agent pipeline upload --dry-run from my machine and confirmed it transforms that single string from image_id into an array with single element when parsing the pipeline before uploading it, so that should work, and remove one level of indentation.
Not perfect, and that's just nitpicking at that point, but we take what we can 😂

    matrix:
      setup:
        image_id: "$IMAGE_ID"
        device: "iPhone SE (3rd generation) (17.5)"
      adjustments:
        - with:
            image_id: "xcode-15.1"
            device: "iPhone SE (3rd generation) (17.2)"

[AliSoftware]

Ah, I see you posted some more comments since I started writing my reply above, and you already figured out that you could pass a single String instead of Array for each param in setup 👍 :jinx:

[AliSoftware]

⚠️ We should always avoid using env vars that are prefixed with BUILDKITE_, as those are reserved by Buildkite itself, and defining our own could mess up the CI infra.

You should instead pass the device as a parameter to the command called by this pipeline, i.e. command: validate_swift_package test device:"{{matrix.device}}", and then have your test lane take a lane :test |device:| do parameter

[andrewdmontgomery]

This is the direction I went first, but ran into trouble. Must have been a typo somewhere. I'll revert and go that route.

And yeah, thanks for the warning about the ENV prefix. Of course!

[AliSoftware]

This is the direction I went first, but ran into trouble. Must have been a typo somewhere.

I wonder if you encountered this current technical limitation of our Mac infra where env vars (other than BUILDKITE_*) are not passed from the Mac hosts (physical MacMinis) to the VMs (where Xcode is installed and the actual build is run) 🤔
[EDIT]Actually, that's unlikely the issue you ran into, so nvm[/EDIT]

[EDIT]
Looking at this commit I see your initial try was calling validate_swift_package device:"{{matrix.device}}". But validate_swift_package expects you to provide the whole list of parameters ("$@") to pass to bundle exec fastlane, so you need to also provide the name of the lane (test) before the parameters to pass to that lane.

It's only if you don't pass any additional argument to validate_swift_package that it defaults to running the lane called test, but as soon as you pass explicit arguments you are expected to provide everything.

[andrewdmontgomery]

Yeah, that was just me forgetting.

[andrewdmontgomery]

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.

[andrewdmontgomery]

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.

[andrewdmontgomery]

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.

[andrewdmontgomery]

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.

[andrewdmontgomery]

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

[andrewdmontgomery]

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).

[andrewdmontgomery]

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.

[AliSoftware]

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.

[AliSoftware]

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.

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.

[AliSoftware]

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:

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.

[andrewdmontgomery]

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.

[AliSoftware]

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.

[AliSoftware]

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?