fix xcode project package resolution when traits are enabled - #14
fix xcode project package resolution when traits are enabled#14lukaskollmer wants to merge 8 commits into
Conversation
Signed-off-by: Lukas Kollmer <hey@lukaskollmer.de>
Signed-off-by: Lukas Kollmer <hey@lukaskollmer.de>
Signed-off-by: Lukas Kollmer <hey@lukaskollmer.de>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Periphery reusable workflow now accepts optional extra build arguments, uses the renamed Xcode version input, clears project-specific DerivedData, and runs scans with clean builds. The Xcode build workflow now resolves dependencies into 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Lukas Kollmer <hey@lukaskollmer.de>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/periphery.yml:
- Around line 67-69: Update the “Clear DerivedData” step to delete only the
workflow-specific .derivedData directory, not the entire user-level Xcode
DerivedData directory. Configure the Periphery/Xcode build settings in this
workflow to use that same .derivedData path consistently.
- Line 80: Update the workflow step using inputs.extra-build-args so it is not
interpolated directly into the shell command. Pass the value through the step’s
env block and parse it as a structured argument list, preferably changing the
input contract to a JSON array, then safely expand the parsed arguments when
invoking the build command while preserving empty-input behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 86c24b61-fb94-47e6-8ecc-488765985449
📒 Files selected for processing (2)
.github/workflows/periphery.yml.github/workflows/xcodebuild.yml
| - name: Clear DerivedData | ||
| run: | | ||
| rm -rf ~/Library/Developer/Xcode/DerivedData/ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Scope DerivedData cleanup to this workflow’s build directory.
Deleting the entire user-level DerivedData directory is unsafe on self-hosted runners: concurrent or unrelated jobs can lose their build artifacts and fail nondeterministically. Remove only the workflow-specific .derivedData directory and configure the Periphery/Xcode build to use that same path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/periphery.yml around lines 67 - 69, Update the “Clear
DerivedData” step to delete only the workflow-specific .derivedData directory,
not the entire user-level Xcode DerivedData directory. Configure the
Periphery/Xcode build settings in this workflow to use that same .derivedData
path consistently.
There was a problem hiding this comment.
Actually probably a fair point @lukaskollmer? We might rather want to make sure each command uses the .derivedData folder?
There was a problem hiding this comment.
huh i was under the impression that a single runner would never run concurrent jobs?
i agree that we want to make sure each command uses the .derivedData folder (and in fact that's what the rest of the PR is about); the specific issue being adressed here is that periphery only uses its own DerivedData folder for some of its work (actually building the app), but it uses the global DerivedData folder when it initially runs xcodebuild -list to query the app's list of schemes. we cannot customize of override this.
the issue here (and this isn't smth theoretical but rather an issue i actually ran into in MHC; hence why this PR exists), is that xcodebuild -list performs package resolution, meaning that it will fail if the package resolution fails.
additionally, for some reason, if the xcode project (e.g. MHC) has a branch-dependency on some package, and there still exists a locally cached checkout in the DerivedData folder, xcodebuild will prefer that over fetching the branch, and will try to resolve the xcodeproj against that.
but if e.g. (and this is the specific issue i was running into) the xcode project now enables a trait on the dependency, but the locally cached version of the dependendy is so outdated that the trait doesn't yet exist there, the xcodebuild -list step in periphery will fail, causing the whole periphery run to fail.
incidentally, the exact same issue also occurred with the xcodebuild -showdestinations step we have in the xcodebuild.yml workflow; the fix there was to explicitly specify the per-run .derivedData folder.
being able to control which DerivedData folder periphery's xcodebuild -list invication uses would fix the issue there as well, but we can't control that so nuking the whole folder seemed like the next-best alternative.
see also here: peripheryapp/periphery#1146
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Yes, I think there are no concurrent runs but ideally we avoid too many caches but seems like that might be hard except if we use xcodebuild first.
There was a problem hiding this comment.
i'll try to scope the deleting (so that eg if we run the periphery workflow for MHC it only does rm -rf ~/Library/Developer/Xcode/DerivedData/MyHeartCounts-*), but i don't think there is another (simple) workaround we could do here right now instead
| -skipPackagePluginValidation \ | ||
| -skipMacroValidation | ||
| -skipMacroValidation \ | ||
| ${{ inputs.extra-build-args }} |
There was a problem hiding this comment.
Could we technically derive them the same way as we do for the main Xcode build?
There was a problem hiding this comment.
what do you mean exactly? what would we derive from where?
i needed to add this mechanism for MHC, in order to make periphery work with the new FHIRModels 0.9.x releases, which fail to link on older apple watch architectures, so what we do now is that we pass extra-build-args: EXCLUDED_ARCHS=armv7k to the periphery workflow as a workaround. this is only an issue for periphery, because all of the other build steps (e.g. when testing or deploying the app) appear to be smart enough to skip unsupported architectures, based on the app's deployment target)
There was a problem hiding this comment.
Makes sense; it's a bit confusing form a naming as we don't pass it into the xcodebuild below; but only with periphery. For xcodebuild we are just using the automatically derived ones.
There was a problem hiding this comment.
sure, but the xcodebuild invocation is a completely separate workflow, with completely separate inputs. we already pass different flags and options into the xcodebuild.yml workflow (eg resultBundle, testPlan, artifactName, scheme, destination, etc) than we pass into the periphery.yml workflow.
within the call site in MHC's static-analysis.yml, the extra-build-args: EXCLUDED_ARCHS=armv7k we pass into periphery.yml has a comment explaining what and why is going on
| default: true | ||
| extra-build-args: | ||
| description: | | ||
| Additional arguments to pass to the underlying build tool (xcodebuild or `swift build`) |
There was a problem hiding this comment.
So far only used for periphery? Is there a chance we can derive them or set reasonable defaults like the derived data folder and others? Ideally the periphery cache should also use that folder and be cleaned up after that?
There was a problem hiding this comment.
periphery doesn't let you control which DerivedData folder it uses, otherwise i would have pointed it to the same (.derivedData) we already use for the rest of the workflows...
There was a problem hiding this comment.
Idea there, what if we just use the normal xcodebuild and then use the build folder that is then generated to run periphery?
--index-store-path can I think read an existing build folder!
There was a problem hiding this comment.
i opened a PR with them (peripheryapp/periphery#1147) that would fix this without us needing to run the build step separately.
since the CI's periphery and test runs are independent jobs, we wouldn't be able to really share this anyway (in the sense of having a single "compile the app" step, and then using the result for all of the other CI stuff).
i'm open to switching to the --index-store-path-based setup, but i first wanna see if they will merge the PR; in that case we wouldn't need to do any workarounds on our end.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/periphery.yml:
- Around line 89-102: Update the DerivedData cleanup loop around the `plist`
check so directories without `info.plist` are skipped as unknown ownership. Only
reach `rm -rf "$dir"` after successfully reading and validating `WorkspacePath`
as belonging to `project_dir`; preserve the existing skip behavior for metadata
that points elsewhere.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b2bbbd8-7151-436f-9f63-b7b6062a0f35
📒 Files selected for processing (1)
.github/workflows/periphery.yml
3 participants
♻️ Current situation & Problem
having the
-showDestinationsstep run before the dependencies have been resolved, and in the global context (instead of the CI-run-specific .derivedData) will cause it to (for some reason) use outdated locally cached checkouts of dependencies. which typically seems to be working fine, actually, but if SPM package traits are involved and the xcode project in question enables package traits that don't yet exist in a locally-cached outdated version of the package, the step will fail.(same for periphery.yml)
⚙️ Release Notes
xcodebuild.ymlandperiphery.yml📚 Documentation
n/a
✅ Testing
n/a
Code of Conduct & Contributing Guidelines
By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: