-
Notifications
You must be signed in to change notification settings - Fork 430
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
Refactor Github Actions to cover more swift versions #858
Conversation
- 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
A couple questions I had on these...
|
IMO just 5.2 and 5.7 is sufficient. Probably best to keep it explicit. |
Well that's weird. Removed Mac Catalyst. All tests pass |
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:
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 |
It looks like the previous |
Ah wonderful. Nothing like a flaky build ...
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 😓 |
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 |
@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. |
Tests passed. But it's not actually using Swift 5.2. It's installing it but still using the built in Swift 5.7 |
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 |
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 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 |
Ok, maybe try the setup-swift action again 👍 |
Also including swift version in cache key
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? 🤞 |
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. |
Nice, CI passed!
Probably.
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. |
.github/workflows/test.yml
Outdated
@@ -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 |
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 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
Ugh. Bummer. Pulled from cache. Build failed 😞 |
Let's get rid of the caching for the Linux jobs then 👍 |
Oh I thought I was an admin, but I guess I'm not so I can't change the required tests. |
@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? 🙏 |
Checklist