Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-Platform support - Phase 1: Introduce TuistGraph.Destination and migrate TuistGraph.Target to use it #5132

Merged
merged 54 commits into from
Aug 11, 2023

Conversation

waltflanagan
Copy link
Member

@waltflanagan waltflanagan commented Apr 3, 2023

We hope to introduce multiplatform support incrementally and reduce the likelihood of requiring a major Tuist release.

Currently we expect the phases to looks something like this:

  • Updating test host for unit test targets (Update TEST_HOST to use BUNDLE_EXECUTABLE_FOLDER_PATH from Xcode 14 #5289)
  • Addition of destinations on TuistGraph.Target + supporting updates (This PR)
  • legacyPlatform clean-ups
    • Go through code paths that currently leverage legacyPlatform to evaluate how best to migrate it to leverage destinations (this PR already covers this, but possibly any additional tidy ups such that we can omit legacyPlatform or replace with something more permanent e.g. primaryPlatform if needed)
  • Update the linters to pre-pare for multi-destination support
    • Which destination sets are / aren't supported
  • Update default settings / config generator to accommodate multi-destinations
    • Applying the correct recommended / essential settings when using multiple destinations
  • Updating the public API
    • Introduce new Target initialiser that accepts multiple destinations and deprecate the legacy API
  • Introduce platform filters API within TuistGraph
  • Introduce platform filters public API in ProjectDescription

Multiplatform targets

This part of the work focuses on refactoring TuistGraph.Target to enable compilation and deployment to multiple destinations.

Instead of Platform and DeploymentTarget have migrated to be:

  • Supported Destinations: a list of devices that the target can be deployed to. supported platforms are derived from this list. This splits iPhone and iPad into distinct destintions, as well as macOS and macCatalyst.
  • Minimum Deployment: a minimum deployment is a version specifying a minimum requirement for a given OS.

image

This part of the project will be a large amount of work and has high potential to introduce breaking changes.

  • Create types to represent new features in Xcode: Destination and DeploymentTargets
  • TuistGraph.Target to use Destination and DeploymentTargets instead of Platform and DeploymentTarget
  • Fix everything the previous step breaks, which is ~20 unit tests.

Platform Filters

Platform filters should be considered but are not the focus on this specific PR.

Platform filters are a feature allowing different parts of a target to be filtered to only target specific deployment platforms. A source file could only be compiled for macOS or a library only linked on iOS for example. There are multiple Target components that can be filtered.

Base Support

  • BuildFilePlatformFilter -> PlatformFilter
  • Extend PlatformFilter with all Xcode filter options

Target membership

  • CopyFilesAction
  • CoreDataModel
  • ResourceFileElement
  • SourceFile
  • ???

Mapping

  • SourceFileGlob
  • TargetDependency
  • ???

Implements #397

Contributor checklist ✅

  • The code has been linted using run ./fourier lint tuist --fix
  • The change is tested via unit testing or acceptance testing, or both
  • The title of the PR is formulated in a way that is usable as a changelog entry
  • In case the PR introduces changes that affect users, the documentation has been updated

Reviewer checklist ✅

  • The code architecture and patterns are consistent with the rest of the codebase
  • Reviewer has checked that, if needed, the PR includes the label changelog:added, changelog:fixed, or changelog:changed, and the title is usable as a changelog entry

@@ -26,7 +26,7 @@ public enum GraphDependencyReference: Equatable, Comparable, Hashable {
product: Product
)
case bundle(path: AbsolutePath)
case product(target: String, productName: String, platformFilter: BuildFilePlatformFilter? = nil)
case product(target: String, productName: String, platformFilter: PlatformFilter? = nil)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it possible to have multiple platform filters?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. Here i just updated the type but did not assess any logic changes within the Graph related code. If this is a path we want to take then this will probably need to be an array or Set


extension PBXFileElement {
public var nameOrPath: String {
name ?? path ?? ""
}
}

extension PBXBuildFile {
public func applyPlatformFilters(_ filters: [PlatformFilter]?) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks empty and nil means the same righy? let's remove the ambiguity

Suggested change
public func applyPlatformFilters(_ filters: [PlatformFilter]?) {
public func applyPlatformFilters(_ filters: [PlatformFilter]) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good usability thing to assess. Within Xcode's file format, if no filters are specified then the item is associated with all platforms the target deploys to. Adding more complication, Xcode only shows filters for platforms the target deploys to, so im not sure what would happen if a platform is filtered for a target that doesnt deploy to it (probably need a lint rule on this)

@@ -13,6 +13,8 @@ public struct CopyFilesAction: Equatable, Codable {

/// Relative paths to the files to be copied.
public var files: [FileElement]

public var platformFilters: [PlatformFilter]?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please also add swift doc as for the other fields?

Suggested change
public var platformFilters: [PlatformFilter]?
public var platformFilters: [PlatformFilter]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is a very early stage prototype, i'll add comment docs intended to facilitate review but i expect the need to iterate a bit so would prefer investment here to wait until later.

@@ -13,6 +13,8 @@ public struct CoreDataModel: Equatable, Codable {

/// Current version without the extension.
public let currentVersion: String

public let platformFilters: [PlatformFilter]?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please also add swift doc as for the other fields?

Suggested change
public let platformFilters: [PlatformFilter]?
public let platformFilters: [PlatformFilter]

@@ -3,15 +3,15 @@ import TSCBasic

public enum ResourceFileElement: Equatable, Hashable, Codable {
/// A file path (or glob pattern) to include, a list of file paths (or glob patterns) to exclude, and ODR tags list. For convenience, a string literal can be used as an alternate way to specify this option.
case file(path: AbsolutePath, tags: [String] = [])
case file(path: AbsolutePath, tags: [String] = [], platformFilters: [PlatformFilter]? = nil)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if nil is same as empty, remove nillability.
If not, clarify the difference

public var product: Product
public var bundleId: String
public var productName: String
public var deploymentTarget: DeploymentTarget?

public var minimumDeployments: [MinimumDeployment]?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't minimumDeployments and destination strictly related?
For example, does it make sense to specify a minimumDeployment for iOS if there isn't a destination whose platform is iOS?

If it doesn't make sense, we should model it better, something like (just very quick proposal, havent thought much about it):

public enum Destination: String, Codable, Equatable, CaseIterable {
    case iOS(minVersion: String?, devices: IOSDevice)
    case macOS(minVersion: String?, devices: MacOSDevice)
    case tvOS(minVersion: String?)
    case watchOS(minVersion: String?)
}

public struct IOSDevice: OptionSet, Codable, Hashable {
    public static let iphone = IOSDevice(rawValue: 1 << 0)
    public static let ipad = IOSDevice(rawValue: 1 << 1)
    public static let macWithiPadDesign = IOSDevice(rawValue: 1 << 2)
}

public struct MacOSDevice: OptionSet, Codable, Hashable {
    public static let mac = MacOSDevice(rawValue: 1 << 0)
    public static let macCatalyst = MacOSDevice(rawValue: 1 << 1)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial thought on this modeling was to follow the modeling from Xcode itself.

A given destination implies an SDK (and thus needs a minimum deployment version). Each SDK can then only have a single version that is supported.

I wonder if a change like you suggested will make the downstream changes easier to manage, could Destination then own some of the logic around linting and graph traversal which currently rely on target.platform and target.deploymentTarget. it seems it could be a win for simplicity if this reduces the complexity from those split types. I'll find some time to play with this approach. thanks

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would try to keep our models close to what the data represents, and as strictly typed as possible, so I would go for the enum even if (not sure if it's the case) it makes slightly harder to implement tuist logic later

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking through this more, I feel like the Destination type is the way to go with this. After more testing, the destination UI controls a few things related to the platforms and devices that a target supports. There may be more, but the base set of settings this controls is TARGETED_DEVICE_FAMILY, SUPPORTED_PLATFORMS, SUPPORTS_MACCATALYST, SUPPORTS_MAC_DESIGNED_FOR_IPHONE_IPAD, and SDKROOT.

Currently if a target were to be iOS but only target iPhone or iPad, that requires two separate settings (platform and deployment target).

Additionally, with "Minimum Deployments" in Xcode, these settings are per platform, optional, and only control <platform>_DEPLOYMENT_TARGET. Since they are optional, they feel distinct from the core options controlled by the list of devices in "Supported Destinations"

So, I'd like to try using the Destination type (as Set<Destination>) to replace platform on TuistGraph.Target and replacing DeploymentTarget with a struct (DeploymentTargets) with optional version strings for each platform. DeploymentTargets would enforce the constraint that any given platform can only have a single minimum version. TuistGraph.Target.deploymentTarget would be replaced with TuistGraph.Target.deploymentTargets but the source of truth for what devices are supported (or catalyst) would move to Destination

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also came across this past comment that expressed some struggles with some settings being split between Platform and DeploymentTarget https://github.com/tuist/tuist/blob/main/Sources/TuistGenerator/Linter/SettingsLinter.swift#L62

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I'd like to try using the Destination type (as Set) to replace platform on TuistGraph.Target and replacing DeploymentTarget with a struct (DeploymentTargets) with optional version strings for each platform. DeploymentTargets would enforce the constraint that any given platform can only have a single minimum version. TuistGraph.Target.deploymentTarget would be replaced with TuistGraph.Target.deploymentTargets but the source of truth for what devices are supported (or catalyst) would move to Destination

Agreed - source of truth should be destinations, and migrating the minimum deployment to a struct with optional properties is a better approach:

  • Some projects may choose to define their min deployments in xcconfig files as opposed to every project (we do this as we want to centrally control this for the entire workspace!)
  • The array + enum approach may look nice but in practice is quite error prone and as you have discovered requires additional linting - we in fact migrated generation options in the last major release away from this approach to a struct for this very reason :)

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this effort @waltflanagan and the detailed comments 🙏

A few suggestions to help make progress on this, it will likely require a major release but we can chunk it into smaller pieces leading up to that - and may find way to mitigate that along the way if needed.

  • TuistGraph models can be evolved internally while maintaining the existing public API ProjectDescription
    • This will ensure a stable public API while the internals are being re-wired to the new approach
    • Provides some sanity / safety check that the re-wiring still produces the same / similar results
    • As a last step ProjectDescription can be updated to include the new APIs
  • Perhaps the platform filters can be added separately? I know it's needed to take full advantage of multiple platform, but perhaps can be a subsequent increment?

Other considerations:

public var product: Product
public var bundleId: String
public var productName: String
public var deploymentTarget: DeploymentTarget?

public var minimumDeployments: [MinimumDeployment]?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I'd like to try using the Destination type (as Set) to replace platform on TuistGraph.Target and replacing DeploymentTarget with a struct (DeploymentTargets) with optional version strings for each platform. DeploymentTargets would enforce the constraint that any given platform can only have a single minimum version. TuistGraph.Target.deploymentTarget would be replaced with TuistGraph.Target.deploymentTargets but the source of truth for what devices are supported (or catalyst) would move to Destination

Agreed - source of truth should be destinations, and migrating the minimum deployment to a struct with optional properties is a better approach:

  • Some projects may choose to define their min deployments in xcconfig files as opposed to every project (we do this as we want to centrally control this for the entire workspace!)
  • The array + enum approach may look nice but in practice is quite error prone and as you have discovered requires additional linting - we in fact migrated generation options in the last major release away from this approach to a struct for this very reason :)

@danieleformichelli danieleformichelli linked an issue Jun 13, 2023 that may be closed by this pull request
@github-actions
Copy link
Contributor

github-actions bot commented Jul 1, 2023

This PR has been marked as stale because it's been opened for more than 30 days. If no action is taken, it'll be closed in 5 days.

@waltflanagan
Copy link
Member Author

👋 i missed the comment from May, but i did get some work done on this over the last few days and will be pushing those changes here in the next day or two preview

I still need to create the new DeploymentTargets type.

@waltflanagan waltflanagan force-pushed the msimons/MultiPlatform branch 2 times, most recently from b30005b to a9506d1 Compare July 7, 2023 15:58
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Comment on lines 6 to 9

public let iOS: String?
public let macOS: String?
public let watchOS: String?
public let tvOS: String?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not a [Platform: String] instead of a list of nullable strings?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, maybe we could use a stricter type instead of String?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any given target can have a minimum deployment target set for each platform that exists, but only one platform can be set. using an array would allow something like [(.iOS, "13.0"), (.iOS, "14.0")] which this approach prevents at the compiler level. I used string because i was following the original DeploymentTarget pattern (open to changing this but it minimizes overall codechanges in the codebase). And they're all optional because they aren't required by Xcode (missing versions get a default provided by that Xcode version) and they can all also be set via xcconfig files or build settings.

@@ -114,18 +114,17 @@ final class RunService {
osVersion: version?.version(),
graphTraverser: graphTraverser
)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

/// - Parameters:
/// - manifest: Manifest representation of deployment target model.
static func from(manifest: ProjectDescription.DeploymentTarget?) -> TuistGraph.DeploymentTargets {
if let manifest {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would use a guard

@waltflanagan waltflanagan force-pushed the msimons/MultiPlatform branch 2 times, most recently from a27c6e8 to 2fa02e5 Compare July 11, 2023 01:16
Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @waltflanagan 🎉

In terms of path forward, I can see the following set of incremental PRs:

  • Updating test host for unit test targets
    • I believe this was an Xcode 14 addition which we can now bump to by default Xcode 14 Support #4531 (can be a small independent change not coupled to multi-destination)
  • Addition of destinations on TuistGraph.Target + supporting updates (mostly this PR)
  • legacyPlatform clean-ups
    • Go through code paths that currently leverage legacyPlatform to evaluate how best to migrate it to leverage destinations (this PR already covers this, but possibly any additional tidy ups such that we can omit legacyPlatform or replace with something more permanent e.g. primaryPlatform if needed)
  • Update the linters to pre-pare for multi-destination support
    • Which destination sets are / aren't supported
  • Update default settings / config generator to accommodate multi-destinations
    • Applying the correct recommended / essential settings when using multiple destinations
  • Updating the public API
    • Introduce new Target initialiser that accepts multiple destinations and deprecate the legacy API
  • Introduce platform filters API within TuistGraph
  • Introduce platform filters public API in ProjectDescription

For most of those the test plan would be ensuring no project diff to any of the existing fixtures when compared to the latest release up to the point we introduce new fixtures that leverage multi-destinations.

Comment on lines +617 to +619
if !target.isExclusiveTo(.macOS) {
pbxBuildFile.applyPlatformFilters([.macos])
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this only required for macOS? don't quite follow the need to specify it here - to my knowledge Xcode by default doesn't apply platform filters unless explicitly specified.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit of a topic of discussion. the logic for whether to include a given build phase is split between the target and BuildPhaseGenerator. some things use accessors on the target like canEmbedSystemExtensions and others just guard like generateEmbedAppClipsBuildPhase.

So question 1 is where should that logic live.

question 2 is how should it behave, forcing a user to specify platform filters during configuration (e.g. the XPC dependency should be defined with the macOS filter by the user) or including filters that apply to the intersection of supported platforms (e.g. if a target has a mac destination, include the XPC dependency but add the platform filter)

This was just an attempt to intelligently apply the filter instead of relying on the previous implementation of canEmbedXPCServices. I could revert and change canEmbedXPCServices to only allow for macOS exclusive targets if we want to address this later.

if target.destinations.contains(.macWithiPadDesign) {
settings["SUPPORTS_MAC_DESIGNED_FOR_IPHONE_IPAD"] = "YES"
} else {
settings["SUPPORTS_MAC_DESIGNED_FOR_IPHONE_IPAD"] = "NO"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For cases where a destination is not specified Xcode doesn't explicitly set this build setting.

The same applies for the other settings.

Doing a smoke test locally on one of the fixtures we can see those settings leak into them 🤐

diff of projects with those changes:

@@ -808,6 +808,9 @@
                                PRODUCT_BUNDLE_IDENTIFIER = io.tuist.AppUITests;
                                PRODUCT_NAME = AppUITests;
                                SDKROOT = iphoneos;
+                               SUPPORTS_MACCATALYST = NO;
+                               SUPPORTS_MAC_DESIGNED_FOR_IPHONE_IPAD = YES;
+                               SUPPORTS_XR_DESIGNED_FOR_IPHONE_IPAD = NO;
                                SWIFT_ACTIVE_COMPILATION_CONDITIONS = DEBUG;
                                SWIFT_COMPILATION_MODE = singlefile;
                                SWIFT_OPTIMIZATION_LEVEL = "-Onone";
@@ -830,6 +833,9 @@

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running through all the fixtures and getting this cleaned up.

One issue that is highlighting a logic change is around in dependencyPlatformFilters. Previously if a DeploymentTarget was not set on a Target then it would not include any filters. Now, since we use destinations, we're matching the logic as closely as we can, but targets that are iOS only without a set deployment target get configured with platformFilter in the Xcode project. We could added a check for an iOS deployment target being set here to match the previous logic for this stage of the migration, but it definitely would be an odd set of logic in there. It's probably fine since this logic will need to likely be rewritten as we address adjusting how targets are linked to each other based on destination anyways.

Thoughts? This method is the area im referring to.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, since we use destinations, we're matching the logic as closely as we can, but targets that are iOS only without a set deployment target get configured with platformFilter in the Xcode project.

in Tuist or default Xcode? We currently don't leverage deploymentTarget from the manifest (we set it in xcconfig) and don't have any platformFitlers set in the generated projects 🤔

We could added a check for an iOS deployment target being set here to match the previous logic for this stage of the migration, but it definitely would be an odd set of logic in there.

It's not an iOS special case to my knowledge, testing in a vanilla project in Xcode for macOS, platform filters aren't set either by default.

For what I can see platformFilters are only to be set when something needs to explicitly diverge (this one framework / file needs a filter) otherwise no filters are set and defaults to all to match whatever the project is configured with. This seems to be the case even for multi-platform apps / frameworks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I think I understood the comment now - indeed there will be some divergence due to the changes from deploymentTarget > destinations - I wonder why they were set in the first place given there wasn't any fine grained filtering support 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found it
#3152

Given this history, I think with the new APIs we can reason about this a bit better :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would something like this work?

extension PBXBuildFile {
    
    /// Apply platform filters either `platformFilter` or `platformFilters` depending on count
    public func applyPlatformFilters(
        _ filters: PlatformFilters,
        hostDestinations: Destinations
    ) {
        guard !filters.isEmpty else { return }
        
        guard platformFiltersRequired(filters: filters, hostDestinations: hostDestinations) else {
            return
        }
        
        if filters.count == 1, let filter = filters.first {
            platformFilter = filter.xcodeprojValue
        } else {
            platformFilters = filters.xcodeprojValue
        }
    }
    
    private func platformFiltersRequired(filters: PlatformFilters, hostDestinations: Destinations) -> Bool {
        !hostDestinations.allSatisfy { filters.supports(destination: $0) }
    }
}

private extension PlatformFilters {
    func supports(destination: Destination) -> Bool {
        switch destination {
        case .iPhone:
            return contains(.ios)
        case .iPad:
            return contains(.ios)
        case .mac:
            return contains(.macos)
        case .macWithiPadDesign:
            return contains(.ios)
        case .macCatalyst:
            return contains(.macos)
        case .appleWatch:
            return contains(.watchos)
        case .appleTv:
            return contains(.tvos)
        case .appleVision:
            // TODO: ...
            return false
        case .appleVisionWithiPadDesign:
            return contains(.ios)
        }
    }
}

The idea being platform filters are applied only if there's an incompatibility between what the dependency supports and the destinations of the target consuming it?

I wonder if possibly a separate update is needed to make the various dependency types have destinations too instead of platformFilters - making it such that platformFilters are derived based on the dependency destinations and consumer destinations. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

such that platformFilters are derived based on the dependency destinations and consumer destinations
I think this is the long term solution. A given target should apply platform filters to a dependency if the dependency supports a subset of the platforms the target supports.

From my research platformFilters on PBXBuildFile control not just target depedencies but also conditional compilation, resource copying, and many other parts of the build process, so i think that the decision on what value to use will ultimately be the responsibility of other parts of the system than applyPlatformFilters.

I'll update dependencyPlatformFilters on TuistGraph.Target to have the same logic as originally so this shouldnt introduce any changes. If this were to add an iOS platform filter to iOS only targets though, this shouldn't be a breaking change for anyone, just a difference in the contents of the Xcode project.

@@ -65,10 +65,10 @@ public final class AutogeneratedWorkspaceSchemeWorkspaceMapper: WorkspaceMapping
var (targets, testableTargets): ([TargetReference], [TestableTarget]) = workspace.projects
.reduce(([], [])) { result, project in
let targets = project.targets
.filter { $0.platform == platform }
.filter { $0.legacyPlatform == platform }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this case, I think .contains(platform) is needed as the goal of the scheme is group targets that can run on that destination.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! this is the kind of input i need on some of these areas to determine what it's trying to accomplish. As i mentioned in the other comment, legacyPlatform currently is intended just support the migration by allowing compilation and test running

Sources/TuistGraph/Models/Destination.swift Show resolved Hide resolved
Comment on lines 202 to 204
public var legacyPlatform: Platform {
return destinations.first?.platform ?? .iOS
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this one, perhaps throwing or optional? it would sadly leak to the call sights but wondering if that's better to detect any issues rather than slightly defaulting to .iOS😕

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is 100% only here to please the compiler, this one isnt intended to actually be here for any other reason that helping the migration. Given the chatter about tuist run and other commands in slack, i think the "proper" version of this is a throwing accessor that returns a platform if there is just one and throws if there are multiple, this supports those tools that assume single platform targets and would require additional flags to support multiplatform targets.

Every other callsite will need to take into account destinations in some form.

Comment on lines 224 to 231
public func isEmbeddableXPCService() -> Bool {
switch (platform, product) {
case (.macOS, .xpc):
return true
default:
return false
}
return product == .xpc
}

/// Determines if the target is an embeddable system extension
/// i.e. a product that can be bundled with a host macOS application
public func isEmbeddableSystemExtension() -> Bool {
switch (platform, product) {
case (.macOS, .systemExtension):
return true
default:
return false
}
return product == .systemExtension
Copy link
Collaborator

@kwridan kwridan Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for those do we need the extra isExclusiveTo(.macOS) check? I guess we don't expect to reach this far with invalid combinations due to the linter so may be ok.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this, if it is an extension is defined by the product type, i think we should rely on linting for whether it makes sense for try to define a system extension for a given destination. I think it would be more informative also for linting to alert the user to this, vs the expected target to just be missing from the generated project.

as for the "can embed" part, it's now possible to have a multi platform app and the macOS flavor is the only one that will embed the extension and that will be achieved with platform filters

case let .iOS(_, devices, _):
if devices.contains(.mac) {
return .catalyst
public var dependencyPlatformFilters: PlatformFilters {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is slightly different since the previous approach would return [] if no DeploymentTarget was set, but with this based on destinations now, there's no equivalent.

Comment on lines 1 to 7
//
// Destination+ManifestMapper.swift
//
//
// Created by Michael Simons on 7/3/23.
//

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I'd remove the header comments

Comment on lines +294 to +297
if target.destinations.contains(.iPhone) { deviceFamilyValues.append(1) }
if target.destinations.contains(.iPad) { deviceFamilyValues.append(2) }
if target.destinations.contains(.appleTv) { deviceFamilyValues.append(3) }
if target.destinations.contains(.appleWatch) { deviceFamilyValues.append(4) }
if target.destinations.contains(.appleVision) { deviceFamilyValues.append(7) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about extracting 1, 2, 3, 4, and 7 into an enum to easily recognise the mapping between a destination and the value in TARGETED_DEVICE_FAMILY.

@@ -159,7 +159,7 @@ final class ConfigGeneratorTests: TuistUnitTestCase {

func test_generateTestTargetConfiguration_macOS() throws {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some changes in ConfigGeneratorTests that would benefit from some unit tests here to prevent regressions:

  • The right TARGETED_DEVICE_FAMILY build setting is set based on the target destinations.
  • The build settings SUPPORTS_MAC_DESIGNED_FOR_IPHONE_IPAD, SUPPORTS_XR_DESIGNED_FOR_IPHONE_IPAD, SUPPORTS_MACCATALYST, and DERIVE_MACCATALYST_PRODUCT_BUNDLE_IDENTIFIER are set accordingly.
  • The right _DEPLOYMENT_TARGET environment variables are set.

@waltflanagan
Copy link
Member Author

@pepicrft @kwridan I'll be working to add new ConfigGeneratorTests tests today. Beyond that tho, i think that this PR is ready to merge with the exception of one last decision that needs to be made around mapping from nil deployment target -> non-optional default destinations.

Beyond this last behavior decision, tests are passing and the only diffs appearing in the fixtures are related to these settings on what decision we make on new default behavior.

Optional deploymentTarget vs non-optional destinations

SUPPORTS_MACCATALYST is explicitly set to YES or NO depending on the devices contained in DeploymentTarget.iOS however, ProjectDescription.Target can have an optional DeploymentTarget which will not do any mapping related to SUPPORTS_MACCATALYST. This leads to a conflict between our ConfigGeneratorTests and assumptions that projects generated by fixtures should not change.

Because we’re removing optionality at the Target level, we need to choose explicit defaults for iOS targets with a nil deployment target.

Within Xcode 14.3, selecting any options in the Destinations UI on a target explicitly sets SUPPORTS_* build settings.

Long term, these build settings should be directly controlled by the destinations set on a target, but what should we do for this PR and for the short term introducing multiplatform support? e.g. if a target is missing .visionWithiPadDesign should we explicitly set SUPPORTS_XR_DESIGNED_FOR_IPHONE_IPAD to NO? (omitting it will use the Xcode default of YES)

What should our explicit defaults be for iOS targets with a nil deployment target?

@waltflanagan waltflanagan changed the title Prototype Multi-Platform support Multi-Platform support - Phase 1: Introduce TuistGraph.Destination and migrate TuistGraph.Target to use it Aug 7, 2023
@waltflanagan waltflanagan marked this pull request as ready for review August 7, 2023 16:32
@pepicrft
Copy link
Contributor

pepicrft commented Aug 7, 2023

Optional deploymentTarget vs non-optional destinations

Thanks for bringing this one up @waltflanagan. I think the answer to this really depends on whether the additional setting is is perceived as a breaking change or not by developers. For example, could those settings introduce compilation issues or introduce regressions in compilation times? If we feel confident they don't, I think it's fine to update the fixtures to reflect the new values we are setting and move forward with it. If we don't feel confident, we can brainstorm how to evaluate whether those changes would be breaking or not. Note that it's sometimes tricky to figure out out, so the only way really is to put the change out there and be prepared to hot-fix if a regression is reported.

@waltflanagan
Copy link
Member Author

The main "breaking" change is whether we provide a Tuist default for these settings or leave it to Xcode to provide a default. If we choose the same defaults as Xcode's provided defaults, we limit any potential impact and it only becomes and issue if there are versions of Xcode that have different defaults than what is provided by 14.3.1.

Even if this does introduce a breaking change for someone, the fix is to update their targets to have an explicit .iOS deployment target that matches their needs.

I'm pretty sure this is a safe path forward but wanted everyone's input to ensure that it's an intentional decision we want to move forward with.

@kwridan
Copy link
Collaborator

kwridan commented Aug 8, 2023

Thanks for all the updates @waltflanagan!

The main "breaking" change is whether we provide a Tuist default for these settings or leave it to Xcode to provide a default. If we choose the same defaults as Xcode's provided defaults, we limit any potential impact and it only becomes and issue if there are versions of Xcode that have different defaults than what is provided by 14.3.1.

Indeed I ran this against our workspace and the only diffs were those and the platformFilter additions.

For the settings, the impact will likely be for projects which rely on xcconfigs. Previously the xcconfig values would take precedence over Xcode defaults and now Tuist would add target level settings which would no longer be overridable. Not sure how common that is (our project is configured this way 🤐 ).

For the platform filter, the impact will be for projects that rely on either the Xcode defaults or xcconfigs that specify those settings.

Will have a think through to see if there any mitigations.

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had another look and indeed it doesn't seem like we can support both the previous approach of leaving Xcode default settings and having a predictable destinations API going forward - so this will have to be a breaking change we live with.

I've left a few suggested fixes to address a few issues I spotted while doing a review of the latest changes.

Thanks again @waltflanagan


return Set(destinations)
case (.iOS, _): // an iOS platform, but `nil` deployment target.
return [.iPhone, .iPad, .macCatalyst, .appleVisionWithiPadDesign]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is specifying .macCatalyst correct here?

Should it be this instead?

Suggested change
return [.iPhone, .iPad, .macCatalyst, .appleVisionWithiPadDesign]
return .iOS

Which is currently defined as:

public static var iOS: Destinations = [.iPhone, .iPad, .macWithiPadDesign]

Adding .macCatalyst here would mean any project that doesn't specify a deployment target would get a .macCatalyst destination.

For example, the App target in the ios_app_with_tests fixture

Latest Tuist:

latest

vs this PR:

pr

return .catalyst
public var dependencyPlatformFilters: PlatformFilters {
// is iOS only and has the equivalent of `.all` devices from `ProjectDescription.DeploymentTarget`
if isExclusiveTo(.iOS), destinations != Set([.iPad, .iPhone, .macCatalyst, .appleVisionWithiPadDesign]) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may help remove the redundant iOS platform filter

Suggested change
if isExclusiveTo(.iOS), destinations != Set([.iPad, .iPhone, .macCatalyst, .appleVisionWithiPadDesign]) {
if isExclusiveTo(.iOS), destinations != .iOS {

@waltflanagan
Copy link
Member Author

@kwridan I incorporated both of those suggestions. Do we feel we need to do any comms on the potential breaking change for folks that dont specify a deployment target? Is it just for those that omit the deployment target but then rely on xcconfig files that could potentially be overridden by the new behavior?

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwridan I incorporated both of those suggestions.

Awesome, thank you @waltflanagan!

Do we feel we need to do any comms on the potential breaking change for folks that dont specify a deployment target?

That's a good idea, I'll send a PR to add a note to the change-log.

Is it just for those that omit the deployment target but then rely on xcconfig files that could potentially be overridden by the new behavior?

From what I can see so far, that seems to be the impact. There will be a small nuisance till we add the new public destination and multiple deployments APIs (thank you for separating those 🙏) - today those two are sadly coupled which means for anyone wanting to fix the above destination overrides would also now need to specify their minimum deployment in manifest too which is commonly in xcconfigs too 🙈.

@kwridan kwridan added the changelog:changed PR will be listed in the Changed section of CHANGELOG label Aug 11, 2023
@kwridan kwridan merged commit a01d4fb into tuist:main Aug 11, 2023
36 checks passed
kwridan added a commit that referenced this pull request Aug 11, 2023
- #5132 has now been merged which is an important pre-requiste for multi-destination work
- There are few potential breaking changes identified which may impact some project setups
- A changelog entry has been added to help raise awareness
@kwridan
Copy link
Collaborator

kwridan commented Aug 11, 2023

#5347

pepicrft pushed a commit that referenced this pull request Aug 17, 2023
- #5132 has now been merged which is an important pre-requiste for multi-destination work
- There are few potential breaking changes identified which may impact some project setups
- A changelog entry has been added to help raise awareness
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:changed PR will be listed in the Changed section of CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multi-platform targets
4 participants