Refactor Github Actions to cover more swift versions #858

braker1nine · 2022-11-09T18:11:12Z

Test against Xcode 14.1 and 13.4.1
Use setup-swift action to install specific swift versions (replaces swiftenv script)
Test against Swift 5.2 and 5.7

Checklist

Updated CHANGELOG.md.

- For Xcode tests, this runs both 13.4.1 (w/ Swift 5.6) and 14.1 (w/ Swift 5.7) - For SwiftPM on Ubuntu, tests are running against 5.2, 5.6, and 5.7 - Switched to using the setup-swift action instead of swift version manager

braker1nine · 2022-11-09T18:14:27Z

A couple questions I had on these...

Is running against 5.2, 5.6, and 5.7 excessive? Would it be sufficient to just run against 5.2 and 5.7?
We could replace "5.7" with "5", which would use the latest 5.X version. Does that feel more maintainable? Or is it best to keep using an explicit version?

mluisbrown · 2022-11-09T18:29:43Z

Is running against 5.2, 5.6, and 5.7 excessive? Would it be sufficient to just run against 5.2 and 5.7?

IMO just 5.2 and 5.7 is sufficient. Probably best to keep it explicit.

.github/workflows/test.yml

braker1nine · 2022-11-09T22:40:40Z

Well that's weird.

Removed Mac Catalyst. All tests pass
Removed Swift 5.6. Swift 5.2 tests fail

mluisbrown · 2022-11-10T09:04:27Z

All the Apple OS tests are passing. The Linux Swift 5.2 job failed, and therefore the Swift 5.7 job was cancelled. It looks like the Linux Swift 5.2 job failed because of this error which appeared for many files:

/home/runner/work/ReactiveSwift/ReactiveSwift/Sources/Action.swift:1:8: error: missing required module '_SwiftDispatchOverlayShims'
import Dispatch

Could this be related to the change in the way that Swift is being installed on Linux? The only link I found related to this is this, and it's not particularly helpful: https://forums.swift.org/t/error-no-such-module-swiftdispatchoverlayshims/34771

mluisbrown · 2022-11-10T09:18:07Z

It looks like the previous swiftenv solution specifically installed shims and put them on the PATH. If you set the SWIFT_VERSION environment variable before running the swiftenv script it will install a specific version, so you could still use swiftenv to install different versions on Linux.

braker1nine · 2022-11-10T14:18:04Z

Could this be related to the change in the way that Swift is being installed on Linux? The only link I found related to this is this, and it's not particularly helpful: https://forums.swift.org/t/error-no-such-module-swiftdispatchoverlayshims/34771

Ah wonderful. Nothing like a flaky build ...

If you set the SWIFT_VERSION environment variable before running the swiftenv script it will install a specific version, so you could still use swiftenv to install different versions on Linux.

I didn't realize that was a feature of swiftenv. I'll roll it back to that and give it a try. When I was toying around with swiftenv locally I noticed it hadn't been updated in quite a while and it's definitions of swift versions had almost nothing post 5.0 😓

braker1nine · 2022-11-10T14:29:02Z

Maybe it's just generally flaky, but I do wonder if it has something to do with the way the builds are being cached.

Anecdotally, it worked fine when we didn't have a cache key hit the first time. And failed the second time when we did and copied the cached build files in .build

mluisbrown · 2022-11-10T16:45:01Z

@NachoSoto it looks like CI is now passing, but the "expected checks" configured for the repo no longer match the actual checks that are run. I'm not an admin on the repo so I can't change this.

braker1nine · 2022-11-10T16:47:18Z

Tests passed. But it's not actually using Swift 5.2. It's installing it but still using the built in Swift 5.7

mluisbrown · 2022-11-10T17:05:23Z

I have a local Linux VM, I'll see if I can get this working there, selecting the correct version of Swift. Testing this via CI is going to take forever!

braker1nine · 2022-11-10T17:13:16Z

I have a local Linux VM, I'll see if I can get this working there, selecting the correct version of Swift. Testing this via CI is going to take forever!

😅 yeah it is. I was close to commenting out all the other tests until I could get this one working. Thanks @mluisbrown! Ping me if there's anything I can do to help with that

mluisbrown · 2022-11-10T18:18:08Z

Ok, I wasn't able to make any progress. I'm on an M1 Mac and on my Ubuntu VM swiftenv is not getting the correct download links for ARM64 versions of Swift. In any case, I don't think there are ARM64 versions of Swift 5.2 so I wouldn't be able to test it. Maybe using swiftenv was a bad idea, but I can't think of a solution :/

Do we really need to test on Swift 5.2?

braker1nine · 2022-11-10T18:48:01Z

Do we really need to test on Swift 5.2?

Yeah I suppose that's an interesting question. The reason I added it was because the library wasn't successfully building with Swift 5.2. Currently we've got 5.2 listed as the swift tools version in the package manifest. It just seems to me that we should be ensuring that any changes to the library build on whatever swift version we've specified there?

I wonder if it would be worth going back to the setup-swift action and removing the caching to see if it builds more consistently. The caching doesn't seem to be saving a TON of time from what I can tell? And frequently misses

mluisbrown · 2022-11-10T20:02:51Z

Ok, maybe try the setup-swift action again 👍

Also including swift version in cache key

braker1nine · 2022-11-10T21:39:53Z

Just pushed it with setup swift. I thought about this as I was making the change...

The swift version needs to be included in the cache key. I doubt swift would be happy using cache build/dependency files from different versions of Swift? 🤞

braker1nine · 2022-11-10T22:18:57Z

I do wonder if testing on 13.4.1 and 14.1 on Xcode is overkill. The main purpose when I did what was to hit versions of swift with primary associated types and without them. Since we have the linux SPM hitting multiple versions of swift. Maybe having MacOS just hit each target platform would be sufficient.

mluisbrown · 2022-11-11T10:00:35Z

Nice, CI passed!

I do wonder if testing on 13.4.1 and 14.1 on Xcode is overkill

Probably.

Maybe having MacOS just hit each target platform would be sufficient.

I would choose iOS instead of macOS, as it's almost certainly the most used platform for this repo.

In the meantime we need an Admin to alter the expected checks for CI.

mluisbrown · 2022-11-11T15:43:48Z

.github/workflows/test.yml

@@ -8,9 +8,6 @@ jobs:
        fail-fast: false
        matrix:
          destination: [macOS, iOS, tvOS, watchOS]
-          xcode: ["14.1", "13.4.1"]
-    env:
-      DEVELOPER_DIR: /Applications/Xcode_${{ matrix.xcode }}.app/Contents/Developer


I would change this to fix the version of Xcode at 14.1, so that we are always in control of the version, and not caught out if/when GitHub updates the version on CI.

So we could keep:

env: DEVELOPER_DIR: /Applications/Xcode_14.1.app/Contents/Developer

braker1nine · 2022-11-11T17:08:35Z

Ugh. Bummer. Pulled from cache. Build failed 😞

mluisbrown · 2022-11-11T17:16:47Z

Let's get rid of the caching for the Linux jobs then 👍

NachoSoto · 2022-11-11T19:18:30Z

Oh I thought I was an admin, but I guess I'm not so I can't change the required tests.

mluisbrown · 2022-11-11T20:18:34Z

@andersio or @RuiAAPeres can one of you change the CI checks to match the new ones (see the checks that passed) and then merge please? 🙏

braker1nine added 2 commits November 9, 2022 12:59

Increase swift version coverage for unit tests

27b718f

- For Xcode tests, this runs both 13.4.1 (w/ Swift 5.6) and 14.1 (w/ Swift 5.7) - For SwiftPM on Ubuntu, tests are running against 5.2, 5.6, and 5.7 - Switched to using the setup-swift action instead of swift version manager

Add explicit self to fix compilation with Swift 5.2

8976dcf

braker1nine mentioned this pull request Nov 9, 2022

Add primary associated types to SignalProducerConvertible & SignalProducerProtocol #855

Merged

1 task

Remove Mac Catalyst from test destinations

fc81ec8

mluisbrown approved these changes Nov 9, 2022

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

Remove Swift 5.6 from test matrix

bde7bbd

braker1nine added 2 commits November 10, 2022 10:59

Restore swiftenv for installing swift version

13f56e6

Switch swift versions before running

d4675d5

Restore use of setup swift

661f450

Also including swift version in cache key

Remove unneeded env setup from test.yml and update changelog

7a580f1

Roll back to just testing on the latest version of Xcode

15010f9

mluisbrown reviewed Nov 11, 2022

View reviewed changes

Specify Xcode versionin Github actions

8918d54

Remove caching for linux tests

ce1fd8e

NachoSoto approved these changes Nov 11, 2022

View reviewed changes

RuiAAPeres merged commit a5690e6 into ReactiveCocoa:master Nov 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Github Actions to cover more swift versions #858

Refactor Github Actions to cover more swift versions #858

braker1nine commented Nov 9, 2022 •

edited

braker1nine commented Nov 9, 2022

mluisbrown commented Nov 9, 2022

braker1nine commented Nov 9, 2022

mluisbrown commented Nov 10, 2022

mluisbrown commented Nov 10, 2022

braker1nine commented Nov 10, 2022

braker1nine commented Nov 10, 2022

mluisbrown commented Nov 10, 2022

braker1nine commented Nov 10, 2022

mluisbrown commented Nov 10, 2022

braker1nine commented Nov 10, 2022

mluisbrown commented Nov 10, 2022

braker1nine commented Nov 10, 2022

mluisbrown commented Nov 10, 2022

braker1nine commented Nov 10, 2022

braker1nine commented Nov 10, 2022

mluisbrown commented Nov 11, 2022 •

edited

mluisbrown Nov 11, 2022

braker1nine commented Nov 11, 2022

mluisbrown commented Nov 11, 2022

NachoSoto commented Nov 11, 2022

mluisbrown commented Nov 11, 2022

Refactor Github Actions to cover more swift versions #858

Refactor Github Actions to cover more swift versions #858

Conversation

braker1nine commented Nov 9, 2022 • edited

Checklist

braker1nine commented Nov 9, 2022

mluisbrown commented Nov 9, 2022

braker1nine commented Nov 9, 2022

mluisbrown commented Nov 10, 2022

mluisbrown commented Nov 10, 2022

braker1nine commented Nov 10, 2022

braker1nine commented Nov 10, 2022

mluisbrown commented Nov 10, 2022

braker1nine commented Nov 10, 2022

mluisbrown commented Nov 10, 2022

braker1nine commented Nov 10, 2022

mluisbrown commented Nov 10, 2022

braker1nine commented Nov 10, 2022

mluisbrown commented Nov 10, 2022

braker1nine commented Nov 10, 2022

braker1nine commented Nov 10, 2022

mluisbrown commented Nov 11, 2022 • edited

mluisbrown Nov 11, 2022

Choose a reason for hiding this comment

braker1nine commented Nov 11, 2022

mluisbrown commented Nov 11, 2022

NachoSoto commented Nov 11, 2022

mluisbrown commented Nov 11, 2022

braker1nine commented Nov 9, 2022 •

edited

mluisbrown commented Nov 11, 2022 •

edited