Generate Xcode projects using a layered data model instead of print statements #639

Merged
merged 1 commit into from Sep 13, 2016

Projects

None yet

3 participants

@abertelrud
Contributor
abertelrud commented Sep 6, 2016 edited

This is a first version of a more layered, robust, and readable Xcode project generation. The idea is to provide a simple representation of the Xcode project model, so that the logic that creates a project is readable, understandable, and maintainable. The actual generation of the project file is then a separate, testable layer, as is the transformation of the plist to text.

There are plenty more tests that should be added, and there are plenty of FIXMEs in the code for future improvements. There generation of schemes should also be redone in the same manner, but that is a much simpler model and completely separate from the project model. It will be done as a separate PR.

At this point the PR has be updated to the top-of-master, passes all unit tests (including the fixtures), is able to generate SwiftPM itself, and has resolved all known issues to handle all cases that the old implementation could. This should be a much better platform on which to do future enhancements and fixes.

Note: This code does not try to assign human-authored object identifiers. I went down that path at first, but it is a lot of effort on the part of the logic that creates the model for no apparent gain. We may all have our opinions about Xcode's use of opaque object identifiers in the project file, but those relate mostly to diffing project file changes in the case in which the project file is primary data. In this case the project is entirely generated and is not intended to be checked into an SCM, so there is less need to diff it. That said, the identifiers are of course entirely predictable, so that the exact same output is always created when generating the project file.

@ddunbar ddunbar commented on the diff Sep 8, 2016
Sources/Xcodeproj/XcodeProjectModel.swift
+ let phase = CopyFilesBuildPhase(dstDir: dstDir)
+ buildPhases.append(phase)
+ return phase
+ }
+
+ /// Adds a "shell script" build phase, i.e. one that runs a custom
+ /// shell script as part of the build.
+ public func addShellScriptBuildPhase(script: String) -> ShellScriptBuildPhase {
+ let phase = ShellScriptBuildPhase(script: script)
+ buildPhases.append(phase)
+ return phase
+ }
+
+ /// Adds a dependency on another target.
+ /// FIXME: We do not check for cycles. Should we? This is an extremely
+ /// minimal API so it's not clear that we should.
@ddunbar
ddunbar Sep 8, 2016 Member

I agree, doesn't seem necessary to me.

@abertelrud
abertelrud Sep 8, 2016 Contributor

Great. I'll remove the FIXME and add a note to the method documentation.

@ddunbar ddunbar and 1 other commented on an outdated diff Sep 8, 2016
Sources/Xcodeproj/XcodeProjectModelSerialization.swift
+ // FIXME: This is a suboptimal format for object identifier strings;
+ // for debugging purposes they should at least sort in numeric order.
+ let id = "OBJ_\(objsToIds.count + 1)"
+
+ objsToIds[serObjRef] = id
+ return id
+ }
+
+ /// Serializes `object` by asking it to construct a plist dictionary and
+ /// then adding that dictionary to the serializer. This may in turn cause
+ /// recursive invocations of `serialize(object:)`; the closure of these
+ /// invocations end up serializing the whole object graph.
+ @discardableResult func serialize(object: PropertyListSerializable) -> String {
+ let id = self.id(of: object)
+ precondition(idsToDicts[id] == nil, "tried to serialize \(object) twice")
+ idsToDicts[id] = .dictionary(object.serialize(to: self))
@ddunbar
ddunbar Sep 8, 2016 Member

Should this write a sentinel into idsToDicts[id] before invoking serialize in order to catch recursion?

@abertelrud
abertelrud Sep 8, 2016 Contributor

Good idea. Yes, definitely, that makes sense.

@ddunbar
Member
ddunbar commented Sep 8, 2016

This looks like a huge improvement, thank you very much for undertaking this... LGTM!

@abertelrud
Contributor

Awesome, thanks for taking a look. Yes, I think this will be much nicer to work with. I am going to implement your suggestion of a sentinel, and resolve some of the FIXMEs. And of course figure out what the remaining couple of unit test failures are all about, then I think this will be close to mergeable.

@abertelrud
Contributor

One of the recurring API questions with things like this is always whether the client:

a) creates an object (e.g. reference) and then adds it to the graph (e.g. a group in the tree)
or
b) invokes a method on an existing graph object (e.g. group) to create a new, already-attached object (e.g. reference)

In this case I started with a) but changed it to b) along the way. I think either is fine (and I'm not inclined to change course now with this one), but I'm curious about your opinion in general about this.

@ddunbar
Member
ddunbar commented Sep 8, 2016

I don't have a strong opinion, I know the issue you are talking about and have definitely used both approaches. My tendency these days would probably be towards (a) since it lends itself more easily to an immutable model where everything is built up piecemeal, and use (b) when doing so means some of the implementation details can be hid (e.g., if one of the objects is backed by something more complicated to init, but clients don't ever need to see it). As I said its not a strong opinion though...

@abertelrud
Contributor

Thanks for your comments: I have gone back and forth, but the deciding factor here is that I don't want to expose any details of what needs to be done to connect an independently created object to the graph. Right now the simplified model doesn't have to do anything tricky in terms of hiding ugly details (as you comment as a factor that would favor option b), but if we have to extend it in that direction, the present API should be able to remain unchanged.

@abertelrud
Contributor

I am wrapping this up and hope to land it today.

@abertelrud
Contributor

@swift-ci please test

@abertelrud abertelrud Generate Xcode projects using a layered data model instead of
a sequence of print statement.
4f1459e
@abertelrud
Contributor

Ready to land. There are more fixes that can be made but I have taken care of all known issues to have parity with the old implementation.

@abertelrud
Contributor

@swift-ci Please smoke test

@abertelrud abertelrud changed the title from WIP: Generate Xcode projects using a layered data model instead of print statements to Generate Xcode projects using a layered data model instead of print statements Sep 13, 2016
@shahmishal
Member

@swift-ci please test

@abertelrud abertelrud merged commit 5c26aa4 into apple:master Sep 13, 2016

2 checks passed

Swift Test Linux Platform Build finished.
Details
Swift Test OS X Platform Build finished.
Details
@abertelrud abertelrud deleted the abertelrud:xcodeproj-generation branch Sep 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment