Add Linting to require conditions for multiplatform dependencies with mismatched platforms. #6251
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…upported all the required platforms of a target.
|
@waltflanagan Thanks a lot for taking care of this issue. So basically you introduced a linting rule whenever a depdendency without conditions is linked to a target which supported set destinations is a superset of of the linked target and the target is not linked with any conditions did I see that correct ? |
alexanderwe
reviewed
May 5, 2024
| if !unaccountedPlatforms.isEmpty { | ||
| let missingPlatforms = unaccountedPlatforms.map(\.rawValue).joined(separator: ", ") | ||
| return [LintingIssue( | ||
| reason: "Target \(fromTargetName) with depends on \(dependentTarget.target.name) and but does not support the required platforms \(missingPlatforms). This dependency requires a condition.", |
There was a problem hiding this comment.
What do you think of making this message a little more verbose ?
Suggested change
| reason: "Target \(fromTargetName) with depends on \(dependentTarget.target.name) and but does not support the required platforms \(missingPlatforms). This dependency requires a condition.", | |
| reason: "Target \(fromTargetName) which \(dependentTarget.target.name) is depending on, does not support the required platforms \(missingPlatforms). \(fromTargetName) requires a condition which restricts it to at most \(missingPlatforms)", |
6 tasks
waltflanagan
requested review from
a team and
fortmarek
and removed request for
a team
fortmarek
approved these changes
May 6, 2024
pepicrft
approved these changes
May 6, 2024
pepicrft
added
changelog:changed
alexanderwe
approved these changes
May 6, 2024
Co-authored-by: Marek Fořt <marekfort@me.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #5978.
Short description 📝
Adding linting to call out when a platform condition is required. It's more complicated than just checking the set of platforms of each target because some target relationships (iOS app depending on WatchOS App) are valid. Here we constrain the linting logic to just linkable targets but we could include
How to test the changes locally 🧐
Tests were added to replicate the issue and are passing with the addition of the linting rules.
Contributor checklist ✅
mise run lint:fixReviewer checklist ✅
changelog:added,changelog:fixed, orchangelog:changed, and the title is usable as a changelog entry