Skip to content
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

Supports specifying multiple package products #1395

Merged
merged 7 commits into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Fixed

- Fixed source file `includes` not working when no paths were found #1337 @shnhrrsn
- Supports specifying multiple package products #1395 @simonbs

## 2.37.0

Expand Down
16 changes: 16 additions & 0 deletions Docs/ProjectSpec.md
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,7 @@ targets:

**Package dependency**
- [ ] **product**: **String** - The product to use from the package. This defaults to the package name, so is only required if a Package has multiple libraries or a library with a differing name
Copy link
Collaborator

@freddi-kit freddi-kit Aug 30, 2023

Choose a reason for hiding this comment

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

IMO, we can deprecate product and we can set products as standard way.

@yonaskolb what do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

I think we have to keep it around due to it's ability to set different options related to the dependency itself, like you mention below

- [ ] **products**: **String** - A list of products to use from the package. This can be used when depending on multiple products from a package.

```yaml
packages:
Expand All @@ -663,6 +664,21 @@ targets:
product: SPMUtility
```

Depending on multiple products from a package:

```yaml
packages:
FooFeature:
path: Packages/FooFeature
targets:
App:
dependencies:
- package: FooFeature
products:
- FooDomain
- FooUI
Comment on lines +675 to +679
Copy link
Collaborator

Choose a reason for hiding this comment

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

How can we handle if we want to do below case?

  • FooDomain: embed = true
  • FooUI: embed = false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking was that people would use product when needing fine-grained control and products (with an s) when products are to be added the same way.

dependencies:
- package: FooFeature
  product: FooDomain
  embed: true
- package: FooFeature
  product: FooUI
  embed: false

Alternatively, products should be objects as shown below while also supporting products as plain strings. If both aren't supported, the benefit of products become too little.

So both of these would be valid:

dependencies:
- package: FooFeature
  products:
  - name: FooDomain
    embed: true
  - name: FooUI
    embed: false
dependencies:
- package: FooFeature
  products:
  - FooDomain
  - FooUI

Copy link
Owner

Choose a reason for hiding this comment

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

@simonbs the embed property is part of the Dependency type, so the list of objects would break that relationship.
I think having to split out into seperate product is fine. Would like to see that perhaps mentioned in the docs under product like "Useful if you want to define different linking options per product. "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yonaskolb That's a good idea. I've added it in 9785749.

```

### Config Files

Specifies `.xcconfig` files for each configuration.
Expand Down
21 changes: 14 additions & 7 deletions Sources/ProjectSpec/Dependency.swift
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,17 @@ public struct Dependency: Equatable {
case framework
case carthage(findFrameworks: Bool?, linkType: CarthageLinkType)
case sdk(root: String?)
case package(product: String?)
case package(products: [String])
case bundle
}
}

extension Dependency {
public var uniqueID: String {
switch type {
case .package(let product):
if let product = product {
return "\(reference)/\(product)"
case .package(let products):
if !products.isEmpty {
return "\(reference)/\(products.joined(separator: ","))"
} else {
return reference
}
Expand Down Expand Up @@ -106,9 +106,16 @@ extension Dependency: JSONObjectConvertible {
type = .sdk(root: sdkRoot)
reference = sdk
} else if let package: String = jsonDictionary.json(atKeyPath: "package") {
let product: String? = jsonDictionary.json(atKeyPath: "product")
type = .package(product: product)
reference = package
if let products: [String] = jsonDictionary.json(atKeyPath: "products") {
type = .package(products: products)
reference = package
} else if let product: String = jsonDictionary.json(atKeyPath: "product") {
type = .package(products: [product])
reference = package
} else {
type = .package(products: [])
reference = package
}
} else if let bundle: String = jsonDictionary.json(atKeyPath: "bundle") {
type = .bundle
reference = bundle
Expand Down
69 changes: 39 additions & 30 deletions Sources/XcodeGenKit/PBXProjGenerator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -928,7 +928,7 @@ public class PBXProjGenerator {
}
}
// Embedding handled by iterating over `carthageDependencies` below
case .package(let product):
case .package(let products):
let packageReference = packageReferences[dependency.reference]

// If package's reference is none and there is no specified package in localPackages,
Expand All @@ -937,40 +937,49 @@ public class PBXProjGenerator {
continue
}

let productName = product ?? dependency.reference
let packageDependency = addObject(
XCSwiftPackageProductDependency(productName: productName, package: packageReference)
)
func addPackageProductDependency(named productName: String) {
let packageDependency = addObject(
XCSwiftPackageProductDependency(productName: productName, package: packageReference)
)

// Add package dependency if linking is true.
if dependency.link ?? true {
packageDependencies.append(packageDependency)
}
// Add package dependency if linking is true.
if dependency.link ?? true {
packageDependencies.append(packageDependency)
}

let link = dependency.link ?? (target.type != .staticLibrary)
if link {
let file = PBXBuildFile(product: packageDependency, settings: getDependencyFrameworkSettings(dependency: dependency))
file.platformFilter = platform
let buildFile = addObject(file)
targetFrameworkBuildFiles.append(buildFile)
} else {
let targetDependency = addObject(
PBXTargetDependency(platformFilter: platform, product: packageDependency)
)
dependencies.append(targetDependency)
let link = dependency.link ?? (target.type != .staticLibrary)
if link {
let file = PBXBuildFile(product: packageDependency, settings: getDependencyFrameworkSettings(dependency: dependency))
file.platformFilter = platform
let buildFile = addObject(file)
targetFrameworkBuildFiles.append(buildFile)
} else {
let targetDependency = addObject(
PBXTargetDependency(platformFilter: platform, product: packageDependency)
)
dependencies.append(targetDependency)
}

if dependency.embed == true {
let pbxBuildFile = PBXBuildFile(product: packageDependency,
settings: getEmbedSettings(dependency: dependency, codeSign: dependency.codeSign ?? true))
pbxBuildFile.platformFilter = platform
let embedFile = addObject(pbxBuildFile)

if dependency.copyPhase != nil {
customCopyDependenciesReferences.append(embedFile)
} else {
copyFrameworksReferences.append(embedFile)
}
}
}

if dependency.embed == true {
let pbxBuildFile = PBXBuildFile(product: packageDependency,
settings: getEmbedSettings(dependency: dependency, codeSign: dependency.codeSign ?? true))
pbxBuildFile.platformFilter = platform
let embedFile = addObject(pbxBuildFile)

if dependency.copyPhase != nil {
customCopyDependenciesReferences.append(embedFile)
} else {
copyFrameworksReferences.append(embedFile)
if !products.isEmpty {
for product in products {
addPackageProductDependency(named: product)
}
} else {
addPackageProductDependency(named: dependency.reference)
}
case .bundle:
// Static and dynamic libraries can't copy resources
Expand Down
6 changes: 3 additions & 3 deletions Tests/ProjectSpecTests/ProjectSpecTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,8 @@ class ProjectSpecTests: XCTestCase {
Dependency(type: .framework, reference: "dependency2"),

// multiple package dependencies with different products should be allowed
Dependency(type: .package(product: "one"), reference: "package1"),
Dependency(type: .package(product: "two"), reference: "package1"),
Dependency(type: .package(products: ["one"]), reference: "package1"),
Dependency(type: .package(products: ["two"]), reference: "package1"),
]
),
Target(
Expand Down Expand Up @@ -205,7 +205,7 @@ class ProjectSpecTests: XCTestCase {
sources: ["invalidSource"],
dependencies: [
Dependency(type: .target, reference: "invalidDependency"),
Dependency(type: .package(product: nil), reference: "invalidPackage"),
Dependency(type: .package(products: []), reference: "invalidPackage"),
],
preBuildScripts: [BuildScript(script: .path("invalidPreBuildScript"), name: "preBuildScript1")],
postCompileScripts: [BuildScript(script: .path("invalidPostCompileScript"))],
Expand Down
6 changes: 3 additions & 3 deletions Tests/ProjectSpecTests/SpecLoadingTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class SpecLoadingTests: XCTestCase {
"toReplace": Settings(dictionary: ["MY_SETTING2": "VALUE2"]),
]
try expect(project.targets) == [
Target(name: "IncludedTargetNew", type: .application, platform: .tvOS, sources: ["NewSource"], dependencies: [Dependency(type: .package(product: nil), reference: "Yams")]),
Target(name: "IncludedTargetNew", type: .application, platform: .tvOS, sources: ["NewSource"], dependencies: [Dependency(type: .package(products: []), reference: "Yams")]),
Target(name: "NewTarget", type: .application, platform: .iOS, sources: ["template", "target"]),
]
}
Expand All @@ -54,7 +54,7 @@ class SpecLoadingTests: XCTestCase {
"toReplace": Settings(dictionary: ["MY_SETTING2": "VALUE2"]),
]
try expect(project.targets) == [
Target(name: "IncludedTargetNew", type: .application, platform: .tvOS, sources: ["NewSource"], dependencies: [Dependency(type: .package(product: nil), reference: "SwiftPM"), Dependency(type: .package(product: nil), reference: "Yams")]),
Target(name: "IncludedTargetNew", type: .application, platform: .tvOS, sources: ["NewSource"], dependencies: [Dependency(type: .package(products: []), reference: "SwiftPM"), Dependency(type: .package(products: []), reference: "Yams")]),
Target(name: "NewTarget", type: .application, platform: .iOS, sources: ["template", "target"]),
]
}
Expand All @@ -70,7 +70,7 @@ class SpecLoadingTests: XCTestCase {
"toReplace": Settings(dictionary: ["MY_SETTING2": "VALUE2"]),
]
try expect(project.targets) == [
Target(name: "IncludedTargetNew", type: .application, platform: .tvOS, sources: ["NewSource"], dependencies: [Dependency(type: .package(product: nil), reference: "Yams")]),
Target(name: "IncludedTargetNew", type: .application, platform: .tvOS, sources: ["NewSource"], dependencies: [Dependency(type: .package(products: []), reference: "Yams")]),
Target(name: "NewTarget", type: .application, platform: .iOS, sources: ["template", "target"]),
]
}
Expand Down
2 changes: 1 addition & 1 deletion Tests/XcodeGenKitTests/GraphVizGeneratorTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ private let app = Target(
Dependency(type: .carthage(findFrameworks: true, linkType: .static), reference: "MyStaticFramework"),
Dependency(type: .carthage(findFrameworks: true, linkType: .dynamic), reference: "MyDynamicFramework"),
Dependency(type: .framework, reference: "MyExternalFramework"),
Dependency(type: .package(product: "MyPackage"), reference: "MyPackage"),
Dependency(type: .package(products: ["MyPackage"]), reference: "MyPackage"),
Dependency(type: .sdk(root: "MySDK"), reference: "MySDK"),
]
)
Expand Down
2 changes: 1 addition & 1 deletion Tests/XcodeGenKitTests/PBXProjGeneratorTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ class PBXProjGeneratorTests: XCTestCase {
let dependency1 = Dependency(type: .target, reference: "TestAll", platformFilter: .all)
let dependency2 = Dependency(type: .target, reference: "TestiOS", platformFilter: .iOS)
let dependency3 = Dependency(type: .target, reference: "TestmacOS", platformFilter: .macOS)
let dependency4 = Dependency(type: .package(product: "Swinject"), reference: "Swinject", platformFilter: .iOS)
let dependency4 = Dependency(type: .package(products: ["Swinject"]), reference: "Swinject", platformFilter: .iOS)
let target = Target(name: "Test", type: .application, platform: .iOS, sources: ["Sources"], dependencies: [dependency1, dependency2, dependency3, dependency4])
let swinjectPackage = SwiftPackage.remote(url: "https://github.com/Swinject/Swinject", versionRequirement: .exact("2.8.0"))
let project = Project(basePath: directoryPath, name: "Test", targets: [target, target1, target2, target3], packages: ["Swinject": swinjectPackage])
Expand Down
59 changes: 46 additions & 13 deletions Tests/XcodeGenKitTests/ProjectGeneratorTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -555,12 +555,12 @@ class ProjectGeneratorTests: XCTestCase {
Dependency(type: .target, reference: resourceBundle.name),
Dependency(type: .framework, reference: "FrameworkC.framework"),
Dependency(type: .carthage(findFrameworks: false, linkType: .dynamic), reference: "CarthageA"),
Dependency(type: .package(product: "RxSwift"), reference: "RxSwift"),
Dependency(type: .package(product: "RxCocoa"), reference: "RxSwift"),
Dependency(type: .package(product: "RxRelay"), reference: "RxSwift"),
Dependency(type: .package(products: ["RxSwift"]), reference: "RxSwift"),
Dependency(type: .package(products: ["RxCocoa"]), reference: "RxSwift"),
Dependency(type: .package(products: ["RxRelay"]), reference: "RxSwift"),

// Validate - Do not link package
Dependency(type: .package(product: "KeychainAccess"), reference: "KeychainAccess", link: false),
Dependency(type: .package(products: ["KeychainAccess"]), reference: "KeychainAccess", link: false),

// Statically linked, so don't embed into test
Dependency(type: .target, reference: staticLibrary.name),
Expand Down Expand Up @@ -1263,8 +1263,8 @@ class ProjectGeneratorTests: XCTestCase {
type: .application,
platform: .iOS,
dependencies: [
Dependency(type: .package(product: "ProjectSpec"), reference: "XcodeGen"),
Dependency(type: .package(product: nil), reference: "Codability"),
Dependency(type: .package(products: ["ProjectSpec"]), reference: "XcodeGen"),
Dependency(type: .package(products: []), reference: "Codability"),
]
)

Expand Down Expand Up @@ -1300,7 +1300,7 @@ class ProjectGeneratorTests: XCTestCase {
type: .application,
platform: .iOS,
dependencies: [
Dependency(type: .package(product: nil), reference: "XcodeGen"),
Dependency(type: .package(products: []), reference: "XcodeGen"),
]
)

Expand All @@ -1324,14 +1324,13 @@ class ProjectGeneratorTests: XCTestCase {
try expect(file.product?.productName) == "XcodeGen"
}


$0.it("generates local swift packages with custom xcode path") {
let app = Target(
name: "MyApp",
type: .application,
platform: .iOS,
dependencies: [
Dependency(type: .package(product: nil), reference: "XcodeGen"),
Dependency(type: .package(products: []), reference: "XcodeGen"),
]
)

Expand Down Expand Up @@ -1495,6 +1494,40 @@ class ProjectGeneratorTests: XCTestCase {

try expect(NSDictionary(dictionary: expectedInfoPlist).isEqual(to: infoPlist)).beTrue()
}

$0.it("generates local swift packages with multiple products") {
let app = Target(
name: "MyApp",
type: .application,
platform: .iOS,
dependencies: [
Dependency(type: .package(products: ["FooDomain", "FooUI"]), reference: "FooFeature")
]
)

let project = Project(name: "test", targets: [app], packages: [
"FooFeature": .local(path: "../FooFeature", group: nil)
], options: .init(localPackagesGroup: "MyPackages"))

let pbxProject = try project.generatePbxProj(specValidate: false)
let nativeTarget = try unwrap(pbxProject.nativeTargets.first(where: { $0.name == app.name }))
let localPackageFile = try unwrap(pbxProject.fileReferences.first(where: { $0.path == "../FooFeature" }))
try expect(localPackageFile.lastKnownFileType) == "folder"

let frameworkPhases = nativeTarget.buildPhases.compactMap { $0 as? PBXFrameworksBuildPhase }

guard let frameworkPhase = frameworkPhases.first else {
return XCTFail("frameworkPhases should have more than one")
}

guard let files = frameworkPhase.files, files.count == 2 else {
return XCTFail("frameworkPhase should have exactly two files")
}

let productNames = files.compactMap(\.product?.productName)
try expect(productNames).contains { $0 == "FooDomain" }
try expect(productNames).contains { $0 == "FooUI" }
}
}
}

Expand Down Expand Up @@ -2978,8 +3011,8 @@ class ProjectGeneratorTests: XCTestCase {

// given
let dependencies = [
Dependency(type: .package(product: "RxSwift"), reference: "RxSwift", embed: true),
Dependency(type: .package(product: "RxCocoa"), reference: "RxSwift", embed: false),
Dependency(type: .package(products: ["RxSwift"]), reference: "RxSwift", embed: true),
Dependency(type: .package(products: ["RxCocoa"]), reference: "RxSwift", embed: false),
]

// when
Expand All @@ -2993,8 +3026,8 @@ class ProjectGeneratorTests: XCTestCase {

// given
let dependencies = [
Dependency(type: .package(product: "RxSwift"), reference: "RxSwift", embed: true, copyPhase: BuildPhaseSpec.CopyFilesSettings(destination: .plugins, subpath: "test", phaseOrder: .postCompile)),
Dependency(type: .package(product: "RxCocoa"), reference: "RxSwift", embed: false, copyPhase: BuildPhaseSpec.CopyFilesSettings(destination: .plugins, subpath: "test", phaseOrder: .postCompile)),
Dependency(type: .package(products: ["RxSwift"]), reference: "RxSwift", embed: true, copyPhase: BuildPhaseSpec.CopyFilesSettings(destination: .plugins, subpath: "test", phaseOrder: .postCompile)),
Dependency(type: .package(products: ["RxCocoa"]), reference: "RxSwift", embed: false, copyPhase: BuildPhaseSpec.CopyFilesSettings(destination: .plugins, subpath: "test", phaseOrder: .postCompile)),
]

// when
Expand Down
2 changes: 1 addition & 1 deletion Tests/XcodeGenKitTests/SchemeGeneratorTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ class SchemeGeneratorTests: XCTestCase {
type: .application,
platform: .iOS,
dependencies: [
Dependency(type: .package(product: nil), reference: "XcodeGen")
Dependency(type: .package(products: []), reference: "XcodeGen")
],
scheme: targetScheme
)
Expand Down