Refactor install_swiftpm_dependencies to allow specifying workspace/scheme/project
#105
Conversation
bin/install_swiftpm_dependencies
Outdated
| if [[ -z "${SCHEME_NAME}" ]]; then | ||
| SCHEME_NAME=$(xcodebuild -workspace "${XCWORKSPACE_PATH}" -onlyUsePackageVersionsFromResolvedFile -skipPackageUpdates -list | sed -n '/Schemes:/{n;s/^ *//p;}') | ||
| echo "No \`--scheme\` parameter provided; assuming the first one found (as it shouldn't matter in practice?): \`${SCHEME_NAME}\`" | ||
| fi | ||
| echo "Using -workspace \"${XCWORKSPACE_PATH}\" -scheme \"${SCHEME_NAME}\"" | ||
| xcodebuild -workspace "${XCWORKSPACE_PATH}" -scheme "${SCHEME_NAME}" -resolvePackageDependencies -onlyUsePackageVersionsFromResolvedFile |
There was a problem hiding this comment.
💡 🤞 I'll have to do some more tests(†) to validate this, but from my quick experiments it seems that when I ran xcodebuild -workspace "${XCWORKSPACE_PATH}" -list (initially to guess a scheme to use)… xcodebuild printed "Resolve Package Graph", suggesting that it was resolving dependencies first before finally printing the list of schemes…
Which might mean that we could use this to our advantage, to just pass -list instead of having to pass an explicit -scheme NAME (i.e. xcodebuild -workspace -resolvePackageDependencies -onlyUsePackageVersionsFromResolvedFile -list)… which would completely avoid the need to pass a -scheme to the command in the first place (just because of the bug of xcodebuild requiring -scheme [or -list] to be provided when -workspace is also provided… even in cases where the scheme doesn't matter, like iinm in the case of resolving dependencies)
(†) we should test this theory thoroughly though, especially with some edge cases—like deliberately have an outdated Package.resolved pointing to older versions, and/or ensuring the packages are not present yet locally, before running those commands—to check especially if the -onlyUsePackageVersionsFromResolvedFile is honored by xcodebuild (as opposed to being ignored when we use -list instead of an explicit -scheme NAME).
But if this ends up behaving like I hope, this means that we could simplify the whole code of this bash script by:
- Removing the
--schemeinput parameter parsing from this utility - Directly invoke
xcodebuild -workspace "${XCWORKSPACE_PATH}" -resolvePackageDependencies -onlyUsePackageVersionsFromResolvedFile -list—in order to resolve the Swift Dependencies withxcodebuildwhile still ensuring it'd use the.xcworkspaceand not.xcodeproj, yet without having to provide a-schemeoption
There was a problem hiding this comment.
but from my quick experiments it seems that when I ran xcodebuild -workspace "${XCWORKSPACE_PATH}" -list (initially to guess a scheme to use)… xcodebuild printed "Resolve Package Graph", suggesting that it was resolving dependencies first before finally printing the list of schemes…
That's what I observe on my end, too. In Pocket Casts iOS:
➜ xcodebuild -workspace "podcasts.xcworkspace" -list
Command line invocation:
/Applications/Xcode-16.0.0-Beta.3.app/Contents/Developer/usr/bin/xcodebuild -workspace podcasts.xcworkspace -list
User defaults from command line:
IDEPackageSupportUseBuiltinSCM = YES
Resolve Package Graph
Resolved source packages:
AutomatticTracksiOS: https://github.com/Automattic/Automattic-Tracks-iOS @ 3.4.2
SwiftProtobuf: https://github.com/apple/swift-protobuf.git @ 1.22.1
GTMAppAuth: https://github.com/google/GTMAppAuth.git @ 4.1.1
Kingfisher: https://github.com/onevcat/Kingfisher @ 7.10.2
TinyCSV: https://github.com/dagronf/TinyCSV @ 1.0.0
JLRoutes: https://github.com/joeldev/JLRoutes @ 2.1.1
Agrume: https://github.com/Automattic/Agrume @ 5.6.13
Sentry: https://github.com/getsentry/sentry-cocoa @ 8.29.1
SwiftSubtitles: https://github.com/dagronf/SwiftSubtitles @ 1.5.2
DSFRegex: https://github.com/dagronf/DSFRegex @ 3.4.0
Firebase: https://github.com/firebase/firebase-ios-sdk.git @ 10.25.0
GoogleSignIn: https://github.com/google/GoogleSignIn-iOS @ 7.1.0
abseil: https://github.com/google/abseil-cpp-binary.git @ 1.2024011601.1
AppAuth: https://github.com/openid/AppAuth-iOS.git @ 1.7.5
DifferenceKit: https://github.com/ra1028/DifferenceKit @ 1.2.0
UIDeviceIdentifier: https://github.com/squarefrog/UIDeviceIdentifier @ 2.1.0
AppCheck: https://github.com/google/app-check.git @ 10.19.0
leveldb: https://github.com/firebase/leveldb.git @ 1.22.2
GoogleAppMeasurement: https://github.com/google/GoogleAppMeasurement.git @ 10.25.0
InteropForGoogle: https://github.com/google/interop-ios-for-google-sdks.git @ 100.0.0
BuildkiteTestCollector: https://github.com/buildkite/test-collector-swift @ 0.1.1
GoogleUtilities: https://github.com/google/GoogleUtilities.git @ 7.12.1
Lottie: https://github.com/airbnb/lottie-ios.git @ 4.4.0
Promises: https://github.com/google/promises.git @ 2.3.1
PocketCastsServer: /Users/gio/Developer/a8c/pcios/Modules/Server
GoogleDataTransport: https://github.com/google/GoogleDataTransport.git @ 9.3.0
PocketCastsDataModel: /Users/gio/Developer/a8c/pcios/Modules/DataModel
Sodium: https://github.com/jedisct1/swift-sodium @ 0.9.1
gRPC: https://github.com/google/grpc-binary.git @ 1.62.2
nanopb: https://github.com/firebase/nanopb.git @ 2.30909.0
Fuse: https://github.com/krisk/fuse-swift @ 1.4.0
FMDB: https://github.com/ccgus/fmdb.git @ 2.7.8
GTMSessionFetcher: https://github.com/google/gtm-session-fetcher.git @ 3.4.1
PocketCastsUtils: /Users/gio/Developer/a8c/pcios/Modules/Utils
SwipeCellKit: https://github.com/shiftyjelly/SwipeCellKit @ 2.7.7
Information about workspace "podcasts":
Schemes:
BuildkiteTestCollector
DSFRegex
google-cast-sdk-no-bluetooth
NotificationContent
NotificationExtension
Pocket Casts Prototype Build
Pocket Casts Staging
Pocket Casts Watch App
pocketcasts
PocketCastsDataModel
PocketCastsServer
PocketCastsUtils
PodcastsIntents
PodcastsIntentsUI
Pods-PocketCastsTests
Pods-podcasts
PreBuildActions
PulseCore
PulseUI
Screenshot Automation
Screenshot Automation Watch
Share Extension
SwiftGen
SwiftLint
WidgetExtension
bin/install_swiftpm_dependencies
Outdated
| if [[ -z "${SCHEME_NAME}" ]]; then | ||
| SCHEME_NAME=$(xcodebuild -workspace "${XCWORKSPACE_PATH}" -onlyUsePackageVersionsFromResolvedFile -skipPackageUpdates -list | sed -n '/Schemes:/{n;s/^ *//p;}') | ||
| echo "No \`--scheme\` parameter provided; assuming the first one found (as it shouldn't matter in practice?): \`${SCHEME_NAME}\`" | ||
| fi | ||
| echo "Using -workspace \"${XCWORKSPACE_PATH}\" -scheme \"${SCHEME_NAME}\"" | ||
| xcodebuild -workspace "${XCWORKSPACE_PATH}" -scheme "${SCHEME_NAME}" -resolvePackageDependencies -onlyUsePackageVersionsFromResolvedFile |
There was a problem hiding this comment.
but from my quick experiments it seems that when I ran xcodebuild -workspace "${XCWORKSPACE_PATH}" -list (initially to guess a scheme to use)… xcodebuild printed "Resolve Package Graph", suggesting that it was resolving dependencies first before finally printing the list of schemes…
That's what I observe on my end, too. In Pocket Casts iOS:
➜ xcodebuild -workspace "podcasts.xcworkspace" -list
Command line invocation:
/Applications/Xcode-16.0.0-Beta.3.app/Contents/Developer/usr/bin/xcodebuild -workspace podcasts.xcworkspace -list
User defaults from command line:
IDEPackageSupportUseBuiltinSCM = YES
Resolve Package Graph
Resolved source packages:
AutomatticTracksiOS: https://github.com/Automattic/Automattic-Tracks-iOS @ 3.4.2
SwiftProtobuf: https://github.com/apple/swift-protobuf.git @ 1.22.1
GTMAppAuth: https://github.com/google/GTMAppAuth.git @ 4.1.1
Kingfisher: https://github.com/onevcat/Kingfisher @ 7.10.2
TinyCSV: https://github.com/dagronf/TinyCSV @ 1.0.0
JLRoutes: https://github.com/joeldev/JLRoutes @ 2.1.1
Agrume: https://github.com/Automattic/Agrume @ 5.6.13
Sentry: https://github.com/getsentry/sentry-cocoa @ 8.29.1
SwiftSubtitles: https://github.com/dagronf/SwiftSubtitles @ 1.5.2
DSFRegex: https://github.com/dagronf/DSFRegex @ 3.4.0
Firebase: https://github.com/firebase/firebase-ios-sdk.git @ 10.25.0
GoogleSignIn: https://github.com/google/GoogleSignIn-iOS @ 7.1.0
abseil: https://github.com/google/abseil-cpp-binary.git @ 1.2024011601.1
AppAuth: https://github.com/openid/AppAuth-iOS.git @ 1.7.5
DifferenceKit: https://github.com/ra1028/DifferenceKit @ 1.2.0
UIDeviceIdentifier: https://github.com/squarefrog/UIDeviceIdentifier @ 2.1.0
AppCheck: https://github.com/google/app-check.git @ 10.19.0
leveldb: https://github.com/firebase/leveldb.git @ 1.22.2
GoogleAppMeasurement: https://github.com/google/GoogleAppMeasurement.git @ 10.25.0
InteropForGoogle: https://github.com/google/interop-ios-for-google-sdks.git @ 100.0.0
BuildkiteTestCollector: https://github.com/buildkite/test-collector-swift @ 0.1.1
GoogleUtilities: https://github.com/google/GoogleUtilities.git @ 7.12.1
Lottie: https://github.com/airbnb/lottie-ios.git @ 4.4.0
Promises: https://github.com/google/promises.git @ 2.3.1
PocketCastsServer: /Users/gio/Developer/a8c/pcios/Modules/Server
GoogleDataTransport: https://github.com/google/GoogleDataTransport.git @ 9.3.0
PocketCastsDataModel: /Users/gio/Developer/a8c/pcios/Modules/DataModel
Sodium: https://github.com/jedisct1/swift-sodium @ 0.9.1
gRPC: https://github.com/google/grpc-binary.git @ 1.62.2
nanopb: https://github.com/firebase/nanopb.git @ 2.30909.0
Fuse: https://github.com/krisk/fuse-swift @ 1.4.0
FMDB: https://github.com/ccgus/fmdb.git @ 2.7.8
GTMSessionFetcher: https://github.com/google/gtm-session-fetcher.git @ 3.4.1
PocketCastsUtils: /Users/gio/Developer/a8c/pcios/Modules/Utils
SwipeCellKit: https://github.com/shiftyjelly/SwipeCellKit @ 2.7.7
Information about workspace "podcasts":
Schemes:
BuildkiteTestCollector
DSFRegex
google-cast-sdk-no-bluetooth
NotificationContent
NotificationExtension
Pocket Casts Prototype Build
Pocket Casts Staging
Pocket Casts Watch App
pocketcasts
PocketCastsDataModel
PocketCastsServer
PocketCastsUtils
PodcastsIntents
PodcastsIntentsUI
Pods-PocketCastsTests
Pods-podcasts
PreBuildActions
PulseCore
PulseUI
Screenshot Automation
Screenshot Automation Watch
Share Extension
SwiftGen
SwiftLint
WidgetExtension
|
I took this branch for a swing in Pocket Casts iOS – for other readers reference: that's the repo that originally gave us problems because of the double https://buildkite.com/automattic/pocket-casts-ios/builds/7680#01912b6b-222e-4f57-bfe5-09cdc9038bff |
Co-authored-by: Gio Lodi <[email protected]>
…essing which method to use
Co-authored-by: Gio Lodi <[email protected]>
+ Improve parsing by now allowing conflicting options but only parsing one + Adjust some wording
If path to `--workspace` or `--project` not found, still compute the `PACKAGE_RESOLVED_LOCATION` and let the test of file existence (`[[ ! -f "${PACKAGE_RESOLVED_LOCATION}" ]]` provide a better error message.
As opposed to using `-d` (`[[ -d "${XCWORKSPACE_PATH}" ]]` and `[[ -d "${XCODEPROJ_PATH}" ]]`), which would be false when that path doesn't exist and would resultin `PACKAGE_RESOLVED_LOCATION` being unassigned… and lead to a confusing error (`unable to guess path`) as a result.
|
@mokagio Ok I've updated the PR to address all your feedback & remove the need for What I'm unsure about now, is if not providing a
Besides, what |
|
Ok, looking a bit more into it, I think the Indeed I think it's likely that both Doing a Similarly, Finally only So TL;DR I guess I just got confused by it being late here and not knowing how it was working with the difference between (Phew, what an adventure.) |
* Add `README` for `install_swiftpm_dependencies` tests * Add `install_swiftpm_dependencies` test for standalone package * Add `install_swiftpm_dependencies` with `--use-spm` parameter * Put commands in the `$PATH` for `install_swiftpm_dependencies` tests * Rename `tests/spm_caching` to `test/install_swiftpm_dependencies` Clearer, right? * Move test instructions to script to correctly export `$PATH` * Fix inverted cmds in CI * Add test for `xcodeproj` with automatic detection * Add test for `xcodeproj` with explicit parameter * Add tests for `install_swiftpm_dependencies` with `xcworkspace` * Remove backticks from group name—They don't render * Rename `standalone_package` to `package` * Rename `xcodeproj` to `project` * Make `test_workspace_*` executable * Add tests for scenario with both workspace and project in the same dir * Fix outdated scripts names * Add `set -o pipefail` to test scripts to avoid false positives * Fix typo in test script path * Move test scripts in dedicated folder to keep tidy * Update `xcworkspace` lookup to avoid false negative * Revert "Update `xcworkspace` lookup to avoid false negative" This reverts commit 4e7c222. * Remove trailing whitespaces from `install_swiftpm_dependencies` * Use `shopt -s nullglob` to avoid false negative * Code-generate project * Add handling to auto-detect standalone project * Generate `Package.resolved` for xcodegen project * Fix path in `test_workspace_and_project_automatic.sh` * Replace hardcode `project` with code-generated one * Replace project in workspace+project setup with code-generated one * Fix code-gen Buildkite script paths * Add missing `xcodegen` in some CI tests * Fix `pushd` not working from Makefile - Bypass it * Streamline `.gitignore` in `workspace` * Use `make` in `project` for consistency * Dry Buildkite pipeline * DRY env setup for SPM tests * DRY `xcodebuild` call in tests * Fix unbound TESTS_LOCATION * Fix wrong path for `xcodebuild` * Remove misplaced `USE_SPM` set * Use a fixture `Package.resolved` * Add test for `xcodeproj` without `Package.resolved` * Make a `~~~` group print a confirmation to avoid empty logs * Refine error logging in `test_project_no_package.sh` * Apply suggestions from code review * Revert change for `workspace_and_project/Makefile` Seems the `cp` of the `project.yml` was intentional after all, as using `--spec` instead broke it on CI --------- Co-authored-by: Olivier Halligon <[email protected]>
|
Now that the Integration Tests from #109 have been merged into this PR, this is now ready for a final review before auto-merge! cc @mokagio @spencertransier |
We forgot to add a CHANGELOG entry for it before merging
We forgot to add a CHANGELOG entry for it before merging
We disabled this because of an issue with package resolution, see #1990 Since then, a fix has been implemented in the command, see Automattic/a8c-ci-toolkit-buildkite-plugin#105 so we can re-enable it.
Why?
To solve issues like the one reported in Automattic/pocket-casts-ios#1990
TL;DR: the
install_swiftpm_dependenciesutility was callingxcodebuild -resolvePackageDependencieswithout the-workspaceflag, thus relying on thePackage.resolvedfrom the.xcodeprojinstead of the.xcworkspaceby default (at least that's how thatxcodebuildcommand seems to behave).This led to us having multiple
Package.resolvedfiles (one inxcodeproj, one inxcworkspace, and two sources of truth that were disagreeing with each other, depending on which method we used to resolve the dependencies last (Xcode UI orxcodebuildor this a8c-ci-toolkit utility).What?
This PR makes some changes to our
install_swiftpm_dependenciesutility so that:--workspace PATH --scheme NAMEexplicitly to resolve dependencies based on an Xcode.xcworkspaceworkspacexcodebuild --helpdocumentation suggests that we should be able to callxcodebuild -workspace -resolvePackageDependencieswithout providing a-schemeparameter… but in practice doing so makesxcodebuildcomplain about that parameter missing and exit. Hence why this initial refactoring will expect a--schemeto be passed too, even if in practice I'm pretty sure the scheme doesn't have any effect on the packages resolution itselfxcodebuildbug and not need to pass-schemewhen passing-workspaceafter all, by instead passing-listinstead. This might require some more testing to validate it works as expected though (and especially still honors the-onlyUsePackageVersionsFromResolvedFileoption in those cases too)--project PATHexplicitly to resolve dependencies based on an Xcode lone.xcodeprojproject--use-spmto useswift package resolveinstead ofxcodebuild(typically for library repos which don't have an Xcode project and workspace and fully rely on aPackage.swiftfile at the root)xcworkspaceat the root of the repo, and noPackage.swiftat the root, usexcodebuild -workspace, using (arbitrarily) the first scheme found in that workspace for the-schemeparameter ofxcodebuild(that is documented not to be needed but in practice errors out if not provided—there might be room for improvement here / hope for not needing this after all though).xcworkspacebut there's aPackage.swiftfile, useswift package resolve-workspacewhen resolving swift dependencies usingxcodebuildand have the cache key and the dependency resolution both based on the.xcworkspace/xcshareddata/swiftpm/Package.resolvedfile—as opposed to incorrectly using one from a*.xcodeproj/**/Package.resolved.WIP / Draft
Important
The changes in this PR and bash script have not been really tested. In practice I've written the logic mostly blind (i.e. without trying to run it on any repo to test it), and I'm only opening the PR as Draft so soon so I can get feedback on the code and the logic.
We should definitively take some extra time to test those changes on various repos in various setups:
Package.swift, repo with.xcworkspaceat repo root but.xcodeprojin subdir, repo with both.xcworkspaceand.xcodeprojfiles at repo root, repo with multiple.xcworkspaceand/or.xcodeprojfiles and multiplePackage.resolvedfiles, repos with noPackage.resolvedfile at all…;--workspace/--scheme/--project/--use-spmparameters, with only--workspaceand letting it guess the scheme, with no parameter, …Compatibility / Breaking Change
Note that this is technically a breaking change for the
install_swiftpm_dependenciesutility, since it previously accepted 2 optional extra arguments (full custom path toPackage.resolved, and build type), while the new implementation doesn't accept those optional extra arguments anymore.This is intentional, as:
Package.resolvedas the first argument made this path used for the computation of the cache key during cache restoration and save… but that custom file was not the one used byswift package resolveorxcodebuild -resolvePackageDependenciesduring dependency resolution, so that would lead to inconsistencies and odd behaviors if usedTbh though, I'm not even sure any of our repos currently calling
install_swiftpm_dependenciesever passed extra parameters to it though, so those were probably never ever used in practice anyway?Suggestion for adopting repos
Repos like PocketCasts-iOS were having issues with that utility because it was inconsistently using either
podcasts.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolvedwhen callingxcodebuild -resolvePackageDependencies(without a-workspace, thus making it fallback to the.xcodeproj) during resolution while the cache was relying onpodcasts.xcworkspace/xcshareddata/swiftpm/Package.resolvedfor the cache (or when developers resolved dependencies via Xcode and commited that file), and those two differentPackage.resolvedfiles were not in sync.I'd suggest for repos in such situations to:
*.xcodeproj/xcshareddata/swiftpm/Package.resolvedfile and remove it from git, so that only the one in the*.xcworkspaceis kept aroundinstall_swiftpm_dependenciesin the.buildkite/**scripts to provide the--workspace(and--scheme) explicitlyCHANGELOG.mdif necessary.