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 possiblity to add resources before the sources build phase #1351

Merged
merged 4 commits into from
Apr 25, 2023
Merged

Add possiblity to add resources before the sources build phase #1351

merged 4 commits into from
Apr 25, 2023

Conversation

mat1th
Copy link
Contributor

@mat1th mat1th commented Apr 21, 2023

Hello, this is my first contribution to this project. Thanks for your project!

Short description

XcodeGen doesn't support reordering the build phases of a target. This PR adds the possibility to move the "add resources" before the "sources build phase".

Background

As noted in #1042 it is sometimes needed to have the resources already moved before building the app to verify the resources compile time. In the same issue @yonaskolb did note that the way to go is adding a property on target. This PR does implement that.

Implementation

  • Added resourcesBeforeSourcesBuildPhase to the Target object
  • Check in PBXProjGenerator if the resourcesBeforeSourcesBuildPhase is enabled if so add the PBXResourcesBuildPhase before the PBXSourcesBuildPhase.
  • Create and update Unit tests covering this change.

}
}

if target.resourcesBeforeSourcesBuildPhase {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will add the PBXResourcesBuildPhase before the PBXSourcesBuildPhase if the resourcesBeforeSourcesBuildPhase property is true. I've moved the logic for adding the PBXResourcesBuildPhase to a function to not duplicate it.

@@ -1652,8 +1652,8 @@
isa = PBXNativeTarget;
buildConfigurationList = 77CE5B5E5DEAC820254D484C /* Build configuration list for PBXNativeTarget "App_macOS" */;
buildPhases = (
96BB43F4706B031DA45166E8 /* Sources */,
Copy link
Contributor Author

@mat1th mat1th Apr 21, 2023

Choose a reason for hiding this comment

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

This change is caused by the added property resourcesBeforeSourcesBuildPhase in Tests/Fixtures/TestProject/project.yml


let targets = pbxProj.projects.first?.targets
try expect(targets?.count) == 2
try expect(targets?.first?.buildPhases.first).to.beOfType(PBXResourcesBuildPhase.self)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This validates that the PBXResourcesBuildPhase is added in front when the property is set.

Copy link
Owner

@yonaskolb yonaskolb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @mat1th, you've covered everything well! Just left a couple of small comments. Could you please also add:

  • an update to Docs/ProjectSpec.md
  • add an entry to CHANGELOG.md

resourcesBeforeSourcesBuildPhase: false
)

let swinjectPackage = SwiftPackage.remote(url: "https://github.com/Swinject/Swinject", versionRequirement: .exact("2.8.0"))
Copy link
Owner

Choose a reason for hiding this comment

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

The swift package isn't part of the test, so we can remove it

@@ -1083,6 +1083,18 @@ public class PBXProjGenerator {
}
}

func addresourcesBuildPhase() {
Copy link
Owner

Choose a reason for hiding this comment

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

Just an uppercase "R": addResourcesBuildPhase

@@ -56,6 +56,7 @@ public struct Target: ProjectTarget {
public var attributes: [String: Any]
public var productName: String
public var onlyCopyFilesOnInstall: Bool
public var resourcesBeforeSourcesBuildPhase: Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the name of this property clear enough? Maybe add put for clarity?

Suggested change
public var resourcesBeforeSourcesBuildPhase: Bool
public var putResourcesBeforeSourcesBuildPhase: 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.

Will change it to putResourcesBeforeSourcesBuildPhase.

@teameh
Copy link
Contributor

teameh commented Apr 24, 2023

Nice work! Don't forget to:

  • Add the property to the docs
  • Add an entry to the changelog

@mat1th
Copy link
Contributor Author

mat1th commented Apr 24, 2023

Thanks for the feedback.

I've resolved all issues. Could you recheck the pull request?

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Owner

@yonaskolb yonaskolb left a comment

Choose a reason for hiding this comment

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

Thanks @mat1th

@yonaskolb yonaskolb merged commit 8256008 into yonaskolb:master Apr 25, 2023
@teameh
Copy link
Contributor

teameh commented Apr 25, 2023

\0/ Nice!

@mat1th
Copy link
Contributor Author

mat1th commented Apr 26, 2023

Thank you @yonaskolb for merging!

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