Allow specifying explicit product destinations in PackageSettings #6073

fortmarek · 2024-03-13T15:55:11Z

Resolves https://github.com/tuist/tuist-cloud/issues/153

Short description 📝

Let's take the following Package.swift:

import PackageDescription

let package = Package(
    name: "MyPackage",
    products: [
        .library(
            name: "MyPackage",
            targets: ["MyPackage"]
        ),
        .library(
            name: "MyUIKitPackage",
            targets: ["MyUIKitPackage"]
        ),
    ],
    targets: [
        .target(
            name: "MyPackage"
        ),
        .target(
            name: "MyUIKitPackage"
        ),
    ]
)

In this example, we have two products – MyPackage and MyUIKitPackage. MyPackage supports all destinations, whereas MyUIKitPackage only support iOS-based platforms since it depends on UIKit (which is not available on platforms like macOS).

If you try to use e.g. tuist cache when developing a package, we will try to cache all platforms since packages implicitly support all platforms. To explicitly encode in Package.swift if some of your products support only some destinations, we're adding a new property of PackageSettings called productDestinations. For the above example, you could encode that MyUIKitPackage supports only iOS-based platforms by:

#if TUIST
    import ProjectDescription

    let packageSettings = PackageSettings(
        productDestinations: [
            "MyUIKitPackage": [
                .iPad,
                .iPhone,
            ],
        ]
    )
#endif

How to test the changes locally 🧐

Run tuist generate in the spm_package fixture. MyPackage target should have all destinations whereas MyUIKitPackage only iPhone and iPad.

Contributor checklist ✅

The code has been linted using run mise run lint: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

danieleformichelli · 2024-03-13T19:49:15Z

Aren't target platform determined depending on which targets depend on the product?

pepicrft · 2024-03-14T10:57:50Z

Aren't target platform determined depending on which targets depend on the product?

This is where you have a Tuist project depending on Swift targets. I believe the scenario @fortmarek addresses here is where you run Tuist against a Swift Package. In that scenario, we have no information that allows us to narrow-down the list of platforms.

pepicrft · 2024-03-14T10:58:43Z

Sources/ProjectDescription/PackageSettings.swift

@@ -29,6 +29,12 @@ public struct PackageSettings: Codable, Equatable {
    /// The custom `Product` type to be used for SPM targets.
    public var productTypes: [String: Product]

+    /// Custom product destinations. This setting should only be used when using Tuist for SPM packages.


I'd mention first that the key in the dictionary represents the name of the product for which the destinations are defined.

pepicrft · 2024-03-14T10:59:19Z

Sources/TuistKit/ProjectEditor/ProjectEditor.swift

@@ -175,7 +175,7 @@ final class ProjectEditor: ProjectEditing {

        /// We error if the user tries to edit a project in a directory where there are no editable files.
        if projectManifests.isEmpty, editablePluginManifests.isEmpty, helpers.isEmpty, templateSources.isEmpty,
-           resourceSynthesizers.isEmpty, stencils.isEmpty
+           resourceSynthesizers.isEmpty, stencils.isEmpty, packageManifestPath == nil


Why is this needed?

So users can run tuist edit on a project that has the Package.swift manifest only

pepicrft · 2024-03-14T11:08:07Z

Tests/TuistAcceptanceTests/EditAcceptanceTests.swift

@@ -40,6 +40,14 @@ final class EditAcceptanceTestAppWithSPMDependencies: TuistAcceptanceTestCase {
    }
 }

+final class EditAcceptanceTestSPMPackage: TuistAcceptanceTestCase {


pepicrft · 2024-03-14T11:10:23Z

Tests/TuistKitTests/Services/InstallServiceTests.swift

@@ -53,6 +53,8 @@ final class InstallServiceTests: TuistUnitTestCase {
        // Given
        let stubbedPath = try temporaryPath()

+        manifestFilesLocator.locatePackageManifestStub = stubbedPath.appending(components: "Tuist", "Package.swift")


Have you considered taking the opportunity to use @Mockable with ManifestsFileLocating?

fortmarek · 2024-03-14T18:06:22Z

I believe the scenario @fortmarek addresses here is where you run Tuist against a Swift Package.

Yup, that's exactly the scenario this PR aims to fix 🙏

danieleformichelli · 2024-03-14T18:40:40Z

I believe the scenario @fortmarek addresses here is where you run Tuist against a Swift Package.

Yup, that's exactly the scenario this PR aims to fix 🙏

I see, are we ignoring the setting in case we are running against a tuist project instead?

danieleformichelli · 2024-03-14T18:42:13Z

Also, I need we need to support tuist editing Package.swift file alone if we want to go down this path, right?
(Maybe this PR does it already and I'm missing it)

fortmarek · 2024-03-15T08:39:29Z

I see, are we ignoring the setting in case we are running against a tuist project instead?

No, we're not. I feel it would be weird if that setting didn't do anything depending on if the Package.swift is a root one or not.

Also, I need we need to support tuist editing Package.swift file alone if we want to go down this path, right?
(Maybe this PR does it already and I'm missing it)

Yes, the PR already adds support for that.

danieleformichelli · 2024-03-15T08:52:44Z

No, we're not. I feel it would be weird if that setting didn't do anything depending on if the Package.swift is a root one or not.

I think we should.
The fact that we need this for Package.swift generation shouldn't impact (and I would say worsen) the "standard" generation

…endencies

fortmarek · 2024-03-15T17:46:04Z

The fact that we need this for Package.swift generation shouldn't impact (and I would say worsen) the "standard" generation

Sure, done!

Allow specifying explicit product destinations in PackageSettings

e32ee37

fortmarek added the changelog:added PR will be listed in the Added section of CHANGELOG label Mar 13, 2024

fortmarek requested review from danieleformichelli and pepicrft March 13, 2024 15:55

Fix InstallServiceTests

ba813c7

pepicrft approved these changes Mar 14, 2024

View reviewed changes

danieleformichelli approved these changes Mar 15, 2024

View reviewed changes

fortmarek added 2 commits March 15, 2024 18:40

Remove MockManifestFilesLocator in favor of Mockable mocks

ab1dbee

Apply productDestinations only to pure SPM projects, not external dep…

12898ff

…endencies

Fix unit tests due to missing mocks

039af35

fortmarek merged commit d10b1b3 into main Mar 18, 2024
8 checks passed

fortmarek deleted the spm/product-destinations branch March 18, 2024 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow specifying explicit product destinations in PackageSettings #6073

Allow specifying explicit product destinations in PackageSettings #6073

fortmarek commented Mar 13, 2024

danieleformichelli commented Mar 13, 2024

pepicrft commented Mar 14, 2024

pepicrft Mar 14, 2024

pepicrft Mar 14, 2024

fortmarek Mar 14, 2024

pepicrft Mar 14, 2024

pepicrft Mar 14, 2024

fortmarek commented Mar 14, 2024

danieleformichelli commented Mar 14, 2024

danieleformichelli commented Mar 14, 2024 •

edited

fortmarek commented Mar 15, 2024

danieleformichelli commented Mar 15, 2024

fortmarek commented Mar 15, 2024

Allow specifying explicit product destinations in PackageSettings #6073

Allow specifying explicit product destinations in PackageSettings #6073

Conversation

fortmarek commented Mar 13, 2024

Short description 📝

How to test the changes locally 🧐

Contributor checklist ✅

Reviewer checklist ✅

danieleformichelli commented Mar 13, 2024

pepicrft commented Mar 14, 2024

pepicrft Mar 14, 2024

Choose a reason for hiding this comment

pepicrft Mar 14, 2024

Choose a reason for hiding this comment

fortmarek Mar 14, 2024

Choose a reason for hiding this comment

pepicrft Mar 14, 2024

Choose a reason for hiding this comment

pepicrft Mar 14, 2024

Choose a reason for hiding this comment

fortmarek commented Mar 14, 2024

danieleformichelli commented Mar 14, 2024

danieleformichelli commented Mar 14, 2024 • edited

fortmarek commented Mar 15, 2024

danieleformichelli commented Mar 15, 2024

fortmarek commented Mar 15, 2024

danieleformichelli commented Mar 14, 2024 •

edited