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.

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

Add Support for Custom Build Rules #197

Merged
merged 1 commit into from Dec 26, 2017
Merged

Conversation

briantkelley
Copy link
Contributor

@briantkelley briantkelley commented Dec 19, 2017

Short description πŸ“

Add support for custom build rules, which Xcode uses to automatically invoke a tool when a matching input file changes.

Solution πŸ“¦

Add a PBXObject subclass, PBXBuildRule, which can load and store the custom build rule configuration.

Implementation πŸ‘©β€πŸ’»πŸ‘¨β€πŸ’»

  • Create PBXBuildRule, which is a subclass of PBXObject and add properties for all possible fields in the Xcode project file. Implement Equatable, Decodable, and encoding support.
  • Update PBXObject and PBXProj to support decoding
  • Update PBXProj and PBXProjEncoder to support encoding
  • Add and update unit tests
  • Manually verify xcproj correctly round trips a project with custom build rules (custom pattern/custom tool, custom pattern/built-in tool, built-in pattern/custom tool, built-in pattern/built-in tool)

This change is Reviewable

@ghost
Copy link

ghost commented Dec 20, 2017

1 Error
🚫 Source files have been added or removed. Execute bundle exec rake generate_carthage_project to regenerate the Carthage.xcodeproj

Generated by 🚫 Danger

public var fileType: String

/// Element is editable.
public var isEditable: UInt
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this take any other values than 1 or 0? Otherwise we can make it a Bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it's only ever a 1 or 0. There were some existing seemingly Bool properties were using (U)Int but definitely happy to change it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, those other ones could be changed in 2.0 as it's a breaking change

Copy link
Collaborator

Choose a reason for hiding this comment

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

public var outputFilesCompilerFlags: [String]?

/// Element script.
public var script: String
Copy link
Collaborator

Choose a reason for hiding this comment

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

Script is also optional right? Is there an advantage to making it non optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. This should definitely be optional.

@briantkelley
Copy link
Contributor Author

Thanks for the feedback @yonaskolb. Updated the PR.

Some Xcode projects have custom build rules to automatically invoke a tool when an input file changes. Add a PBXObject subclass, PBXBuildRule, which describes when and how a custom build tool should be invoked. Update PBXProj to store custom build rules and add decoding/encoding support.

In addition to updating the tests, verified xcproj correctly round trips a project with custom build rules (custom pattern/custom tool, custom pattern/built-in tool, built-in pattern/custom tool, built-in pattern/built-in tool).
public init(reference: String,
compilerSpec: String,
filePatterns: String? = nil,
fileType: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

One more very minor thing. As fileType is non optional (I don't know if it should be or not), can we move it before optional filePatterns?

public var compilerSpec: String

/// Element file patterns.
public var filePatterns: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity. Did you check the optionality of the attributes with Xcode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for fixing this up and merging. @pepibumur, yes, I tried examining the encoded form of of a number of different permutations. compilerSpec is required so specify how the file should be processed, but filePatterns is optional since it's a filter on fileType.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem @briantkelley πŸ˜› it was just curiosity. The changes should be in xcproj 1.8.0 ready to be used :)

@pepicrft pepicrft merged commit aab035e into pbxrezbuildphase Dec 26, 2017
@pepicrft pepicrft deleted the pbxbuildrule branch December 26, 2017 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants