Update CI to run on macOS 11 by dfed · Pull Request #63 · dfed/CacheAdvance

dfed · 2022-09-02T17:53:05Z

macOS 10.15 is no longer supported by Github Actions. So instead let's run CI on macOS 11 and macOS 12.

While I was here, I also added Xcode 13 CI runs, and resolved #44

dfed · 2022-09-02T18:44:46Z

+        // Set the configuration to be release – this configuration is not supported when running xcodebuild without a xcodeproj file.
+        xcodeBuildArguments.append("-configuration")
+        xcodeBuildArguments.append("Release")


This configuration only existing when we have an xcodeproj file sucks. I managed to continue testing in Release in CI by using swift test -c release -Xswiftc -enable-testing. We're still running via xcodebuild for test coverage on Xcode 13, but that run is in debug rather than Release.

dfed · 2022-09-02T18:45:07Z

+             .tvOS_15,
+             .macOS_12,
+             .watchOS_8:
+            // Xcode 13 does not require xcodeproj generation


fun fact: we can run xcodebuild without an xcodeproj on Xcode 13!

Interesting!

dfed · 2022-09-02T18:45:37Z

+  spm-13-swift:
+    name: Swift Build Xcode 13
+    runs-on: macOS-12
+    steps:
+      - name: Checkout Repo
+        uses: actions/checkout@v2
+      - name: Bundle Install
+        run: bundle install
+      - name: Select Xcode Version
+        run: sudo xcode-select --switch /Applications/Xcode_13.4.1.app/Contents/Developer
+      - name: Build and Test Framework
+        run: swift test -c release -Xswiftc -enable-testing


These new lines resolve #44

Qing has told me to always use xcrun swift for these things which IIRC it's so we use the swift version from Xcode rather than the one installed in the system.

dfed · 2022-09-02T18:47:48Z

          'iOS_12,tvOS_12',
          'iOS_13,tvOS_13,watchOS_6',
-          'macOS_10_15'
+          'macOS_10_15',


Fun fact, we can still run tests against the macOS 10.15 SDK even while running on macOS 11. We can only do this while running on Xcode 11.

dfed · 2022-09-02T18:48:14Z

      matrix:
        platforms: [
          'iOS_14,tvOS_14,watchOS_7',
+          'macOS_11',


Now that we're running on macOS 11 and targeting Xcode 12 we can run tests against the macOS 11 SDK!

dfed · 2022-09-02T18:53:31Z

    strategy:
      matrix:
        platforms: [
-          'iOS_12,tvOS_12',


This sucks, but Github Actions' macOS 11 install does not have iOS or tvOS 12 simulators

dfed · 2022-09-02T19:02:34Z

    case tvOS_14
+    case tvOS_15
    case macOS_10_15
-    case watchOS_5


I don't know why watchOS_5 was here: we weren't running tests on it!

fdiaz · 2022-09-02T19:10:49Z

+      - name: Bundle Install
+        run: bundle install
+      - name: Select Xcode Version
+        run: sudo xcode-select --switch /Applications/Xcode_13.4.1.app/Contents/Developer


JFYI xcversion comes installed in these machines, which would make selecting an Xcode much easier than this https://github.com/actions/runner-images/blob/main/images/macos/macos-11-Readme.md#xcode-support-tools

Nice! de95961

dfed · 2022-09-02T21:10:10Z

-        let decodedSize = withUnsafePointer(to: data) {
-            return UnsafeRawBufferPointer(start: $0, count: Self.storageLength)
+        let decodedSize = data.withUnsafeBytes {
+            return $0.load(as: Self.self)
        }
-        self = Self.swapToHost(decodedSize.load(as: Self.self))
+        self = Self.swapToHost(decodedSize)


Woof. I don't understand why this change was necessary to get CI passing in Release on Xcode 13 – Debug worked fine.

The good news is the only tests that were failing were:

-[CacheAdvanceTests.BigEndianHostSwappableTests test_init_canBeInitializedFromEncodedData] -[CacheAdvanceTests.EncodableMessageTests test_encodedData_encodesCorrectSize]

These tests are effectively testing implementation details – the integration tests where we persist messages and de-persist them were still succeeding.

So, what does this change do? Instead of creating a buffer pointer from the data's pointer, I instead access the underlying data's bytes directly. Skipping the translation to a buffer pointer simplifies what's happening here, which fixed the underlying issue. Skipping the translation buffer likely also gave us a very small performance win.

Edit: I DO understand why this changed worked. See explanation above.

fdiaz · 2022-09-02T22:20:21Z

    ///
    /// - Parameter data: A data blob representing encodable data. Must be of length `Self.storageLength`.
    init(_ data: Data) {
-        let decodedSize = withUnsafePointer(to: data) {


I don't have enough context to review these changes but everything else LGTM!

I am confident in these changes (we have excellent test coverage), so I'm okay merging this in.

What's happening in this method is we're instantiating a BigEndianHostSwappable (i.e. a UInt64, UInt32, or UInt8) from a Data blob.

The way we did that was:

Get a pointer to the underlying data (withUnsafePointer(to: data) { ... })

Turn that pointer into a buffer pointer (i.e. a pointer range) starting at the pointer and ending at the length of storage required to store this type (UnsafeRawBufferPointer(start: $0, count: Self.storageLength))

Using that buffer pointer to load the underlying BigEndianHostSwappable, i.e. read the pointer buffer from memory and cast it as the current type. (decodedSize.load(as: Self.self))

Swap the read-in value to have the same Endian-ness as the current machine – this is only necessary if the file has been transferred between machines of different Endian-ness. (Self.swapToHost(...))

The way we're doing it now is:

Get the underlying buffer pointer for the underlying data (withUnsafeBytes { ... })

Using that buffer pointer (i.e. a pointer range) to load the underlying BigEndianHostSwappable, i.e. read the pointer buffer from memory and cast it as the current type. ($0.load(as: Self.self))

Swap the read-in value to have the same Endian-ness as the current machine – this is only necessary if the file has been transferred between machines of different Endian-ness. (Self.swapToHost(...))

The issue with the old code (which of course I figured out by typing all of this out) was that we were working over the unsafe buffer pointer range outside of the withUnsafePointer call. If I change this code to read:

let decodedSize = withUnsafePointer(to: data) { return UnsafeRawBufferPointer(start: $0, count: Self.storageLength).load(as: Self.self) }

The tests pass in Release! So changing from withUnsafePointer to withUnsafeBytes wasn't the issue. Instead, it was where we were interacting with the buffer! That said, I do like the semantics of withUnsafeBytes better.

To make sure I follow, the issue was that we were violating this requirement?

dfed · 2022-09-03T00:00:32Z

I have made all the new CI checks required. Merging!

bachand

I reviewed the changes to BigEndianHostSwappable. Thanks for the explanation Dan!

dfed added 3 commits September 2, 2022 10:51

Update CI to run on macOS 11

df90099

Update build script to run on more platforms

f0b0fa1

Run CI on more platforms

1337c18

dfed commented Sep 2, 2022

View reviewed changes

Github actions does not support iOS or tvOS 12

87013a3

dfed commented Sep 2, 2022

View reviewed changes

We don't run tests on watchOS 5

b91a441

dfed commented Sep 2, 2022

View reviewed changes

fdiaz reviewed Sep 2, 2022

View reviewed changes

fdiaz approved these changes Sep 2, 2022

View reviewed changes

dfed commented Sep 2, 2022

View reviewed changes

Comment thread .github/workflows/ci.yml Outdated

dfed and others added 3 commits September 2, 2022 12:23

Use the 'swift' from the Xcode install

05e3517

Fix copy/paste

3a56412

Get CI passing on Xcode 13 in Release

aaab74d

dfed force-pushed the dfed--update-ci branch from e366e6e to aaab74d Compare September 2, 2022 21:04

dfed requested a review from fdiaz September 2, 2022 21:06

dfed marked this pull request as ready for review September 2, 2022 21:06

dfed requested a review from bachand September 2, 2022 21:07

dfed commented Sep 2, 2022

View reviewed changes

Use xcversion

de95961

fdiaz reviewed Sep 2, 2022

View reviewed changes

fdiaz approved these changes Sep 2, 2022

View reviewed changes

dfed merged commit 944702f into main Sep 3, 2022

dfed deleted the dfed--update-ci branch September 3, 2022 00:01

bachand reviewed Sep 3, 2022

View reviewed changes

Uh oh!

Conversation

dfed commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dfed Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfed commented Sep 3, 2022

Uh oh!

bachand left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dfed commented Sep 2, 2022 •

edited

Loading

dfed Sep 2, 2022 •

edited

Loading