Remove appIDHasFeaturesEnabled #41
Conversation
References: swiftlang#16
ekeege
requested review from
jakepetroules,
mirza-garibovic,
neonichu and
owenv
as code owners
There was a problem hiding this comment.
Thank you, this is a nice cleanup! I'll leave a few comments below where I think this change needs a few compatibility fixes to the message serialization format.
For context:
Generally speaking there are two types of compatibility we care about in Swift Build:
- ABI compatibility of the SwiftBuild module, which clients link directly
- Serialization format compatibility for messages passed between the SwiftBuild module and SWBBuildService, which may either be linked into the client or running in a separate process.
For case #2, we want to ensure that older versions of SwiftBuild are still able to communicate with SWBBuildService. That means that SWBBuildService should still tolerate receiving messages with the appIDHasFeaturesEnabled field, even though newer versions of the API will not include it.
Since the message in this PR is used by the client to communicate code-signing information to the build system, the swift package launch-xcode --disable-sandbox command described in the README is a good way to test it manually - serialization issues should generally appear as failures in the build log.
| } | ||
|
|
||
| public init(from deserializer: any Deserializer) throws { | ||
| let count = try deserializer.beginAggregate(20...22) | ||
| self.configurationName = try deserializer.deserialize() | ||
| self.appIDHasFeaturesEnabled = try deserializer.deserialize() |
There was a problem hiding this comment.
Here, we should check the count of fields received to determine whether appIDHasFeaturesEnabled is still present - if it is, we can immediately discard it
| @@ -157,7 +154,6 @@ public struct ProvisioningTaskInputsSourceData: Serializable, Equatable, Sendabl | |||
| public func serialize<T: Serializer>(to serializer: T) { | |||
| serializer.serializeAggregate(21) { | |||
There was a problem hiding this comment.
This count should be updated since one of the serialized fields was deleted
| @@ -67,7 +49,6 @@ extension ProvisioningSourceData: PendingSerializableCodable { | |||
| public func legacySerialize<T: Serializer>(to serializer: T) { | |||
| serializer.serializeAggregate(4) { | |||
There was a problem hiding this comment.
Similarly, the field count here should be updated
| @@ -76,7 +57,6 @@ extension ProvisioningSourceData: PendingSerializableCodable { | |||
| public init(fromLegacy deserializer: any Deserializer) throws { | |||
| let count = try deserializer.beginAggregate(4...5) | |||
| self.configurationName = try deserializer.deserialize() | |||
| self.appIDHasFeaturesEnabled = try deserializer.deserialize() | |||
There was a problem hiding this comment.
Similar to the above, we should check the count here to determine if we need to deserialize and then discard the now-unused field
Closes: #16