yonaskolb / XcodeGen

A Swift command line tool for generating your Xcode project
MIT License
6.88k stars 809 forks source link

Using 'supportedDestinations' with watchOS app doesn't generate an 'Embed Watch Content' build phase #1463

Open FelixLisczyk opened 3 months ago

FelixLisczyk commented 3 months ago

I'm currently in the process of migrating my project specs from platform to supportedDestinations. According to the project spec, this should work for all target types:

Note that the definition of supported destinations can be applied to every type of bundle making everything more easy to manage (app targets, unit tests, UI tests etc).

I've noticed when I configure my watch app target with supportedDestinations, XcodeGen no longer generates an 'Embed Watch Content' build phase for the iOS app. Here is an example:

name: MyApp
options:
  bundleIdPrefix: com.example
targets:
  UniversalApp:
    dependencies:
      - target: WatchApp
    supportedDestinations: [iOS, macOS]
    type: application
    sources:
      - Universal
  WatchApp:
    supportedDestinations: [watchOS]
    type: application
    sources:
      - Watch

Is this a bug or intended behavior? Should I continue to use platform for my watchOS targets? Thank you!

bcardarella commented 2 months ago

I am also looking for an answer on this. The doc's aren't clear if watchOS is supported for supportedDestinations or not. It's not included in the list of supported destrinations but if watchOS isn't supported I was curious as to why and what our options are for including it?

bcardarella commented 2 months ago

Perhaps it is fixed with https://github.com/yonaskolb/XcodeGen/pull/1438

yonaskolb commented 2 months ago

@atsuky do you have any info on this? Did your PR #1438 get this working? I've just added watchOS as a supported destination in the docs, as that PR missed that out

Also pinging @amatig as the original implementor of the supported destinations work, in case he has any insights

tatsuky commented 2 months ago

Hi, thank you for the doc update. I'll take a look at the issue.

tatsuky commented 2 months ago

Looks like the following part is causing the issue. https://github.com/yonaskolb/XcodeGen/blob/03017410027ba244f277e03e38d05d3d3c3544fd/Sources/XcodeGenKit/PBXProjGenerator.swift#L779-L780

platform is auto when supportedDestinations is set. I think it's why the "Embed Watch Content" phase gets skipped even if the destinations include watchOS.

I think we can fix it by also checking if supportedDestinations == [.watchOS] here. It's not supportedDestinations.contains(.watchOS) so that we can rule out the apps with e.g., supportedDestinations: [iOS, watchOS] just in case (I find this type of combination (watchOS + other destinations) uncommon for an application, though).

Let me know what you all think. I'll create a PR if this approach looks good.

bcardarella commented 2 months ago

@tatsuky does this mean that it would skip watchos?

tatsuky commented 2 months ago

@bcardarella By "it" do you mean:

so that we can rule out the apps with e.g., supportedDestinations: [iOS, watchOS] just in case

If so - it will just skip adding the "Embed Watch Content" phase for the target. The supportedDestinations will be kept as initially configured.


After reading Apple's Configuring a multiplatform app - apparently the watchOS destination for the multiplatform apps is not supported at the moment. The destination is also unavailable (not showing up) for a multiplatform app on Xcode 15.3.

Destination options for multiplatform apps on Xcode 15.3

Given this information & on second thought, we should maybe disallow the watchOS supportedDestination for an application instead of add the logic fix I mentioned in my previous comment?

bcardarella commented 2 months ago

@tatsuky perhaps emmit a warning or error if it is included and point to this issue so understand why it isn't (yet) supported. Looking at the issue tracker and it seems to be a common question

yonaskolb commented 2 months ago

@tatsuky, I support the change to add the check for supportedDestinations == [.watchOS] when embedding watch content, as well as add an error for configuring a destination to be watchOS AND iOS, as well as highlighting that in the documentation. If you still want to create the PR that would be fantastic

tatsuky commented 2 months ago

@yonaskolb Just to confirm, can we still add the changes you suggested, given Xcode 15.3 seemingly doesn't allow us to create multiplatform apps that contain the watchOS destination? (at least not on the UI, that is)

Alternatively, I think we could consider it as an invalid configuration and have xcodegen error out like @bcardarella suggested.

yonaskolb commented 2 months ago

@tatsuky yes agree. That's what I meant by an error when watchOS and iOS are in the same destinations array 👍

tatsuky commented 2 months ago

OK, I'll work on the fix and open a PR one of these days