-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add StagehandTesting to SPM package, update CI #76
Conversation
baab24e
to
2ac4019
Compare
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.
Per my comment on #75, it'd be good to re-add unit tests to CI. Could maybe be part of this PR?
Ah my bad, yes I can look at getting tests added via the SPM workflow / pod lib lint. |
2ac4019
to
8b79a7f
Compare
@dfed see #77 as to why re-adding example CI is out-of-scope for this PR. I hope ill have time to get the examples updated but right now GitHub actions do not support the iOS versions / Xcode versions the example tests require. I'm focused on adding better support for SPM as that's what were moving to internally. |
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.
Change here generally makes sense to me, but getting CI's pod linting in sync with the Cocoapods version in Gemfile.lock is blocking imo.
.github/workflows/ci.yml
Outdated
- name: Lint StagehandTesting Podspec | ||
run: bundle exec --gemfile=Example/Gemfile pod lib lint --verbose --fail-fast --include-podspecs=Stagehand.podspec StagehandTesting.podspec | ||
- name: Lint podspecs | ||
run: pod lib lint --verbose --fail-fast --include-podspecs=Stagehand.podspec StagehandTesting.podspec |
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.
why do we not want bundle exec
here? We should always ensure we're using the same cocoapods version to lint as the one we're going to publish with.
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 had removed that because the root of the repo doesnt have a Gemfile
and it was referring to Example
here which i was going to address build & testing in #77. I added back the bundle exec
until we add a root Gemfile
swift build \ | ||
--sdk "$(xcode-select -p)/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk" \ | ||
--triple "arm64-apple-ios13.0" | ||
# select minimum supported Xcode version |
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.
future-looking comment:
we have some prior art for dropping Xcode version support in minor version bumps. We also do not state the minimum required Xcode version in this project's README 😅 so there's no real contract to break. (maybe we should fix that last part, but... that also doesn't need to be today)
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.
The comment is referring to the minimum Xcode version on macos-latest
so we can target the earliest iOS version on those workers, but Stagehand still supports iOS 12 and that hasn't changed. Updating the Xcode version used in CI for builds/tests shouldn't effect semver afaik
@@ -1,4 +1,4 @@ | |||
// swift-tools-version:5.0.1 | |||
// swift-tools-version:5.8 |
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.
🎉
.library( | ||
name: "StagehandTesting", | ||
targets: ["StagehandTesting"] | ||
), |
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.
love that this is getting exposed via SPM now!
5345169
to
0cf0c9d
Compare
Co-authored-by: Dan Federman <[email protected]>
0cf0c9d
to
407c5cc
Compare
Exposes StagehandTesting via the
Package.swift
. Cleans up some more Bazel references and updates CI to test SPM viaxcodebuild
which allows building the package targets but with the correct platforms. Usingswift build
it was trying to link the macOS version of XCTest even though i was specifying the iOS sdk 🤷🏼