Skip to content
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

Update CI for latest macOS and Xcode #75

Merged
merged 1 commit into from
Sep 20, 2024
Merged

Update CI for latest macOS and Xcode #75

merged 1 commit into from
Sep 20, 2024

Conversation

luispadron
Copy link
Collaborator

@luispadron luispadron commented Sep 19, 2024

Updates CI and the build script:

  • Use macos-latest for latest macOS version and toolchain
  • Replace package generate (which no longer exists) with a simple swift build
  • Remove Bazel build support, bazel users should consume via https://registry.bazel.build/modules/rules_swift_package_manager.
  • Remove Xcode job, the pod lib lint job still exists and continues to validate the podspec which is what we care about.
  • Remove build.swift and instead update the github workflow to run the commands via Bash, this is a lot simpler and does the same in less code.

@luispadron luispadron force-pushed the luis/update-ci branch 3 times, most recently from fbc090f to 5e6476a Compare September 19, 2024 18:01
@luispadron luispadron merged commit 77046d2 into master Sep 20, 2024
2 checks passed
Copy link
Collaborator

@dfed dfed left a comment

Choose a reason for hiding this comment

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

I'd recommend reverting this PR + taking another pass. Very down to modernize CI on this repo (and I'm happy to help!) but we really should not be removing all the tests from CI.

Comment on lines -91 to -92
case .xcode:
return "Stagehand Demo App"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@luispadron I appreciate your trying to update CI here, but unless I'm missing something we've made a bit of an oops.

In this PR we've removed the instruction to run the demo app tests, and the demo app is where the tests for this project live.

You mention in the PR description that:

the pod lib lint job still exists and continues to validate the podspec which is what we care about

However, the podspec does not list any tests, and therefore I believe this PR actually removed any validation that Stagehand is behaving correctly. All we're testing in CI now is that Stagehand builds, which is pretty insufficient.

Moreover, by no longer building the sample app, it is now possible for the sample code shipped with Stagehand to not compile, which is less-than-ideal.

Comment on lines -111 to -112
case .xcode:
return true
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can see here that we were only running tests on the Stagehand Demo App, which is no longer built or tested in CI.

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.

3 participants