Skip to content

Modernize CI#300

Merged
dfed merged 19 commits into
square:masterfrom
jshier:modernize-ci
Jun 9, 2023
Merged

Modernize CI#300
dfed merged 19 commits into
square:masterfrom
jshier:modernize-ci

Conversation

@jshier

@jshier jshier commented Jun 5, 2023

Copy link
Copy Markdown
Collaborator

This PR starts modernizing the project's CI integration, starting with updating the platform support.

@CLAassistant

CLAassistant commented Jun 5, 2023

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@dfed dfed left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you for the update!

Comment thread Scripts/build.swift
Comment thread Sources/LegacyValet/VALLegacySecureEnclaveValet.m
Comment thread .github/workflows/ci.yml
@dfed

dfed commented Jun 5, 2023

Copy link
Copy Markdown
Collaborator

Unit tests failing due testEnvironmentSupportsWhenPasscodeSet not being updated to include newer OS versions.

Xcode 12 hanging is likely the same as this issue

@codecov-commenter

codecov-commenter commented Jun 5, 2023

Copy link
Copy Markdown

Codecov Report

Merging #300 (17028bb) into master (4ca4f30) will decrease coverage by 6.10%.
The diff coverage is n/a.

❗ Current head 17028bb differs from pull request most recent head 3eab0a4. Consider uploading reports for the commit 3eab0a4 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #300      +/-   ##
==========================================
- Coverage   86.81%   80.72%   -6.10%     
==========================================
  Files          16       15       -1     
  Lines         986      887      -99     
==========================================
- Hits          856      716     -140     
- Misses        130      171      +41     

see 4 files with indirect coverage changes

@jshier

jshier commented Jun 5, 2023

Copy link
Copy Markdown
Collaborator Author

@dfed Are you okay dropping any versions of Xcode in the CI matrix? Or do you want to keep going with Xcode 11?

@dfed

dfed commented Jun 5, 2023

Copy link
Copy Markdown
Collaborator

@dfed Are you okay dropping any versions of Xcode in the CI matrix? Or do you want to keep going with Xcode 11?

I'd hate to lose building for macOS 10.15 😕. That said... we are a bit long overdue for a breaking version bump. Could be nice to drop support for watchOS 2-3, tvOS 9-11, iOS 9-11, and macOS 10.11-10.13. Only issue there is we can't set our minimum Xcode version to something recent and still test older simulators in CI.

Regarding the current failing tests... seems like tests protected by testEnvironmentIsSigned() are failing due to an entitlement error. This is new/exciting, and may not be easy to fix 😬

@jshier

jshier commented Jun 5, 2023

Copy link
Copy Markdown
Collaborator Author

Regarding the current failing tests... seems like tests protected by testEnvironmentIsSigned() are failing due to an entitlement error. This is new/exciting, and may not be easy to fix 😬

I hoped that those failures were dealt with in the existing CI infrastructure, but I guess not. I'll take a look.

@jshier

jshier commented Jun 5, 2023

Copy link
Copy Markdown
Collaborator Author

I'd hate to lose building for macOS 10.15 😕.

I can keep it, but technically that image is deprecated by GitHub, so I don't know how much longer it'll exist at all.

@dfed

dfed commented Jun 5, 2023

Copy link
Copy Markdown
Collaborator

I hoped that those failures were dealt with in the existing CI infrastructure, but I guess not. I'll take a look.

Thank you! In the past we've avoided running unit tests that required entitlements in CI. We may need to take the same approach here, at which point we'd rename isTestEnvironmentSigned() to something like testEnvironmentSupportsEntitlements() or some such and update the check. Valet is the kind of repo that, unfortunately, really requires local testing during development.

I can keep it, but technically that image is deprecated by GitHub, so I don't know how much longer it'll exist at all.

Oh! That's information I didn't have. If those images are indeed deprecated and slated for removal then yeah we can remove them. cc @efirestone

@jshier

jshier commented Jun 5, 2023

Copy link
Copy Markdown
Collaborator Author

What's the recommended way to test locally with the proper entitlements?

@dfed

dfed commented Jun 5, 2023

Copy link
Copy Markdown
Collaborator

What's the recommended way to test locally with the proper entitlements?

Our Contributing.md file outlines how to test with local entitlements on macOS – if iOS now requires testing on device as of Xcode 14, you'll need to go through similar steps on iOS. Though note that the iOS Host Application is already set.

@jshier

jshier commented Jun 6, 2023

Copy link
Copy Markdown
Collaborator Author

@dfed I updated the condition and the tests pass locally without entitlements, so I'd like to try it here before I full rename and add all the additional CI runs.

@dfed dfed left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Really appreciate your progress here.

Comment thread Tests/ValetIntegrationTests/ValetIntegrationTests.swift
Comment thread Tests/ValetIntegrationTests/ValetIntegrationTests.swift
Comment thread .github/workflows/ci.yml
@jshier

jshier commented Jun 7, 2023

Copy link
Copy Markdown
Collaborator Author

Odd, I pushed changes but they aren't being reflected here. GitHub issues I guess, but I think we're at a good point once those changes are in. I moved the Carthage and CocoaPods runners to use Xcode 14.3, do you want some dedicated Xcode 11 tests instead?

@jshier

jshier commented Jun 7, 2023

Copy link
Copy Markdown
Collaborator Author

Ah, looks like Xcode 14.3 removed generate-xcodeproj (it'd been deprecated for a while), so a new way to run those SPM tests is needed.

@dfed

dfed commented Jun 7, 2023

Copy link
Copy Markdown
Collaborator

I moved the Carthage and CocoaPods runners to use Xcode 14.3, do you want some dedicated Xcode 11 tests instead?

What you've done here makes sense to me. However, it seems our supporting absurdly old iOS versions is causing our Carthage build to fail with modern Xcode. Oof.

I am leaning towards ripping off the bandaid and rolling a major version bump for this. Let's drop support for anything prior to iOS 11, tvOS 11, watchOS 4, and macOS 10.13 and update the README to reflect that.

I'm also seeing an error that swift package generate-xcodeproj was removed in Xcode 14... which... ugh. To fix that, we're going to:

  1. Make shouldGenerateXcodeProject return false for anything running on Xcode 14+
  2. Remove the "-project", task.project, arguments from the xcodeBuildArguments list on Xcode 14+

I think that'll do it given a similar script I have in another repo.

@jshier

jshier commented Jun 7, 2023

Copy link
Copy Markdown
Collaborator Author

I'm also seeing an error that swift package generate-xcodeproj was removed in Xcode 14... which... ugh. To fix that, we're going to:

  1. Make shouldGenerateXcodeProject return false for anything running on Xcode 14+
  2. Remove the "-project", task.project, arguments from the xcodeBuildArguments list on Xcode 14+

One other wrinkle I recall is that xcodebuild doesn't support building Xcode projects and SPM projects out of the same directory. So to build the package using xcodebuild we'd have to first delete the project file, otherwise the schemes can't be properly looked up.

@dfed

dfed commented Jun 7, 2023

Copy link
Copy Markdown
Collaborator

I'm also seeing an error that swift package generate-xcodeproj was removed in Xcode 14... which... ugh. To fix that, we're going to:

  1. Make shouldGenerateXcodeProject return false for anything running on Xcode 14+
  2. Remove the "-project", task.project, arguments from the xcodeBuildArguments list on Xcode 14+

One other wrinkle I recall is that xcodebuild doesn't support building Xcode projects and SPM projects out of the same directory. So to build the package using xcodebuild we'd have to first delete the project file, otherwise the schemes can't be properly looked up.

welp. I'd be fine moving the xcodeproj/xcworkspace or Package.swift into a new directory. I think I'd prefer moving the xcodeproj/xcworkspace?

@jshier

jshier commented Jun 7, 2023

Copy link
Copy Markdown
Collaborator Author

welp. I'd be fine moving the xcodeproj/xcworkspace or Package.swift into a new directory. I think I'd prefer moving the xcodeproj/xcworkspace?

I don't think it can work any other way, given Xcode / SPM doesn't support nested package files AFAIK.

Comment thread Sources/Valet/SinglePromptSecureEnclaveValet.swift

@dfed dfed left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looking good!

Comment thread Scripts/build.swift Outdated
Comment thread Scripts/build.swift Outdated
Comment thread Scripts/build.swift
Comment thread Sources/LegacyValet/VALLegacySecureEnclaveValet.m
Comment thread .github/workflows/ci.yml
Co-authored-by: Dan Federman <dfed@me.com>
@dfed

dfed commented Jun 9, 2023

Copy link
Copy Markdown
Collaborator

Thanks for the work here @jshier! Merging 😄

@dfed dfed merged commit 317addb into square:master Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants