-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
ci: add visionOS pod lib lint check #15903
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
- OS: macos-12 | ||
- XCODE: Xcode_14.1 | ||
- PLATFORM: "visionos" | ||
OS: macos-14 |
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.
https://github.com/actions/runner-images seems to say this image is beta, any idea the stability? We won't want something on CI that can't be relied on.
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.
Looks like https://github.com/actions/runner-images/blob/main/images/macos/macos-13-Readme.md#xcode might have Xcode 15.2, so maybe only macos-13 is needed.
But now that I think about it, doesn't a visionOS build really need to be arm? Does Apple fully support the SDK on an Intel based machine?
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.
When I run pod lib lint on my own repo based on macos-13 image and Xcode 15.2,the error's description as [visionOS] unknown: Encountered an unknown error (Could not find a
xros simulator (valid values: ios, tvos, watchos).
appeared also.
See: https://github.com/growingio/growingio-sdk-ios-utilities/actions/runs/7984766703/job/21802111821
And: actions/runner-images#8144 (comment)
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.
In my latest workflow file, there is macos-14
,but macos-14-arm64
get to used actually.
Note: we can not qualify external PRs with workflow changes. If we decide to accept this we'll need to import it to run the tests |
It might make sense to split this PR: one PR for the |
560f05c
to
a8af9da
Compare
LGTM, I split the part of |
@thomasvl Please take a look again when you have time, thanks so much |
I'm going to defer the CI related things to the core protobuf-team @mkruskal-google I'll leave this one up to you. |
DEVELOPER_DIR: /Applications/Xcode_14.1.app/Contents/Developer | ||
include: | ||
- OS: macos-12 | ||
- OS: macos-14 |
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.
Can we bump this down to macos-13? It looks like that supports the xcode version you need, and isn't in beta
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.
When I run pod lib lint on my own repo based on macos-13 image and Xcode 15.2,the error's description as [visionOS] unknown: Encountered an unknown error (Could not find a xros simulator (valid values: ios, tvos, watchos).
appeared also.
See: https://github.com/growingio/growingio-sdk-ios-utilities/actions/runs/7984766703/job/21802111821
And: actions/runner-images#8144 (comment)
steps: | ||
- name: Checkout pending changes | ||
uses: protocolbuffers/protobuf-ci/checkout@v2 | ||
with: | ||
ref: ${{ inputs.safe-checkout }} | ||
- name: Xcode version | ||
if: matrix.OS == 'macos-14' |
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 think it would make sense to keep this unconditional and just pin a version in macos-12
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.
If I'm not misunderstanding your opinion:
This job not pin version in macos-12 before, so it dependency on the default version which the runner providered.
Sry about my poor English.
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 just wanna to do not change the behavior it is used to be, by just add visionOS pod-lib-lint check.
I tried to import this, but it looks like the test is failing: https://github.com/protocolbuffers/protobuf/actions/runs/8025330522/job/21925766125 I suspect this is an issue with our CI repo, e.g. https://github.com/protocolbuffers/protobuf-ci/blob/main/internal/bazel-setup/action.yml#L61. This may take some work |
Thank you about nice changes on #15939, feel ashamed for my lack of knowledge about bazel. |
no worries! thanks for the contribution and sorry for the delay here. I got very sidetracked by our editions launch |
hmm so it looks like we may be hitting the issue described in CocoaPods/CocoaPods#11965 (comment). I'm seeing highly flaky behavior where sometimes it can't find the visionos simulator (but does have |
Yep, sure enough runners using image 20240116.1 are consistently failing now, and ones using 20240219.1 are passing. Will give this some time and then try again :) |
Is adding a step to update CocoaPods within the macos14 runner image like a good idea? Like the earlier solution. |
Yea that's an option, but it would come with increased flakiness and slower builds. Since the rollout of the cocoapods update has already started, I think waiting a week or so for it to finish is the simplest path forward |
as title