Helper methods required for adding build file to the target #213

ilyapuchka · 2018-01-09T18:35:04Z

Resolves #120, #40

Short description 📝

Added some helper methods required for adding build files

Solution 📦

With these methods adding a new build file requires just few calls:

// 1. get target
guard let target = project.pbxproj.objects.targets(named: "MyApp").first else { return }

// 2. if needed add new group for this file
let groups = project.pbxproj.objects.addGroup(named: "Sources/Home", to: project.pbxproj.rootGroup)

// 3. add file to the project, if needed
let (fileRef, _) = project.pbxproj.objects.addFile(at: "Sources/HomeViewController.swift")

// 4. add file to the group, if needed
project.pbxproj.objects.addFile(toGroup: groups[1].group, reference: fileRef)

// 5. add file to the target, if needed
project.pbxproj.objects.addBuildFile(toTarget: target, reference: fileRef)

If approved for merge I'll add some integration tests.

This change is

pepicrft · 2018-01-10T08:41:55Z

Sources/xcproj/PBXProj+Helpers.swift

+
+    /// Returns project's root group
+    public var rootGroup: PBXGroup {
+        let groupRef = objects.projects[rootObject]!.mainGroup


I'd make this property optional and remove the force unwraps from this getter

I was considering that of course but it will lead to always force unwrapping or checking for nil when using this API, which will make it inconvenient. Is it possible that a project can have no rootObject or group with such reference? If so probably it's a malformed project, so failing should be expected in this case or this should be validated even earlier. As a user I would prefer convenience over safety in this particular case.

I also don't like forced unwapping. It can be legitimately be missing when you are constructing project file from scratch. In that case it would be bad behaviour to crash in runtime. Instead this should be either optional variable or a function that throws an error.

I would create helper for accessing rootProject.

var rootProject: PBXProject? { return objects.projects[rootObject] }

This would make usage of rootObject different. Instead you would need to access it through project: .rootProject?.mainGroup. But it would be more clear what is optional.

Is it possible that a project can have no rootObject or group with such reference?

There are two usages of xcproj that I can think of right now:

Opening a project and modify it.

Create a new project.

In the first case, it's very unlikely that we force unwrap a nil value since the project already has a root group. However, in the second case, it might happen. This is a bad usage sign because the developer should have created the root group before accessing it.

It depends on whether we want the library to crash at runtime, or execute with unexpected behavior. Thinking about a developer consuming the API, I'd prefer the crash rather than spending time debugging where the unexpected behavior might be coming from.

What do you think @allu22 ?

Example where rootObject id is set and root project does not exist until generate is called:
https://github.com/yonaskolb/XcodeGen/blob/46b1512c7c4dde0621c25e4388d5e02823d3f6ab/Sources/XcodeGenKit/PBXProjGenerator.swift#L33

I think there is no right or wrong approach here. The question is, how defensive you want to be in the API. Either way is fine by me. But in case of unwrapping it here, maybe call fatalError with message that hints that you are missing a project object?

How about this?

public var rootGroup: PBXGroup { guard let rootProject = self.rootProject else { fatalError("Missing root project") } guard let rootGroup = objects.groups[rootProject.mainGroup] else { fatalError("Root project has no root group") } return rootGroup }

pepicrft · 2018-01-10T08:42:50Z

Sources/xcproj/PBXProjObjects+Helpers.swift

+    ///
+    /// - Parameter target: target object.
+    /// - Returns: target's sources build phase, if found.
+    public func sourcesBuildPhase(target: PBXTarget) -> PBXSourcesBuildPhase? {


Nice additions @ilyapuchka 👏

sourcesBuildPhases is deprecated. Instead you should use objects.sourcesBuildPhases.

this is exactly what is being used, this is extension on PBXObjects.

👍 sorry, missed that.

pepicrft · 2018-01-10T09:00:04Z

Sources/xcproj/PBXProjObjects+Helpers.swift

+    ///   - options: additional options, default is empty set.
+    /// - Returns: new group and its reference.
+    public func addGroup(
+        named groupName: String,


The problem that I see exposing this method with all these arguments is that if any property is introduced in the PBXGroup class, we need to update this method to support it. Given that xcproj represents the objects with mutable classes what do you think about simplifying it:

public func addGroup(named groupName: String, to toGroup: PBXGroup, withFolder: Bool = false) -> [(String, PBXGroup)]

Notice that the method returns an array with all the groups that got created. If the developer passes a path like this /group_a/group_b/group_c then it'd return [GroupA, GroupB, GroupC] corresponding the last element in the array to the last path component. If the developer is interested in updating let's say, the usesTabs property of all the created groups, it can be easily done iterating with a forEach on the returned array.

On the other side, I think the implementation could be simplified adding some recursion:

// Given the path /component_a/component_b/component_c 1. Generate/Get the first component group: GroupA 2. Concatenate the group with the result of calling the method with GroupA and name /component_b/component_c: [GroupA, result[0], result[1], ...]

Right, I didn't notice that those types are mutable classes, so it will makes sense to remove all those parameters. And I like your idea about returning array.

As Xcode allows you to use / in group name, i think this should not be used as separator.

Can you show an example? At least Xcode 9 does not allow to rename groups using /.

Once you point a group to a folder with name different than group name, you can change group name, without changing the folder name.
Example: https://imgur.com/oveZwsI

Ok. This seems to be allowed also when group is created without folder. What is the alternative then? We can require passing in array of group names instead. But then if / is used in one of them we should not create a group with such name if it is created with folder, as it will probably lead to invalid project. We should probably normalise each item. But if it is created without folder we should allow that.

pepicrft · 2018-01-10T09:02:28Z

Sources/xcproj/PBXProjObjects+Helpers.swift

+    ///
+    /// - Parameter target: target obejct
+    /// - Parameter reference: file reference
+    public func addBuildFile(toTarget target: PBXTarget, reference: String) {


What do you think about returning the PBXBuildFile here?

I'll return file and reference to match other APIs

pepicrft · 2018-01-10T09:13:57Z

Sources/xcproj/PBXProjObjects+Helpers.swift

+        xcLanguageSpecificationIdentifier: String? = nil
+        ) -> (String, PBXFileReference) {
+
+        if let existingFileReference = fileReferences.first(where: { $0.value.path == filePath.string }) {


I think this method does a lot of assumptions. Let's say we have the following file structure:

group_a/ group_b/ file_a

Where file_a is represented as a PBXFileReference with name file_a and sourceTree .group. file_a is a child of the group b.

Now you call this method saying that you want to add a file with path /group_a/group_b/file_a which already exists in our project. We won't get any existing reference when we actually should (because there's already a reference for that file). Moreover, if the path is relative the group, the group where reference gets added to as a child matters (and the developer might not know about it).

I'd aim for an implementation similar to the one above for groups. You'd call a method like this:

func addFile(named fileName: String, to toGroup: PBXGroup)

And it'd recursively generate the groups and file references and return them.

What do you think?

This falls into the problem of calculating file paths, indeed path property can be relative in this case and so will not match the absolute path in the predicate. Maybe there is another way to do such check that I'm missing?

Regarding your suggestion to create groups and files together in this method I was doing that originally, but decided to break it into separate methods not to violate single responsibility. And it will hide details of project structure from developer that you mentioned in the issue discussion saving only one call for the user. So I would keep it as it is, just remove additional parameters as you suggested for another method.

I think the safest way would be to compare the absolute paths but that's a bit expensive because you'd need to generate the absolute path for all the file references that you want to compare. We could use the method that you showed in another PR for getting the absolute path and optimize it. That way your first check would be legitimate.

Regarding your point on creating files and groups I agree with you so forget what I said. I wish we could leverage the type system to prevent the developer from passing a file name such as path/to/my/file.swift failing at compilation time. What do you think about throwing in case the string doesn't represent a file @ilyapuchka ?

I was thinking about using this method too, but yeah, it may be too expensive.
Regarding name we can make a check check if path is file path with path.isFile and then throw if it is not.

pepicrft

Good job @ilyapuchka. I just left a few comments regarding some API methods that I'm not very sure about. Thanks for tackling this since it's been in the backlog for quite some time.

I wonder what other @xcodeswift/contributors think about it.

alvarhansen · 2018-01-10T12:05:29Z

Sources/xcproj/PBXProj+Helpers.swift

+
+    /// Returns project's root group
+    public var rootGroup: PBXGroup {
+        let groupRef = objects.projects[rootObject]!.mainGroup


I also don't like forced unwapping. It can be legitimately be missing when you are constructing project file from scratch. In that case it would be bad behaviour to crash in runtime. Instead this should be either optional variable or a function that throws an error.

alvarhansen · 2018-01-10T12:16:51Z

Sources/xcproj/PBXProjObjects+Helpers.swift

+    ///
+    /// - Parameter target: target object.
+    /// - Returns: target's sources build phase, if found.
+    public func sourcesBuildPhase(target: PBXTarget) -> PBXSourcesBuildPhase? {


sourcesBuildPhases is deprecated. Instead you should use objects.sourcesBuildPhases.

alvarhansen · 2018-01-10T12:25:36Z

Sources/xcproj/PBXProjObjects+Helpers.swift

+    ///   - options: additional options, default is empty set.
+    /// - Returns: new group and its reference.
+    public func addGroup(
+        named groupName: String,


As Xcode allows you to use / in group name, i think this should not be used as separator.

…ile references

ilyapuchka · 2018-01-13T13:12:31Z

Rebased and addressed all the comments, except related to using / in the group names. I think we can just disallow that for now to make it simpler. It seems unlikely to me that using / in group names is a common practice.

alvarhansen · 2018-01-13T13:19:21Z

Sources/xcproj/PBXProjObjects+Helpers.swift

+    }
+}
+
+extension Path {


I think it should be made fileprivate or moved to separate file.

alvarhansen · 2018-01-13T13:27:32Z

Sources/xcproj/PBXProjObjects+Helpers.swift

+
+    /// Adds file at the give path to the project or returns existing file and its reference.
+    ///
+    /// - Parameters:


These parameter descriptions do not match function.

alvarhansen · 2018-01-13T13:31:18Z

Sources/xcproj/PBXProjObjects+Helpers.swift

+    /// Adds file to the given target's sources build phase or returns existing build file and its reference.
+    /// If target's sources build phase can't be found returns nil.
+    ///
+    /// - Parameter target: target obejct


type on work obejct

alvarhansen · 2018-01-13T13:34:19Z

Sources/xcproj/PBXProjObjects+Helpers.swift

+        return (reference, buildFile)
+    }
+
+    public func fullPath(fileElement: PBXFileElement, reference: String, sourceRoot: Path) -> Path? {


could you add few unit tests to this function. I am not sure what cases it has to cover and how. Especially with groups.

alvarhansen · 2018-01-13T13:42:29Z

Sources/xcproj/PBXProjObjects+Helpers.swift

+    ///   - sourceTree: file source tree, default is `.group`.
+    ///   - name: file name, by default gets file name from the path
+    /// - Returns: new or existing file and its reference.
+    public func addFile(at filePath: Path, sourceRoot: Path) throws -> (String, PBXFileReference) {


Please add unit test for this function. At the moment, I have a feeling that filePath == fullPath(fileElement: $0.value, reference: $0.key, sourceRoot: sourceRoot) check will fail unless filePath is initialized as absolute path. But it should not be a absolute path as PBXFileReference is created with sourceTree: .group

ilyapuchka · 2018-01-13T18:54:11Z

@allu22 done

alvarhansen · 2018-01-13T20:00:44Z

Sources/xcproj/PBXProjObjects+Helpers.swift

+        }
+
+        let fileReference = PBXFileReference(
+            sourceTree: .absolute,


Adding absolute path is very restrictive. This will restrict the usage of this project to single path in a filesystem. It would be useful only for situations where project is always generated.

I recommend rethinking this helper function.
My suggestion:
addFile function will take filePath and optional PBXGroup where nil will be a fallback to mainGroup.

@allu22 indeed! I improved this method by adding group and source tree parameters. Source tree is required to calculate initial value for file path property, as it will be different for different source tree values, i.e. it can be relative to group or to project. Also added helper method to calculate relative paths. I made it public as this method can be used for example to update file path when moving it to a different group.
Some of the source tree values are not supported in file path calculation (file will be added with nil path), but we can extend it later, it feels that use cases for these values should be rare.

alvarhansen · 2018-01-15T07:17:14Z

Sources/xcproj/PBXProjObjects+Helpers.swift

+    ///   - sourceRoot: path to project's source root.
+    /// - Returns: new or existing file and its reference.
+    public func addFile(at filePath: Path, toGroup: PBXGroup, sourceTree: PBXSourceTree = .group, sourceRoot: Path) throws -> (reference: String, file: PBXFileReference) {
+        guard filePath.isFile else {


Why is it necessary to check if the file really exists?

First to not make project buildable otherwise (it will not build if this file reference is then added to the build phase). Second to not allow paths to folders.

Actually you can non existing add file to build phase. This is common case with generated files. But as this is a helper function, I believe it is ok to make these kinds of checks here.

toshi0383 · 2018-01-15T11:08:45Z

Sources/xcproj/PBXProj+Helpers.swift

+    /// Returns root project's root group.
+    public var rootGroup: PBXGroup {
+        guard let rootProject = self.rootProject else {
+            fatalError("Missing root project")


Is fatalError suitable here?
Maybe we can make rootGroup optional (public var rootGroup: PBXGroup?).

I think it's more suitable than requiring to unwrap this property which will be nil only in specific situations when accessed before project is correctly setup. In my opinion it would be better if API would not allow to create objects in such invalid or incomplete states (what it does now as far as I understand), but not sure what it will take to change it in this direction.

I agree with @ilyapuchka here. If we use an optional attribute, the developer could wrongly access that attribute and get unexpected behaviours as a result. If we fatalErrror they would know that they are doing something wrong and it'd save them some debugging time.

Understood!👌

pepicrft · 2018-01-17T13:21:02Z

@allu22 could you have another look and tell us if you think it's ready to be merged?

pepicrft · 2018-01-17T13:21:34Z

@ilyapuchka it seems that linting is failing, that's why Travis-CI is returning an error.

pepicrft · 2018-01-18T15:18:08Z

Great work @ilyapuchka 👏

ilyapuchka · 2018-01-18T15:26:07Z

🎉

ilyapuchka mentioned this pull request Jan 9, 2018

Add new file to Xcode project #120

Closed

pepicrft reviewed Jan 10, 2018

View reviewed changes

pepicrft requested changes Jan 10, 2018

View reviewed changes

alvarhansen suggested changes Jan 10, 2018

View reviewed changes

ilyapuchka force-pushed the add-file-helpers branch 3 times, most recently from 2cdec26 to 574070e Compare January 13, 2018 13:07

ilyapuchka added 4 commits January 13, 2018 14:08

added helper methods required for adding build file to the target

12a5cd0

addressing code review comments

6e2b308

added method to calcualte full path and fixed checking for existing f…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

a7accb9

…ile references

updated CHANGELOG

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

d2ca466

ilyapuchka force-pushed the add-file-helpers branch from 574070e to d2ca466 Compare January 13, 2018 13:09

alvarhansen suggested changes Jan 13, 2018

View reviewed changes

added unit tests for helper methods, addressed code review comments

2613306

alvarhansen suggested changes Jan 13, 2018

View reviewed changes

imporved addFile method

d5bc256

alvarhansen reviewed Jan 15, 2018

View reviewed changes

toshi0383 reviewed Jan 15, 2018

View reviewed changes

pepicrft approved these changes Jan 17, 2018

View reviewed changes

fixed linting error

ce3cfda

alvarhansen approved these changes Jan 18, 2018

View reviewed changes

pepicrft merged commit 9668e27 into tuist:master Jan 18, 2018

ilyapuchka deleted the add-file-helpers branch January 18, 2018 15:25

yonaskolb mentioned this pull request Jan 22, 2018

Add ObjectReference #218

Merged

ilyapuchka mentioned this pull request Jan 30, 2018

Fully qualified Names of source files #40

Closed

Helper methods required for adding build file to the target #213

Helper methods required for adding build file to the target #213

Conversation

ilyapuchka commented Jan 9, 2018 • edited Loading

Short description 📝

Solution 📦

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

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

pepicrft 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

ilyapuchka commented Jan 13, 2018

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

ilyapuchka commented Jan 13, 2018

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

pepicrft commented Jan 17, 2018

pepicrft commented Jan 17, 2018

pepicrft commented Jan 18, 2018

ilyapuchka commented Jan 18, 2018

ilyapuchka commented Jan 9, 2018 •

edited

Loading