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.

Sign up for GitHub

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

Swift Package fixes #444

Merged
merged 6 commits into from
Jun 16, 2019
+29 −27
Merged

Swift Package fixes #444

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9679ef4
make swift package inits and properties public
yonaskolb Jun 7, 2019
bb25dc6
add .swiftpm to .gitignore
yonaskolb Jun 7, 2019
2da634f
update changelog
yonaskolb Jun 8, 2019
6c9c400
make project packages public and non optional
yonaskolb Jun 8, 2019
371cd13
Make PBXTarget.packageProductDependencies non optional
yonaskolb Jun 10, 2019
22215cc
Make packageReferences attribute optional
Jun 16, 2019
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
2 changes: 1 addition & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.DS_Store
/.build
/Packages

.swiftpm
# Created by https://www.gitignore.io/api/macos,swift,xcode,appcode,swiftpm

### AppCode ###
Expand Down
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
- **Breaking** Added throwing an error in case group path can't be resolved by @damirdavletov
- **Breaking** Added remote project support to PBXContainerItemProxy by @damirdavletov
- **Breaking** Add support for `RemoteRunnable` https://github.com/tuist/xcodeproj/pull/400 by @pepibumur.
- Support for Swift Package Manager https://github.com/tuist/xcodeproj/pull/439 by @pepibumur.
- Support for Swift PM Packages https://github.com/tuist/xcodeproj/pull/439 https://github.com/tuist/xcodeproj/pull/444 by @pepibumur @yonaskolb.

### Fixed

Expand Down
22 changes: 11 additions & 11 deletions Sources/xcodeproj/Objects/Project/PBXProject.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,15 @@ public final class PBXProject: PBXObject {
}

/// Package references.
var packageReferences: [PBXObjectReference]?
Copy link
Contributor

Choose a reason for hiding this comment

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

What we've been doing for deciding whether a property should be optional or not is whether Xcode is able to open a project without that attribute. If I'm not wrong, Xcode is able to open a project that doesn't have this attribute. If that's the case, I'd keep this property as optional. It also ensures that we are not adding attributes to Xcode 10 projects that are are not readable by Xcode 10.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just changed it to an empty array if there is nothing to decode, and only encode if there's something in it. I'm not sure I understand you, does this mess up a specific use case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say this change makes sense - optional arrays are often troubling when it comes to accessing their contents. I can't see a situation where this will cause a problem.

If you want to check the presence of a package inside the Xcodeproj it's possible to by calling packageReferences.count > 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's take the following scenario:

  • The user edits a Xcode project that doesn't have the packageReferences attribute.
  • We decode the attribute to an empty array.
  • After saving the project, the user will see in the diff that a new attribute is added.

Xcode 10.x will ignore the attribute, but we are introducing unexpected modifications to the project. That's something we've always avoided, and I don't think it's solvable by not writing the array when it's empty, because that's another possible value for the attribute.

If I'm not wrong, I think Xcode 11 updates some attribute in the project to say that the project is compatible with version X of Xcode. What about using that value to decide whether the attribute should be written or not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pepibumur What do you mean by attribute? The packageReferences? If the array is empty we don't write it to the project, exactly as if it was nil

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let me put it on a table:

Project in disk Read project Diff?
No packageReferences Empty packageReferences Yes
Empty packageReferences Empty packageReferences No
Non-empty packageReferences Non-empty packageReferences No

By turning nil into an empty array we'll get a diff after writing the project to disk in the first scenario.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume'd we wouldn't write Empty packageReferences. Is there a benefit to doing so? If we assume empty means we don't write it then we don't have to worry about the truth table above since we only handle two scenarios, empty and non-empty

Copy link
Contributor

Choose a reason for hiding this comment

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

There isn't any difference for Xcode. The point is that users don't want to see git diffs if the project hasn't been modified, and with the current implementation they will. I suggest the following modification:

// This one remains optional
var packageReferences: [PBXObjectReference]?

// We define this one as non-optional
public var packages: [XCRemoteSwiftPackageReference] {
  set {
    packageReferences = newValue.references()
  }
  get {
     return packageReferences.objects() ?? []
  }
}

Does that look good @ollieatkinson / @yonaskolb ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense - 👍

var packageReferences: [PBXObjectReference]

/// Swift packages.
var packages: [XCRemoteSwiftPackageReference]? {
public var packages: [XCRemoteSwiftPackageReference] {
set {
packageReferences = newValue?.map { $0.reference }
packageReferences = newValue.references()
}
get {
return packageReferences?.objects()
return packageReferences.objects()
}
}

Expand Down Expand Up @@ -175,14 +175,12 @@ public final class PBXProject: PBXObject {
// Reference
let reference = XCRemoteSwiftPackageReference(repositoryURL: repositoryURL, versionRules: versionRules)
objects.add(object: reference)
if packages == nil { packages = [] }
packages?.append(reference)
packages.append(reference)

// Product
let productDependency = XCSwiftPackageProductDependency(productName: productName, package: reference)
objects.add(object: productDependency)
if target?.packageProductDependencies == nil { target?.packageProductDependencies = [] }
target?.packageProductDependencies?.append(productDependency)
target?.packageProductDependencies.append(productDependency)

// Build file
let buildFile = PBXBuildFile(product: productDependency)
Expand Down Expand Up @@ -223,6 +221,7 @@ public final class PBXProject: PBXObject {
projects: [[String: PBXFileElement]] = [],
projectRoots: [String] = [],
targets: [PBXTarget] = [],
packages: [XCRemoteSwiftPackageReference] = [],
attributes: [String: Any] = [:],
targetAttributes: [PBXTarget: [String: Any]] = [:]) {
self.name = name
Expand All @@ -237,6 +236,7 @@ public final class PBXProject: PBXObject {
projectReferences = projects.map { project in project.mapValues { $0.reference } }
self.projectRoots = projectRoots
targetReferences = targets.references()
packageReferences = packages.references()
self.attributes = attributes
targetAttributeReferences = [:]
super.init()
Expand Down Expand Up @@ -297,8 +297,8 @@ public final class PBXProject: PBXObject {
let targetReferences: [String] = (try container.decodeIfPresent(.targets)) ?? []
self.targetReferences = targetReferences.map { referenceRepository.getOrCreate(reference: $0, objects: objects) }

let packageRefeferenceStrings: [String]? = try container.decodeIfPresent(.packageReferences)
packageReferences = packageRefeferenceStrings?.map { referenceRepository.getOrCreate(reference: $0, objects: objects) }
let packageRefeferenceStrings: [String] = try container.decodeIfPresent(.packageReferences) ?? []
packageReferences = packageRefeferenceStrings.map { referenceRepository.getOrCreate(reference: $0, objects: objects) }

var attributes = (try container.decodeIfPresent([String: Any].self, forKey: .attributes) ?? [:])
var targetAttributeReferences: [PBXObjectReference: [String: Any]] = [:]
Expand Down Expand Up @@ -356,7 +356,7 @@ extension PBXProject: PlistSerializable {
return .string(CommentedString(targetReference.value, comment: target?.name))
})

if let packages = packages {
if !packages.isEmpty {
dictionary["packageReferences"] = PlistValue.array(packages.map {
.string(CommentedString($0.reference.value, comment: "XCRemoteSwiftPackageReference \"\($0.name ?? "")\""))
})
Expand Down
8 changes: 4 additions & 4 deletions Sources/xcodeproj/Objects/SwiftPackage/XCRemoteSwiftPackageReference.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,17 +111,17 @@ public class XCRemoteSwiftPackageReference: PBXContainerItem, PlistSerializable
}

/// Repository url.
var repositoryURL: String?
public var repositoryURL: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch! Good catch


/// Version rules.
var versionRules: VersionRules?
public var versionRules: VersionRules?

/// Initializes the remote swift package reference with its attributes.
///
/// - Parameters:
/// - repositoryURL: Package repository url.
/// - versionRules: Package version rules.
init(repositoryURL: String,
public init(repositoryURL: String,
versionRules: VersionRules? = nil) {
self.repositoryURL = repositoryURL
self.versionRules = versionRules
Expand All @@ -143,7 +143,7 @@ public class XCRemoteSwiftPackageReference: PBXContainerItem, PlistSerializable
}

/// It returns the name of the package reference.
var name: String? {
public var name: String? {
return repositoryURL?.split(separator: "/").last?.replacingOccurrences(of: ".git", with: "")
}

Expand Down
4 changes: 2 additions & 2 deletions Sources/xcodeproj/Objects/SwiftPackage/XCSwiftPackageProductDependency.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ public class XCSwiftPackageProductDependency: PBXContainerItem, PlistSerializabl
var packageReference: PBXObjectReference?

/// Package the product dependency refers to.
var package: XCRemoteSwiftPackageReference? {
public var package: XCRemoteSwiftPackageReference? {
get {
return packageReference?.getObject()
}
Expand All @@ -20,7 +20,7 @@ public class XCSwiftPackageProductDependency: PBXContainerItem, PlistSerializabl

// MARK: - Init

init(productName: String,
public init(productName: String,
package: XCRemoteSwiftPackageReference? = nil) {
self.productName = productName
packageReference = package?.reference
Expand Down
16 changes: 9 additions & 7 deletions Sources/xcodeproj/Objects/Targets/PBXTarget.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,15 @@ public class PBXTarget: PBXContainerItem {
}

/// Swift package product references.
var packageProductDependencyReferences: [PBXObjectReference]?
var packageProductDependencyReferences: [PBXObjectReference]

/// Swift packages products.
var packageProductDependencies: [XCSwiftPackageProductDependency]? {
public var packageProductDependencies: [XCSwiftPackageProductDependency] {
set {
packageProductDependencyReferences = newValue?.map { $0.reference }
packageProductDependencyReferences = newValue.references()
}
get {
return packageProductDependencyReferences?.objects()
return packageProductDependencyReferences.objects()
}
}

Expand All @@ -105,13 +105,15 @@ public class PBXTarget: PBXContainerItem {
buildPhases: [PBXBuildPhase] = [],
buildRules: [PBXBuildRule] = [],
dependencies: [PBXTargetDependency] = [],
packageProductDependencies: [XCSwiftPackageProductDependency] = [],
productName: String? = nil,
product: PBXFileReference? = nil,
productType: PBXProductType? = nil) {
buildConfigurationListReference = buildConfigurationList?.reference
buildPhaseReferences = buildPhases.references()
buildRuleReferences = buildRules.references()
dependencyReferences = dependencies.references()
self.packageProductDependencyReferences = packageProductDependencies.references()
self.name = name
self.productName = productName
productReference = product?.reference
Expand Down Expand Up @@ -157,8 +159,8 @@ public class PBXTarget: PBXContainerItem {
productReference = nil
}

let packageProductDependencyReferenceStrings: [String]? = try container.decodeIfPresent(.packageProductDependencies)
packageProductDependencyReferences = packageProductDependencyReferenceStrings?.map { objectReferenceRepository.getOrCreate(reference: $0, objects: objects) }
let packageProductDependencyReferenceStrings: [String] = try container.decodeIfPresent(.packageProductDependencies) ?? []
packageProductDependencyReferences = packageProductDependencyReferenceStrings.map { objectReferenceRepository.getOrCreate(reference: $0, objects: objects) }

productType = try container.decodeIfPresent(.productType)
try super.init(from: decoder)
Expand Down Expand Up @@ -195,7 +197,7 @@ public class PBXTarget: PBXContainerItem {
let fileElement: PBXFileElement? = productReference.getObject()
dictionary["productReference"] = .string(CommentedString(productReference.value, comment: fileElement?.fileName()))
}
if let packageProductDependencies = packageProductDependencies {
if !packageProductDependencies.isEmpty {
dictionary["packageProductDependencies"] = .array(packageProductDependencies.map {
PlistValue.string(.init($0.reference.value, comment: $0.productName))
})
Expand Down
4 changes: 2 additions & 2 deletions Sources/xcodeproj/Utils/ReferenceGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ final class ReferenceGenerator: ReferenceGenerating {
fixReference(for: project, identifiers: identifiers)

// Packages
project.packages?.forEach {
project.packages.forEach {
var identifiers = identifiers
identifiers.append($0.repositoryURL ?? $0.name ?? "")
fixReference(for: $0, identifiers: identifiers)
Expand All @@ -91,7 +91,7 @@ final class ReferenceGenerator: ReferenceGenerating {
identifiers.append(target.name)

// Packages
target.packageProductDependencies?.forEach {
target.packageProductDependencies.forEach {
var identifiers = identifiers
identifiers.append($0.productName)
fixReference(for: $0, identifiers: identifiers)
Expand Down