refactor!: strongly typed values in `BuildSettings` and `BuildFileSettings` #903

waltflanagan · 2025-02-17T18:26:06Z

Short description 📝

Mostly work that was done on the 9.0.0 branch that shouldn't be coupled to Sendability changes.

This introduces the BuildSetting type that will allow safer manipulation of build settings and set us with more typesafety when working with build settings that have constraints.

Keys issues are that this introduces source breaking changes with the signature of the BuildSettings typealias.

- Updates to xcdiff for compatibility with tuist/XcodeProj#903 - Build setting values are no longer type erased and instead can now either be a `String` or `[String]` - This alleviates the need to casting values and throwing errors when dealing with build settings Test Plan: - Verify CI passes Signed-off-by: Kassem Wridan <[email protected]>

kwridan

Thanks @waltflanagan this is a much needed change! 👌

Indeed this is source breaking and would likely need a major release

e.g. updates needed for xcdiff bloomberg/xcdiff#145

kwridan · 2025-02-19T08:52:03Z

Tests/XcodeProjTests/Utils/BuildSettingsProviderTests.swift

+//    func XCTAssertEqual(_ lhs: BuildSettings, _ rhs: BuildSettings, file: StaticString = #file, line: UInt = #line) {
+//        XCTAssertEqual(lhs as NSDictionary,
+//                       rhs as NSDictionary,
+//                       file: file,
+//                       line: line)
+//    }


Is this still needed?

probably not

then let's remove it? 😁

waltflanagan · 2025-02-19T14:03:39Z

@kwridan thats what i was afraid of. I'll see if i can find some time to put together a 9.0 checklist so we can identify anything else that we may want to break There are some other Any collections but those seem like they could follow a similar approach to these callsites.

Do you think it would be worth a transitional phase that preserves BuildSettings as [String: Any] but introduces strongly typed backing stores and parsing to ease into a 9.0 later? It could be something like typealias TypedBuildSettings = [String: BuildSetting] and we would have interfaces that expose the legacy BuildSetting type but convert to TypedBuildSettings.

kwridan · 2025-02-20T08:04:33Z

I wouldn't shy away from a a major release for this per se as there are already a few compatibility APIs lying around that could do with a clean up:

e.g.

https://github.com/tuist/XcodeProj/blob/main/Sources/XcodeProj/Scheme/XCScheme%2BTestableReference.swift#L10

You are right, there are other collections (e.g. attributes) that could do with a similar update, as such to incrementally tackle them deferring the breaking changes such that they are all done together for the next major release makes sense.

Another accessor could be exposed with the strong types typedBuildSettings: TypedBuildSettings that could then be renamed back to buildSettings once the breaking changes are made, that could offer some value till then but at the slightly inconvenience of introducing new public API that is very short lived and would requires clients to migrate of off.

Sources/XcodeProj/Objects/Project/PBXProject.swift

fortmarek

Left some minor comments, but overall, I'm very much in favor of this. Getting rid of Any will be quite an improvement when using this library and has been long overdue. Thanks Mike!

fortmarek · 2025-02-27T13:50:58Z

Sources/XcodeProj/Objects/BuildPhase/PBXBuildFile.swift

@@ -195,3 +213,47 @@ final class PBXBuildPhaseFile: PlistSerializable, Equatable {
        lhs.buildFile == rhs.buildFile && lhs.buildPhase == rhs.buildPhase
    }
 }
+
+public enum BuildFileSetting: Sendable, Equatable {


nit: would move this to a separate file.

fortmarek · 2025-02-27T13:51:59Z

Sources/XcodeProj/Objects/BuildPhase/PBXBuildFile.swift

+    public var stringValue: String? {
+        if case let .string(value) = self {
+            value
+        } else {
+            nil
+        }
+    }
+
+    public var arrayValue: [String]? {
+        if case let .array(value) = self {
+            value
+        } else {
+            nil
+        }
+    }


where do we plan to use these? I wonder if it would be better for consumers to do the switch themselves. Anything we put here is something that we're committed to not break (or release a major version when we do)

Mostly convenience, the usages of these [String: Any] collections generally are casting as? <Type> and this felt like a reasonable convenience that would help minimize the disruption to current codebases (if case let syntax is not fun to work with).

not totally opposed, so we can keep these.

fortmarek · 2025-02-27T13:53:28Z

Tests/XcodeProjTests/Objects/Configuration/BuildFileSettingTests.swift

nit: for completely new test suites, I'd start using Swift testing instead. Eventually, we'd like to get to a place where all Tuist repositories use Swift testing, although that's going to be a long way.

Sounds good, another thing to learn about.

Sources/XcodeProj/Objects/BuildPhase/PBXBuildFile.swift

fortmarek · 2025-02-27T14:00:07Z

Sources/XcodeProj/Objects/Configuration/BuildSettings.swift

+    case string(String)
+    case array([String])
+
+    var valueForWriting: String {


What do you think of this using the CustomStringConvertible instead of a completely custom property?

That should work fine 👍

fortmarek · 2025-02-27T14:01:16Z

Sources/XcodeProj/Objects/Configuration/BuildSettings.swift

+    public var stringValue: String? {
+        if case let .string(value) = self {
+            value
+        } else {
+            nil
+        }
+    }
+
+    public var arrayValue: [String]? {


would probably also remove this in favor of consumers doing the switch themselves.

fortmarek · 2025-02-27T14:02:02Z

Sources/XcodeProj/Objects/Project/PBXProject.swift

@@ -602,3 +605,64 @@ extension PBXProject: PlistSerializable {
        })
    }
 }
+
+public enum ProjectAttribute: Sendable, Equatable {


nit: would also move this to its own file.

fortmarek · 2025-02-27T14:02:30Z

Sources/XcodeProj/Objects/Project/PBXProject.swift

+    public var stringValue: String? {
+        if case let .string(value) = self {
+            value
+        } else {
+            nil
+        }
+    }
+
+    public var arrayValue: [String]? {
+        if case let .array(value) = self {
+            value
+        } else {
+            nil
+        }
+    }


as mentioned above, would consider not including these.

fortmarek · 2025-02-27T14:04:05Z

Tests/XcodeProjTests/Utils/BuildSettingsProviderTests.swift

+//    func XCTAssertEqual(_ lhs: BuildSettings, _ rhs: BuildSettings, file: StaticString = #file, line: UInt = #line) {
+//        XCTAssertEqual(lhs as NSDictionary,
+//                       rhs as NSDictionary,
+//                       file: file,
+//                       line: line)
+//    }


then let's remove it? 😁

waltflanagan · 2025-02-27T18:36:06Z

@kwridan i tackled the other things you mentioned and could use a re-review. also tried to include some convenience methods to make the impact smaller from a migration perspective (avoiding the need for if case let)

Curious generally what else should be done to land this as a 9.0

…ization

This was fun. `NSDictionary(buildSettings)` is not able to be compared because we wrap swift values so we must use `==` on the swift types since they are not concrete and equatable.

The settings here are constrained to two cases, one as a string and one as a string array as defined here: https://buck.build/javadoc/com/facebook/buck/apple/xcode/xcodeproj/PBXBuildFile.html Given the narrow use case we should constraint the available types here to fit the need.

…nd not give an accurate failure count

6.0.3 is not supported and also doesnt have a fix we need.

This need a fix that is in swift 6.1 to pass on linux: swiftlang/swift-foundation#1002

kwridan

Thanks for the looking into the other parts @waltflanagan 🙏

I didn't necessarily mean they had to happen in the build settings PR in one go rather as separate PRs we can incrementally do for the 9.0 release. Is the worry the release process for XcoeProj would automatically ship this PR as an minor release?

The attribute updates may need a closer look to see how to deal with references correctly to ensure their final values are resolved - my thinking is potentially adding another case .targetReference() which would be accounted for.

Sources/XcodeProj/Objects/Project/PBXProject.swift

Sources/XcodeProj/Objects/Project/ProjectAttribute.swift

kwridan

marking as requesting changes per above (for the attributes + target reference issues)

thanks!

fortmarek · 2025-02-28T12:29:06Z

Is the worry the release process for XcoeProj would automatically ship this PR as an minor release?

That would actually happen. Given the amount of changes, I think it's fine to bundle everything here. The alternative would be to create a release branch, post individual PRs there, and then merge the release branch into main, so we end up with just one major release. But I'd say it's not worth the extra complexity.

Also removed `Encodable` conformance as we have a custom `plist` method that is used for writing.

waltflanagan · 2025-02-28T13:34:49Z

@kwridan just added the targetReference case and a test that covers calling fix after adding to the project. I think this covers the case you mentioned. strong types made it a quick change 🎉

One other risk of this branch is that it would block other fixes from shipping if this was merged but not quite ready to ship.

lmk next steps on releasing a 9.0.0, i was definitely going to find some time this weekend to check on the XcodeGraph and Tuist impacts of these changes to make sure they fit those needs. I can also see what kind of impact it would be on XcodeGen.

fortmarek · 2025-02-28T14:56:52Z

i was definitely going to find some time this weekend to check on the XcodeGraph and Tuist impacts of these changes to make sure they fit those needs. I can also see what kind of impact it would be on XcodeGen.

I think that having a draft PR accommodating the change in XcodeGraph and Tuist would be a good exercise before we merge this. If you would have time for that, that'd be amazing 🙏

fortmarek

This is good to from my side. But as mentioned, would recommend validating these changes in XcodeGraph and tuist/tuist before merging.

fortmarek · 2025-02-28T14:59:04Z

Sources/XcodeProj/Objects/BuildPhase/BuildFileSetting.swift

@@ -0,0 +1,43 @@
+public enum BuildFileSetting: Sendable, Equatable {


It's pretty self-explanatory but given this is public now, I'd add some basic documentation.

fortmarek · 2025-02-28T14:59:38Z

Sources/XcodeProj/Objects/Configuration/BuildSettings.swift

+private let yes = "YES"
+private let no = "NO"
+
+public enum BuildSetting: Sendable, Equatable {


Would also add some basic documentation.

fortmarek · 2025-02-28T15:03:22Z

Sources/XcodeProj/Objects/Configuration/BuildSettings.swift

+            case no: false
+            default: nil


Xcode's behavior is actually defaulting to false if the value is defined but it's not YES or NO. But conceptually, this feels better, so I'll let you decide what's better here 😅

I think the purposes of this utility it strictly checking for boolean build settings as opposed to attempting to resolve a boolean value for the build setting.

Makes sense!

kwridan

Thanks for the updates @waltflanagan

@fortmarek what's the process for marking this as a major release ahead of merging?

kwridan · 2025-03-06T13:20:45Z

Sources/XcodeProj/Objects/Configuration/BuildSettings.swift

+            case no: false
+            default: nil


I think the purposes of this utility it strictly checking for boolean build settings as opposed to attempting to resolve a boolean value for the build setting.

Sources/XcodeProj/Objects/Project/PBXProject.swift

fortmarek · 2025-03-17T18:08:46Z

what's the process for marking this as a major release ahead of merging?

I believe the only thing necessary is the exclamation mark already added by @waltflanagan

fortmarek · 2025-03-17T18:09:41Z

Thanks for the contribution @waltflanagan! And sorry for taking a bit longer to merge this.

waltflanagan · 2025-03-17T18:53:49Z

No worries! glad its moving along. I've had some other things going on as well. Thanks for merging!

waltflanagan requested a review from fortmarek February 17, 2025 18:26

waltflanagan changed the title ~~Strongly Typed values in BuildSettings~~ refactor: Strongly Typed values in BuildSettings Feb 18, 2025

waltflanagan changed the title ~~refactor: Strongly Typed values in BuildSettings~~ refactor: strongly typed values in BuildSettings and BuildFileSettings Feb 18, 2025

waltflanagan force-pushed the waltflanagan/StrongTypes branch 2 times, most recently from a28c2ff to bf96639 Compare February 19, 2025 01:48

kwridan mentioned this pull request Feb 19, 2025

Update for XcodeProj with strongly typed settings bloomberg/xcdiff#145

Draft

kwridan approved these changes Feb 19, 2025

View reviewed changes

waltflanagan mentioned this pull request Feb 22, 2025

[feedback wanted] refactor: Limit BuildSettings usage to only String and [String] values tuist/XcodeGraph#139

Closed

waltflanagan commented Feb 22, 2025

View reviewed changes

Sources/XcodeProj/Objects/Project/PBXProject.swift Show resolved Hide resolved

fortmarek changed the title ~~refactor: strongly typed values in BuildSettings and BuildFileSettings~~ refactor!: strongly typed values in BuildSettings and BuildFileSettings Feb 27, 2025

fortmarek reviewed Feb 27, 2025

View reviewed changes

waltflanagan requested a review from kwridan February 27, 2025 18:30

waltflanagan added 14 commits February 27, 2025 18:59

Introduce BuildSetting enum to enable strongly typed setting serial…

e52015a

…ization

Migrate BuildSettings to use BuildSetting enum.

dc0187c

Remove unused method that was the result of another PR’s rebase.

80c6dee

Fix equality on XCBuildConfiguration

4c49b9c

Update PlistValue to write BuildSetting correctly

0d54629

Fix Tests

dd33958

Fix tests

091d6c4

This was fun. `NSDictionary(buildSettings)` is not able to be compared because we wrap swift values so we must use `==` on the swift types since they are not concrete and equatable.

Fix tests

2dfe8f2

Linting

9e2a6cb

Allow string literal interpolation

f0b621c

Add tests

098cdb5

Linting

519c511

Fix pbxbuildfile equality

8e32b39

waltflanagan added 10 commits February 27, 2025 18:59

Add bool bridging to BuildSetting

f2098b7

Update to Swift 6

6dac103

Update from #file to #filePath for swift 6

2fea426

Try to get the right swift version on linux

16e9412

Maybe 6.0.3 fixes the issue

0f3a60e

Migrate to XCTUnwrap to avoid ! which will crash the test suite a…

7f7aaca

…nd not give an accurate failure count

Revert to 6.0.2

09b3fac

6.0.3 is not supported and also doesnt have a fix we need.

Fix tests

5cf9160

Exclude tests that need plist serialization from linux

2461290

This need a fix that is in swift 6.1 to pass on linux: swiftlang/swift-foundation#1002

Lint fixes

648909e

waltflanagan force-pushed the waltflanagan/StrongTypes branch from 875cafa to 648909e Compare February 28, 2025 00:02

moar linting

c4eb844

kwridan reviewed Feb 28, 2025

View reviewed changes

Sources/XcodeProj/Objects/Project/PBXProject.swift Show resolved Hide resolved

Sources/XcodeProj/Objects/Project/ProjectAttribute.swift Show resolved Hide resolved

Sources/XcodeProj/Objects/Project/ProjectAttribute.swift Show resolved Hide resolved

kwridan requested changes Feb 28, 2025

View reviewed changes

Add specific targetReference to ProjectAttribute

b8aeb90

Also removed `Encodable` conformance as we have a custom `plist` method that is used for writing.

dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Feb 28, 2025

Linting

5d26cf5

fortmarek approved these changes Feb 28, 2025

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Feb 28, 2025

Add Hashable and literal expressibility for Tuist Tests

d04f4ff

kwridan approved these changes Mar 7, 2025

View reviewed changes

Linting

3945b8c

fortmarek merged commit a9eb6af into main Mar 17, 2025
9 checks passed

fortmarek deleted the waltflanagan/StrongTypes branch March 17, 2025 18:10

		@@ -0,0 +1,43 @@
		public enum BuildFileSetting: Sendable, Equatable {

refactor!: strongly typed values in BuildSettings and BuildFileSettings #903

refactor!: strongly typed values in BuildSettings and BuildFileSettings #903

Conversation

waltflanagan commented Feb 17, 2025

Short description 📝

kwridan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waltflanagan commented Feb 19, 2025

kwridan commented Feb 20, 2025 • edited Loading

fortmarek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waltflanagan commented Feb 27, 2025

kwridan left a comment

Choose a reason for hiding this comment

kwridan left a comment

Choose a reason for hiding this comment

fortmarek commented Feb 28, 2025

waltflanagan commented Feb 28, 2025

fortmarek commented Feb 28, 2025

fortmarek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwridan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortmarek commented Mar 17, 2025

fortmarek commented Mar 17, 2025

waltflanagan commented Mar 17, 2025

refactor!: strongly typed values in `BuildSettings` and `BuildFileSettings` #903

refactor!: strongly typed values in `BuildSettings` and `BuildFileSettings` #903

kwridan commented Feb 20, 2025 •

edited

Loading