Multiplatform Phase 3 - Expose support in `ProjectDescription` #5381

waltflanagan · 2023-08-28T14:46:25Z

Very first exposure of multiplatform work in ProjectDescription. All feedback on API design welcome.

Short description 📝

tbd

How to test the changes locally 🧐

tbd

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

kwridan

Thanks @waltflanagan

kwridan · 2023-08-31T08:07:55Z

Sources/ProjectDescription/MultiplatformTarget.swift

@@ -0,0 +1,112 @@
+import Foundation
+
+extension Multiplatform {


The approach you've taken here is much cleaner though so I see the value in that, main concern is maintaining two duplicate Targets

Would it be possible to make this the main Target structure but map the the legacy properties to the appropriate underlying new properties?

e.g.

init( // ... platform: Platform, deploymentTarget: DeploymentTarget, // ... ) { self.init( // ... destinations: [Destination.from(platform)], deploymentTargets: DeploymentTargets.from(deploymentTarget) // ... ) }

yes we can and should. this approach was taken to continue to support source compatibility for current Tuist users. given that platform is a public property of Target, I didn't feel safe changing how things like that are exposed at this point in time.

If we're open to breaking changes, we can very much adjust the main Target type for our new needs as well as provide a convenience initializer that makes migration simpler for folks that dont need multiplatform support.

Makes sense to maintain source compatibility.

My thinking is we may be able to map / derive the old properties to prevent breaking changes, however if that proves to be too much hassle the approach you have taken is much cleaner.

I'm on the fence here. While supporting source compatibility is important, the cost of maintaining two similar structures in the codebase creates room for inconsistencies. I'm not a big fan of breaking changes, but multi-platform support is an important change for Tuist and might warrant going with a major release, in which we can batch other deprecated APIs. If I'm not wrong here, the changes developers would have to make are not that significant, so even if it's a breaking change, which is always undesirable, the process "should" be straightforward.

My main thought for this was that it would be "short" lived. We could publish the source compatible version for a while, allow users to migrate at their own pace and incrementally if they have many targets to migrate. After enough time we can make the source breaking change to remove the old deprecated target implementation and promote the multiplatform one to the only one.

pepicrft · 2023-09-04T11:14:26Z

Sources/TuistKit/Utils/Target+PlatformResolution.swift

+
+extension Target {
+    /// Platform for use with services when a platform is not specified
+    var servicePlatform: Platform {


What about naming this one platformIfOneOrThrow? It reads more verbose, but someone reading the code will immediately get the logic by reading the getter name.

…ations - When specifying multiple destination, the `SUPPORTED_PLATFORMS` build setting needs to be set to include the various sdks for both simulator and device Notes: - This was cherry-picked from a draft PR for multi-platform support #5381 Test Plan: - Multiple destinations is not yet exposed via public as such can only be validated with unit tests at this time

- Updated the `GraphLinter` to account for multi-destination (multi-platform) targets - Lint issues are now flagged only if there are no valid source and destination platform combination based on the existing rules rather than only using the `legacyPlatform` (single platform) to base the rules on Test Plan: - Existing unit tests should continue to pass - New cases for valid and invalid combinations were addeed Notes: - This is a pre-requiste for #5381 which adds the public API for multi-desintations

…ations - When specifying multiple destination, the `SUPPORTED_PLATFORMS` build setting needs to be set to include the various sdks for both simulator and device Notes: - This was cherry-picked from a draft PR for multi-platform support tuist#5381 Test Plan: - Multiple destinations is not yet exposed via public as such can only be validated with unit tests at this time

…ations (#5438) - When specifying multiple destination, the `SUPPORTED_PLATFORMS` build setting needs to be set to include the various sdks for both simulator and device Notes: - This was cherry-picked from a draft PR for multi-platform support #5381 Test Plan: - Multiple destinations is not yet exposed via public as such can only be validated with unit tests at this time

- Updated the `GraphLinter` to account for multi-destination (multi-platform) targets - Lint issues are now flagged only if there are no valid source and destination platform combination based on the existing rules rather than only using the `legacyPlatform` (single platform) to base the rules on Test Plan: - Existing unit tests should continue to pass - New cases for valid and invalid combinations were addeed Notes: - This is a pre-requiste for #5381 which adds the public API for multi-desintations

kwridan

Thanks @waltflanagan 🙏

I'll experiment locally to see how much of a lift would the refactor of the graph types to account for platforms be if we think that is a valid path.

I think we can split this (I appreciate it further delays getting a working version 🙈)

Option A - Exposing API first to allow limited usage + further testing

(1) Expose destinations in ProjectDescription and map using existing logic
(2) Add platformFilters support to dependencies etc...

Option B - Internally refactor to support platformFilters on dependencies ahead of exposing API

(1) Propagating platformFilters internally within Graph / Models
(2) Exposing API in ProjectDescription

Sources/TuistCore/Graph/GraphLoader.swift

projects/tuist/fixtures/multiplatform_app_with_sdk/Project.swift

kwridan · 2023-10-24T13:57:58Z

Sources/TuistCore/Graph/GraphTraverser.swift

            )
        case let .target(name, path):
            guard let target = target(path: path, name: name) else { return nil }
            return .product(
                target: target.target.name,
                productName: target.target.productNameWithExtension,
-                platformFilters: target.target.dependencyPlatformFilters
+                platformFilters: target.target.dependencyPlatformFilters // What to do here now that we have smart things?


we have a few options:

We can either omit platformFilters from product dependencies (i.e. .target / .project) dependencies and automatically derive this as we do today on main

if we expose platformFilters we'd have to do some form of intersection between the user specified filters (which may contain errors - possibly lint warn / error ahead of time) + valid filters we determine

…m:)` to resolve filters

…ests`

This will filter out windows, linux, and other platforms when a dependency is exclusively conditional on them.

waltflanagan · 2023-11-20T00:36:42Z

Sources/ProjectDescription/Platform.swift

@@ -15,3 +15,19 @@ public enum Platform: String, Codable, Equatable, CaseIterable {
    /// The visionOS platform
    case visionOS = "visionos"
 }
+
+/// A supported platform representation.
+public enum PackagePlatform: String, Codable, Equatable, CaseIterable {


This was introduced because SPM includes catalyst as a platform while many other parts of our system map catalyst to iOS. This was a necessary distinction when inferring destinations from a SPM package.

kwridan reviewed Aug 31, 2023

View reviewed changes

pepicrft reviewed Sep 4, 2023

View reviewed changes

waltflanagan force-pushed the multi-platform/Phase3 branch from e24a1d0 to 5e31c9b Compare September 11, 2023 14:27

kwridan mentioned this pull request Sep 22, 2023

Add supported platforms build setting when specifying multiple destinations #5438

Merged

6 tasks

kwridan mentioned this pull request Sep 25, 2023

Support linting multi-destination targets #5441

Merged

6 tasks

waltflanagan force-pushed the multi-platform/Phase3 branch from 98cc77c to a9621b5 Compare October 10, 2023 16:40

waltflanagan force-pushed the multi-platform/Phase3 branch from e697373 to 3b47e00 Compare October 23, 2023 12:36

kwridan mentioned this pull request Oct 24, 2023

Support multiplatform Targets #5515

Closed

kwridan reviewed Oct 24, 2023

View reviewed changes

Sources/TuistCore/Graph/GraphLoader.swift Outdated Show resolved Hide resolved

projects/tuist/fixtures/multiplatform_app_with_sdk/Project.swift Outdated Show resolved Hide resolved

kwridan reviewed Oct 24, 2023

View reviewed changes

danieleformichelli linked an issue Oct 24, 2023 that may be closed by this pull request

Support multiplatform Targets #5515

Closed

kwridan mentioned this pull request Nov 2, 2023

Fix platformFilter usage for single filters #5544

Merged

6 tasks

waltflanagan force-pushed the multi-platform/Phase3 branch 6 times, most recently from 8288fe1 to 44b9a81 Compare November 10, 2023 20:13

waltflanagan changed the base branch from main to multi-platform/GraphEdges November 10, 2023 20:14

kwridan mentioned this pull request Nov 14, 2023

[Multiplatform] Update Graph to store filter data in edges. #5557

Merged

6 tasks

waltflanagan force-pushed the multi-platform/GraphEdges branch from b5980f7 to 2282fa8 Compare November 14, 2023 13:53

waltflanagan and others added 19 commits November 18, 2023 10:35

Update directStaticDependencies to use `dependencyReference(to: fro…

baed57a

…m:)` to resolve filters

Fix settings resolution to correctly merge settings dictionaries

8779722

Fix rebase issue with test data

f5372d5

Fix tuist generate -p to include multiplatform targets

170a041

Use ProjectDescription platforms to ease use in `PackageInfoMapperT…

3883901

…ests`

Add missingn linting on multiplatform targets

f16713f

Update tests for multiplatform dependencies

dc7abcc

Linting

e50be1c

Remove unneeded parameters for platforms

46c653f

Skip dependencies that are targeting unsupported platforms

f7f6339

This will filter out windows, linux, and other platforms when a dependency is exclusively conditional on them.

FIx typo

d4de36d

Fixing rebase issues

671bff1

Update multiplatform fixture location and add multiplatform dependency

2a4192f

Fix invalid dependency error to work with multiplatform dependencies

298d917

Remove things that should be ignored

7b3a04f

Ignore dependencies artifacts

5a234c7

Rebase fixes

f3dc7ee

Fix rebase and use .all instead of []

2238b7a

Adding TargetDependency filter tests

0c16654

waltflanagan force-pushed the multi-platform/Phase3 branch from 59dc6a4 to 0c16654 Compare November 18, 2023 18:49

waltflanagan commented Nov 20, 2023

View reviewed changes

waltflanagan added 4 commits November 20, 2023 10:47

Update dependency finding logic to match only on name

71f625f

Add missed packagePlugin filter application

7890331

Match resource filter to targets filters

68158ec

Fix small issue mapping that could have been dropping filters

b0840d7

waltflanagan force-pushed the multi-platform/GraphEdges branch 2 times, most recently from 15c5268 to 842c4a6 Compare November 27, 2023 22:21

Base automatically changed from multi-platform/GraphEdges to main November 28, 2023 15:23

waltflanagan closed this Nov 28, 2023

waltflanagan deleted the multi-platform/Phase3 branch November 28, 2023 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiplatform Phase 3 - Expose support in `ProjectDescription` #5381

Multiplatform Phase 3 - Expose support in `ProjectDescription` #5381

waltflanagan commented Aug 28, 2023

kwridan left a comment

kwridan Aug 31, 2023

waltflanagan Aug 31, 2023

kwridan Sep 1, 2023

pepicrft Sep 4, 2023

waltflanagan Sep 12, 2023

pepicrft Sep 4, 2023 •

edited

kwridan left a comment

kwridan Oct 24, 2023

waltflanagan Nov 20, 2023

Multiplatform Phase 3 - Expose support in ProjectDescription #5381

Multiplatform Phase 3 - Expose support in ProjectDescription #5381

Conversation

waltflanagan commented Aug 28, 2023

Short description 📝

How to test the changes locally 🧐

Contributor checklist ✅

Reviewer checklist ✅

kwridan left a comment

Choose a reason for hiding this comment

kwridan Aug 31, 2023

Choose a reason for hiding this comment

waltflanagan Aug 31, 2023

Choose a reason for hiding this comment

kwridan Sep 1, 2023

Choose a reason for hiding this comment

pepicrft Sep 4, 2023

Choose a reason for hiding this comment

waltflanagan Sep 12, 2023

Choose a reason for hiding this comment

pepicrft Sep 4, 2023 • edited

Choose a reason for hiding this comment

kwridan left a comment

Choose a reason for hiding this comment

kwridan Oct 24, 2023

Choose a reason for hiding this comment

waltflanagan Nov 20, 2023

Choose a reason for hiding this comment

Multiplatform Phase 3 - Expose support in `ProjectDescription` #5381

Multiplatform Phase 3 - Expose support in `ProjectDescription` #5381

pepicrft Sep 4, 2023 •

edited