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.

Sign up for GitHub

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

Jump to bottom

Add Support for Build Carbon Resources Build Phase #196

Merged
merged 1 commit into from
Dec 23, 2017
Merged

Add Support for Build Carbon Resources Build Phase #196

merged 1 commit into from
Dec 23, 2017

Conversation

briantkelley
Copy link
Contributor

@briantkelley briantkelley commented Dec 19, 2017 •
edited by pepicrft
Loading

Short description 📝

Some macOS apps still build legacy Carbon resources, which xcproj didn't support.

Solution 📦

The Xcode project file models this in the same way as the abstract PBXBuildPhase, so implementation of this build phase is as easy as adding and wiring up a trivial subclass.

Implementation 👩‍💻👨‍💻

  • Add to BuildPhase with the comment used in the pbxproj file
  • Subclass PBXBuildPhase with the isa name used in the pbxproj file
  • Add decoding support to PBXObject
  • Add a Rez build phase property PBXProj.Objects, add decoding support, update the buildPhases computed property, and update the equality operator.
  • Add encoding support by updating PBXProjEncoder and PBXProjObjects+Helpers.swift
  • Add and update unit tests
  • Verify a roundtrip of an .xcodeproj file through xcproj yields the PBXRezBuildPhase in the same location in the project file with the same information.

This change is Reviewable

briantkelley
briantkelley commented Dec 19, 2017
View reviewed changes
Sources/xcproj/PBXProjEncoder.swift
@@ -40,6 +40,7 @@ final class PBXProjEncoder {
write(section: "PBXLegacyTarget", proj: proj, object: proj.objects.legacyTargets)
write(section: "PBXProject", proj: proj, object: proj.objects.projects)
write(section: "PBXResourcesBuildPhase", proj: proj, object: proj.objects.resourcesBuildPhases)
write(section: "PBXRezBuildPhase", proj: proj, object: proj.objects.carbonResourcesBuildPhases)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verified Xcode writes the PBXRezBuildPhase section between PBXResourcesBuildPhase and PBXShellScriptBuildPhase.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@briantkelley briantkelley force-pushed the pbxrezbuildphase branch 2 times, most recently from b6a9fcd to fb7c56b Compare December 19, 2017 22:58
@ghost
Copy link

ghost commented Dec 20, 2017 •
edited by ghost
Loading

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

Generated by 🚫 Danger

pepicrft
pepicrft reviewed Dec 20, 2017
View reviewed changes
Sources/xcproj/PBXRezBuildPhase.swift Outdated
/// This is the element for the Build Carbon Resources build phase.
final public class PBXRezBuildPhase: PBXBuildPhase {

public override var buildPhase: BuildPhase {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's a problem with how GitHub is representing the code here but it looks like the indentation of this property is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Had some tabs in there. Fixed. Nice catch!

pepicrft
pepicrft approved these changes Dec 20, 2017
View reviewed changes
Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Just a small comment. I think this is the first time I hear about Build Carbon Resources.

@pepicrft pepicrft added this to the 1.7.1 milestone Dec 20, 2017
@briantkelley
Add Support for Build Carbon Resources Build Phase
6167d59 
Some macOS apps still build legacy Carbon resources. Fortunately the Xcode project file models this in the same way as the abstract PBXBuildPhase so implementation of the build phase is as easy as adding a subclass, updating the BuildPhase enum, updating PBXProj, and adding decoding/encoding support.

In addition to updating the tests, verified xcproj correctly round trips a project with this build phase.
@briantkelley
Copy link
Contributor Author

briantkelley commented Dec 20, 2017 •
edited by yonaskolb
Loading

@pepibumur I'm not able to figure out the Danger error for the PR. The Travis build log didn't seem to contain any error information. I tried running danger locally (e.g. bundle exec danger local --base=master --head=6167d59 --dangerfile=Dangerfile or without any of the arguments) and it showed me two infos for #190. It then displayed the errors:

  • One of the lines below found in CHANGELOG.md doesn't match the expected format. Please make it look like the other lines, pay attention to periods and spaces.
  • Please put back the * Your contribution here. line into CHANGELOG.md.

4 times. I didn't remove any text and there's not enough context to identify the first, though as far as I can tell the changelog follows existing conventions.

Any pointers on how to debug and fix this? Thanks!

@yonaskolb
Copy link
Collaborator

@briantkelley I'm not sure about your local errors, but the danger bot posted in this PR with the changes that need to happen. Have you run bundle exec rake generate_carthage_project like the bot mentions? That's needed so the Carthage project has all the new files

@yonaskolb
Copy link
Collaborator

Sorry @briantkelley, I see you've already committed the Carthage changes

@briantkelley
Copy link
Contributor Author

Unsurprisingly #197 is failing with presumably the same error. The build and tests passed and I did fix the original Danger error. Do you think the Danger PR error is ignorable and this is safe to merge?

@yonaskolb
Copy link
Collaborator

Merging this even though the tests failed, as pretty sure it's because of a messed up CI config

@yonaskolb yonaskolb merged commit c9d4d23 into master Dec 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pepicrft pepicrft pepicrft approved these changes

Assignees

@briantkelley briantkelley

@pepicrft pepicrft

Labels
None yet
Projects
None yet
Milestone
1.7.1
Development

Successfully merging this pull request may close these issues.
3 participants
@briantkelley @yonaskolb @pepicrft