yonaskolb / XcodeGen

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

Support for multiple deployment targets with xcode 14 #1336

Closed amatig closed 11 months ago

amatig commented 1 year ago

Hi, I am Giovanni Amati. I am new, I have never contributed to this project but because in my company we are using XcodeGen, and we are starting a new app, it was good to have this new feature to support iOS and AppleTV in one single target.

This fixes #1333 , opened by me then I decided to try to solve it.

What I have done:

Ex. of spec

targets:
  MyApp:
    type: application
    supportedDestinations: [iOS, tvOS]
    sources:
      - path: ../Project/GLEAP/Sources
        inferDestinationFiltersByPath: true
      - path: ../Project/GLEAP/Storyboards
        destinationFilters: [iOS]
    dependencies:
      - package: Yams
        product: Yams
        destinationFilters: [tvOS]
      - package: Lottie
        product: Lottie

Everything works with every bundle so it's possible to do the same for UI tests, 1 single target and if you have different file you can filter them by platform. It is very useful.

This spec becomes:

Screenshot 2023-03-20 at 13 48 39

Screenshot 2023-07-19 at 17 17 25

Many thanks!

yonaskolb commented 1 year ago

Thank you for your work here @amatig. Sorry I haven't had the time to go over all these changes yet, just know I haven't forgotten about this

amatig commented 1 year ago

Thank you for your work here @amatig. Sorry I haven't had the time to go over all these changes yet, just know I haven't forgotten about this

No worries at all. I tried to do my best, take your time or everyone to review this is a proper way :D

amatig commented 1 year ago

Thank you for your work here @amatig. Sorry I haven't had the time to go over all these changes yet, just know I haven't forgotten about this

Hi, any news?

amatig commented 1 year ago

Sorry for the delay on this @amatig. This is a really fantastic PR, thank you for all your hard work here! I really appreciate all the tests. I've left some small comments above but mostly looks good.

I would say too there would probably still be a little bit of confusion for users about when to use platform or supportedPlatforms.

One thing that would be great to add is an example multi platform app in the TestProject, where you've added the platform files. That project is designed to have every xcodegen feature in it. It doesn't need to do anything, just to build

Sorry for the delay on this @amatig. This is a really fantastic PR, thank you for all your hard work here! I really appreciate all the tests. I've left some small comments above but mostly looks good.

I would say too there would probably still be a little bit of confusion for users about when to use platform or supportedPlatforms.

One thing that would be great to add is an example multi platform app in the TestProject, where you've added the platforms files. That project is designed to have every xcodegen feature in it. It doesn't need to do anything, just to build

Yes I know that I could be confusing but it seems that we need a base platform (the SDK to use) and then multiple platforms that we want to support. The Xcode 14 settings work in this way.

I think that people, now, can not use anymore version less than Xcode 14 to upload builds.

So one possibility could rue enaming platform in basePlatform or sdkPlatform (or something else) and completely remove the possibility to manage it as an array and generate different target per platform.

Screenshot 2023-07-19 at 17 17 25

amatig commented 1 year ago

Pushed fixes after review and added under TestProject/App_supportedPlatforms a spec and project already generated.

bdrelling commented 1 year ago

@amatig just wanted to say thanks for your hard work on this, looking forward to having it integrated.

I am specifically hoping to avoid the problem that occurs currently when frameworks generated with XcodeGen using Xcode 14+ seem to automatically include all supported destinations, even when not specified, like so:

image

For now, I've just added the following keys to my project.yml target.settings dictionary, like so:

targets:
  MyTarget:
    type: framework
    platform: iOS
    deploymentTarget: "13.0"
    sources: [Source]
    settings:
      SUPPORTS_MACCATALYST: NO
      SUPPORTS_MAC_DESIGNED_FOR_IPHONE_IPAD: NO

Also, if not already considered, going with supportedDestinations would disambiguate it from platforms, and one platform could potentially be multi or something equivalent to specify the new multiplatform setup. Just an idea! But if you've already accounted for all of this, ignore me. πŸ™‚

Thanks again!

amatig commented 1 year ago

@amatig just wanted to say thanks for your hard work on this, looking forward to having it integrated.

I am specifically hoping to avoid the problem that occurs currently when frameworks generated with XcodeGen using Xcode 14+ seem to automatically include all supported destinations, even when not specified, like so:

image

For now, I've just added the following keys to my project.yml target.settings dictionary, like so:

targets:
  MyTarget:
    type: framework
    platform: iOS
    deploymentTarget: "13.0"
    sources: [Source]
    settings:
      SUPPORTS_MACCATALYST: NO
      SUPPORTS_MAC_DESIGNED_FOR_IPHONE_IPAD: NO

Also, if not already considered, going with supportedDestinations would disambiguate it from platforms, and one platform could potentially be multi or something equivalent to specify the new multiplatform setup. Just an idea! But if you've already accounted for all of this, ignore me. πŸ™‚

Thanks again!

Hi, if you look at the top there is an example how to define supported platforms.

targets:
  MyApp:
    type: application
    platform: iOS
    supportedPlatforms: [iOS, tvOS]
    sources:
      - path: ../Project/GLEAP/Sources
        inferPlatformFiltersByPath: true
      - path: ../Project/GLEAP/Storyboards
        platformFilters: [iOS]
    dependencies:
      - package: Yams
        product: Yams
        platformFilters: [tvOS]
      - package: Lottie
        product: Lottie 

Platform the base SDK to use, you need to choose one and the supportedPlatforms is the list of supported platforms. Everything can be filtered by platforms.

I have also updated the doc so if you look the diff of the doc in this PR you can understand what it does.

amatig commented 1 year ago

Thanks @amatig, looking good, we're getting there!

A couple of comments:

  1. @bdrelling makes an interesting point that in the Xcode interface cross platform apps have multiple "Supported Destinations" and use "Destination" terminology instead of "Platform". Renaming supportedPlatforms to supportedDestinations would match this and remove some of the ambiguity. What do you think of this?
Screenshot 2023-08-08 at 8 47 18 pm
  1. On the point of if we still need platform if we define supportedPlatforms, you mentioned we still need a base SDK. I just created a new cross platform app in Xcode 14.3 and the base SDK is set to auto. Is that not something we could replicate?
Screenshot 2023-08-08 at 8 37 38 pm
  1. Master now has support for visionOS. Ideally this work would support that. How do you feel about adding support for a visionOS destination? It could be a followup PR, though you might encounter some build errors if you update from master now anyway.

Oh yes I can rename it. I kept that name because supportedPlatforms is the one used by XcodeProj. I can look at the auto platform and the other comments. About visionOS we still on Xcode beta but I can have a look. I am in holidays at the moment but I will be back soon.

amatig commented 1 year ago

Hi, I did a couple of things and now I am investigating about "auto" for baseSDK and visionOS.

About "auto" I have a couple of things to ask. At the moment, a lot of logics are based on the platform that is mandatory. If I add a new case auto in the enum we are going to lose presets that we are applying now (Platform and Product_Platform) so the result will be a project a little bite more empty where the user needs to configure more build settings stuff in the spec because we can not help the generation.

Is this an issue? Or isn't it better to leave to he user the decision to define one "baseSDK" so we can give him few presets and then he needs to customised the extra platforms things if he wants a multi-platform solution?

Or both? You can put what ever you want iOS, tvOS or auto (or nothing that fallbacks to auto). The user will lose presets only in auto.

What do you think? @yonaskolb

amatig commented 1 year ago

Hi, I did a couple of things and now I am investigating about "auto" for baseSDK and visionOS.

About "auto" I have a couple of things to ask. At the moment, a lot of logics are based on the platform that is mandatory. If I add a new case auto in the enum we are going to lose presets that we are applying now (Platform and Product_Platform) so the result will be a project a little bite more empty where the user needs to configure more build settings stuff in the spec because we can not help the generation.

Is this an issue? Or isn't it better to leave to he user the decision to define one "baseSDK" so we can give him few presets and then he needs to customised the extra platforms things if he wants a multi-platform solution?

Or both? You can put what ever you want iOS, tvOS or auto (or nothing that fallbacks to auto). The user will lose presets only in auto.

What do you think? @yonaskolb

I have an idea. Because the destinations have a priority defined in the enum of SupportedDestination, I can assign to the target the platform preset and product_preset of the destination with higher priority. So the user can have something if the platform is "auto" or undefined.

I pushed the changes have a look, if this approach is ok I will add unit tests and add stuff in the doc.

I have also removed "isResolved" as a property of the Target (the new field that I have added), I am adding that information only in the dictionary to help me to check if the spec is correct. For this reason I moved the error of that check in SpecParsingError.

Added visionOS support and fixed doc.

Everything is done, I have also added more unit tests and checks to prevent issue in the spec and help the user.

amatig commented 1 year ago

Any news?

FelixLisczyk commented 11 months ago

This feature would significantly reduce the number of framework targets in my project. πŸ‘ When can we merge it?

amatig commented 11 months ago

This feature would significantly reduce the number of framework targets in my project. πŸ‘ When can we merge it?

Hope soon, I made the last changes after the review I am waiting for @yonaskolb

FelixLisczyk commented 11 months ago

@amatig I've noticed that the supportedDestination enum doesn't contain the watchOS platform. Is there any reason for this? I can add it manually in Xcode.

Screenshot 2023-11-09 at 11 51 01