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
Fix integration of SPM packages with slashes in their targets names #6260
Fix integration of SPM packages with slashes in their targets names #6260
Conversation
84dca6d
to
e27c85f
Compare
I think the same happens with whitespaces - example package. |
b83ea92
to
85f02d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR, a couple of comments, but nothing major π
Tests/TuistDependenciesAcceptanceTests/DependenciesAcceptanceTests.swift
Outdated
Show resolved
Hide resolved
Tests/TuistDependenciesAcceptanceTests/DependenciesAcceptanceTests.swift
Outdated
Show resolved
Hide resolved
fixtures/ios_app_with_spm_dependencies/App/Sources/ContentView.swift
Outdated
Show resolved
Hide resolved
let sanitizedTargetName = targetName | ||
.replacingOccurrences(of: "-", with: "_") | ||
.replacingOccurrences(of: "/", with: "_") | ||
let expectedBundleName = "\(projectName)_\(sanitizedTargetName)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably hardcode this. The less business logic in tests the better.
let expectedBundleName = "\(projectName)_\(sanitizedTargetName)" | |
let expectedBundleName = "sdk_with_dash_target_with_dash" |
@@ -594,9 +595,9 @@ public final class PackageInfoMapper: PackageInfoMapping { | |||
product: product, | |||
productName: PackageInfoMapper | |||
.sanitize(targetName: target.name) | |||
.replacingOccurrences(of: "-", with: "_"), | |||
.replacingOccurrences(of: "-", with: "_").replacingOccurrences(of: "/", with: "_"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the sanitize
method already do the replacement?
let sanitizedTargetName = target.name | ||
.replacingOccurrences(of: "-", with: "_") | ||
.replacingOccurrences(of: "/", with: "_") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given in how many places we needed to update this, I wonder if we should extract this into a helper method such as:
extension TuistGraph.Target {
static func sanitizedTargetName(_ targetName: String) -> String { ... }
}
f60aa56
to
b5a3707
Compare
@fortmarek I've rebased and made some fixes. Still have no idea what's happening on CI tho. All tests pass fine locally. |
The main part of the crash is |
337fc02
to
4b64efd
Compare
Yay thank you @fortmarek this helped a lot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great findings @kapitoshka438 π
@fortmarek when can we expect to see this bugfix in release and which version? |
We try to release weekly on Monday, i.e., there should be |
Resolves #6145
Short description π
Although targets names and bundle identifiers have to be Uniform Type Identifiers, SPM is able to process some non-compliant targets such as those that contain a slash "/" in their name just by replacing it with an underscore "_".
This PS brings the same to enable Tuist to work with dependencies like KSCrash.
How to test the changes locally π§
KSCrash was added as an SPM dependency in the
app_with_spm_dependencies
fixture.Contributor checklist β
mise run lint:fix
Reviewer checklist β
changelog:added
,changelog:fixed
, orchangelog:changed
, and the title is usable as a changelog entry