Automattic / pocket-casts-ios

Pocket Casts iOS app 🎧
Mozilla Public License 2.0
1.66k stars 133 forks source link

Move `xcodeproj` folder away from the project root #1990

Open mokagio opened 3 months ago

mokagio commented 3 months ago

When called in the root of the project xcodebuild -resolvePackageDependencies will generate Package.resolved in both the xcodeproj and xcworkspace folders. This is likely a bug, as the same command with the 16.0 beta 3 version only generates the resolved file in the xcodeproj folder.

Regardless of the cause, this behavior can, sometimes, result in CI failures such as the one in this build: https://buildkite.com/automattic/pocket-casts-ios/builds/7634#019113ca-d4bd-4838-b93a-aae0e4a0528f

  an out-of-date resolved file was detected at /opt/ci/builds/builder/automattic/pocket-casts-ios/podcasts.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved, which is not allowed when automatic dependency resolution is disabled; please make sure to update the file to reflect the changes in dependencies. Running resolver because requirements have changed.

One solution is to specify the workspace to the command. See how this build passes: https://buildkite.com/automattic/pocket-casts-ios/builds/7657

xcodebuild -resolvePackageDependencies \
  -workspace podcasts.xcworkspace \
  -scheme pocketcasts

While this approach does the job, it requires us to drop the SPM caching because the install_swift_dependencies command in the CI toolkit does not support those options.

Of course, we could add support at the toolkit level, but there is an alternative approach.

A different approach would be to move the xcodeproj in a subfolder. This is the setup used in WordPress and WooCommerce, for example.

I find this approach attractive because it would give us a chance to tidy up the project root folder. For example, we could use this structure:

Modules/  # already exists and contains Swift packages
Targets/ # would contain the sources for the various targets - each target could have its set of xcconfig
Xcode/
  Config/
    Project.base.xcconfig
    Project.debug.xcconfig
    # etc
  podcasts.xcodeproj/
# all the other files that stay in the root

@Automattic/apps-infrastructure what do you think? Is it worth establishing a convention of moving the xcodeproj folder away from the root if an xcworskpace is in use? Or should we take this as the occasion to make the install_swift_dependencies CI toolkit plugin more flexible?

Of course, nothing stops us from doing both, but give each approach would make the other unnecessary, I'd rather take a choice on which one to focus on in the short term.

AliSoftware commented 3 months ago

@Automattic/apps-infrastructure what do you think? Is it worth establishing a convention of moving the xcodeproj folder away from the root if an xcworskpace is in use? Or should we take this as the occasion to make the install_swift_dependencies CI toolkit plugin more flexible?

While moving the xcodeproj file in a subfolder could be an easy fix but also a nice way to keep the repo root cleaner (and avoid new devs to accidentally open the xcodeproj instead of the xcworkspace), I think it would be better to provide a long-term solution that would work on all of our products and repos, without requiring them to have a specific file structure to work.

(also because, otherwise, how would we guarantee that someone wouldn't decide to move the xcodeproj file back to the repo root in the future without realizing such an apparently innocuous change would break SPM?)

TL;DR

I got some brainstorm about it below, but the TL;DR is that I think it'd make more sense to pass --workspace … --scheme … to the xcodebuild command in the hope that this would mimic Xcode UI's "Resolve Package Dependencies" behavior the closest, and thus have both local and CI be guaranteed to behave the same wrt to which Package.resolved to use/update.

That being said, despite the --scheme being a required parameter when you pass --workspace, I wonder if the scheme we pass makes any difference in practice (after all, the list of dependencies is defined at each xcodeproj's level, not at a scheme level), so maybe we can try to be clever and guess a --scheme to use in our invocation within install_swiftpm_dependencies?

Braindump of alternative ideas

Here is a braindump of a couple of alternative ideas / potential solutions I could think of when I thought about this problem:

1️⃣ See if creating a symlink of one Package.resolved to the other one could work

2️⃣ Add some logic to our install_swift_dependencies to sort out the Package.resolved files before calling the xcodebuild … command

3️⃣ Maybe we should pass --project to the xcodebuild command we call from install_swift_dependencies?

…instead of passing nothing, or of passing --workspace but also having to pass --scheme?

That would only make sense if the real SoT to consider—as suggested by Xcode 16.0b3's behavior, apparently?—is the xcodeproj and not the xcworkspace.

4️⃣ Make our install_swift_dependencies utility support additional parameters like scheme… or guess it?

Basically what you suggested above. Only make sense if the xcworkspace is the SoT to use.

Optionally, maybe there could be a way for us to try to be smart if the scheme is not passed explicitly, e.g. maybe we could use xcodebuild command to list all the schemes present and call xcodebuild with each of them in a loop… or just use the first one (as, despite the --scheme parameter being required by xcodebuild when also providing --workspace, in practice I'm not sure it matters with scheme we use when it comes to resolving Package Dependencies, given those dependencies are declared at project level and don't depend on the scheme selected in the UI… right?) 🤷


What's the real Source of Truth?

To be honest I'm not sure which one of the two makes more sense to be the Source of Truth.

Overall I'm not even sure what Xcode (UI) does when it tries to resolve package dependencies on a workspace:

But whatever Xcode UI's does, in the end what probably makes the most sense is to try to make our invocation of xcodebuild … on CI behave the exact same as when Xcode's own UI does a "Resolve Package Dependencies" step after you open the workspace in the UI. Which might involve providing the --workspace parameter explicitly to the xcodebuild command.

AliSoftware commented 3 months ago

Funnily enough, xcodebuild --help suggests that one is supposed to be able to specify -workspace without -scheme when using -resolvePackageDependencies

$ xcodebuild --help
Usage: …
       xcodebuild -resolvePackageDependencies [-project <projectname>|-workspace <workspacename>] -clonedSourcePackagesDirPath <path>

But, as you already noticed, in practice calling that suggested invocation in practice ends up exiting with xcodebuild: error: If you specify a workspace then you must also specify a scheme. Use -list to see the schemes in this workspace. (at least in Xcode 15.4) 😒

mokagio commented 3 months ago

in practice calling that suggested invocation in practice ends up exiting with xcodebuild: error: If you specify a workspace then you must also specify a scheme. Use -list to see the schemes in this workspace. (at least in Xcode 15.4) 😒

Same in Xcode 16.0

image

mokagio commented 3 months ago

Allowing callers to forward -workspace + -scheme and/or -project to install_swift_dependencies (i.e. using the same "allow only a subset of flags to be passed for security and controlled usage, like we already do with other scripts) might be the options that gives us the most flexibility.

mokagio commented 3 months ago

...It would also save us from reverse engineer / understand the various Xcode / xcodebuild quirks...

AliSoftware commented 3 months ago

...It would also save us from reverse engineer / understand the various Xcode / xcodebuild quirks...

True… though I'd argue we'd still benefit from understanding which xcodebuild … command matches the Xcode UI's behavior the closest (i.e. does Xcode UI do xcodebuild -workspace … -resolvePackageDependencies or xcodebuild -project … -resolvePackageDependencies under the hood when we select "Resolve Package Dependencies" from the File menu?), and then prefer using that for future projects or projects like PCiOS (where it doesn't do the right thing by default with our current implementation)

Besides, we still kinda need to understand what Xcode vs xcodebuild does, and in particular which of the two Package.resolved it relies on when we'll invoke our xcodebuild -resolvePackageDependencies … command from install_swiftpm_dependencies… because we compute the cache key from the hash of the Package.resolved file. So if we compute the hash from one Package.resolved but the xcodebuild command we use ends up updating the other one, the cache logic would be incorrect and break.

AliSoftware commented 3 months ago

As a side note, I noticed that our current implementation of install_swiftpm_dependencies assumes that the xcworkspace is at the root of the repo, and that there's only one xcworkspace in total there (or at least always pick the first one even if there are more than one

While it's likely currently the case for most if not all our current projects (?)… I don't see any reason why we should really assume this to always be true; e.g. imagine a monorepo hosting multiple apps and where we'd have decided to put the xcworkspace of each app in their respective subfolders (and none at the root). Or even if we decided to keep the various xcworkspace files at the root, there would be more than one there…

All that is one more point in the camp of allowing install_swiftpm_dependencies to accept -workspace and -scheme parameters to be passed along, I guess, instead of having it try to guess it magically (and being right most of the time… but not always)

AliSoftware commented 3 months ago

@mokagio I've started a draft implementation of some improvements to our install_swiftpm_dependencies utility in https://github.com/Automattic/a8c-ci-toolkit-buildkite-plugin/pull/105. This is still WIP but curious about your thoughts on the approach and your configuration it would help solve the issue here in PCiOS.

I actually have some hope about a solution that would allow us to use -workspace without having to also specify -scheme (TL;DR: pass -list instead), which would solve the whole issue in mostly a transparent way (given the new auto-detect logic of the xcworkspace on the repo this utility is invoked as been improved too which means that in 95% of cases this would work transparently without having to pass --workspace to install_swiftpm_dependencies explicitly either…?). That theory needs to be thoroughly tested first though, but I'm at my EOD so won't test it today.