Skip to content

Conversation

@vldalx
Copy link
Contributor

@vldalx vldalx commented Dec 5, 2024

Needed for tuist/tuist#7006

Target.supportsResources returns true if Target.product == .staticFramework.

Copy link
Member

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

Looks good.

Let's wait with the merge until the accompanying Tuist PR is merged: tuist/tuist#7006

Comment on lines 233 to 234
case .staticFramework:
return containsResources
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we just return true? supportsResources and containsResources are two different things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You've suggested on tuist/tuist#7006 to make .staticFramework embeddable only when it contains resources.
The easiest way to do so in the tuist's codebase is to consider .staticFramework as a framework that supports resources only when it really contains it.
Do you have doubts about this approach?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should still embed .staticFramework only if it contains resources. But this variable should always return true for .staticFramework, otherwise, we're changing the meaning of this variable for saving ourselves a couple of lines.

I'd check for containsResources in the place where we make the decision whether .staticFramework should be embedded or not.

Comment on lines 238 to 241
/// Returns true if the target contains resources or CoreData models
public var containsResources: Bool {
!resources.resources.isEmpty || !coreDataModels.isEmpty
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd keep this inside TuistCore as an extension. We're trying to keep the API surface of XcodeGraph smaller to make it easier not to make breaking changes here and to limit the need for cross-repo PRs.

@vldalx vldalx force-pushed the update_supportsResources branch from 555b8af to ba69c2d Compare December 13, 2024 18:26
@vldalx vldalx changed the title update Target.supportsResources supportsResources returns true if target is staticFramework Dec 13, 2024
@vldalx vldalx force-pushed the update_supportsResources branch from ba69c2d to 7ae5590 Compare December 16, 2024 18:25
@vldalx vldalx changed the title supportsResources returns true if target is staticFramework feat!: supportsResources returns true if product is staticFramework Dec 16, 2024
@fortmarek fortmarek changed the title feat!: supportsResources returns true if product is staticFramework feat: supportsResources returns true if product is staticFramework Dec 17, 2024
@fortmarek fortmarek merged commit 7c6584c into tuist:main Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants