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 scheme pre-actions and post-actions #231

Merged
merged 12 commits into from
Jan 30, 2018
Merged

Conversation

kastiglione
Copy link
Collaborator

@kastiglione kastiglione commented Jan 26, 2018

Add support for scheme pre-actions and post-actions

This pull request adds preActions and postActions to each of the scheme action types. For example:

schemes:
  Foo:
    build:
      preActions:
        - name: Prepare Build
          script: echo Preparing for Build

Check List

  • Add tests
  • Add some actions to the scheme spec loading test
  • Add a custom scheme with actions to the test fixture
  • Add support for inheriting build settings (see environmentBuildable)
  • Update ProjectSpec.md
  • Check whether script needs to be escaped

This change requires tuist/XcodeProj#217.

@kastiglione
Copy link
Collaborator Author

@yonaskolb To build XcodeGen with the latest HEAD of xcproj, I had to make the changes in 1fe2e96. Feel free to cherrypick it into #227.

@yonaskolb
Copy link
Owner

@kastiglione, #227 is merged

@yonaskolb
Copy link
Owner

Do you want to add pre and post actions to TargetScheme as well? It would just apply them to all the different actions in the Scheme init here https://github.com/yonaskolb/XcodeGen/blob/master/Sources/XcodeGenKit/ProjectGenerator.swift#L178

@kastiglione
Copy link
Collaborator Author

I don't need that functionality for my work. Do you think it will be common enough for people to have a script they want executed before/after every build, run, test, profile, and archive? I don't know the answer to this, but I can check, are these scripts called with arguments or environment variables that indicate which action (build/run/test/etc) is being invoked? If not, then I would say not to add it.

@kastiglione
Copy link
Collaborator Author

I just checked, the pre/post action scripts don't have any arguments or environment variables that tell which type of action (build/run/test/etc) triggered the script.

@kastiglione kastiglione changed the base branch from xcproj_4.0 to master January 28, 2018 22:14
@kastiglione
Copy link
Collaborator Author

Thanks for the review and approval. I plan to do the changes mentioned in the todo checklist. That can be here or a follow up.

@yonaskolb
Copy link
Owner

They can be done here 👍

@kastiglione
Copy link
Collaborator Author

Ready to merge, pending review of updates.

@yonaskolb
Copy link
Owner

Looks great @kastiglione, thanks!
If you have time to add some actions to the scheme spec loading test, and add a custom scheme with actions to the test fixture that would be great

@kastiglione
Copy link
Collaborator Author

Tests added.

@yonaskolb
Copy link
Owner

yonaskolb commented Jan 30, 2018

Nice! Just the updated test project needs to be committed after running the tests. (The tests won't fail automatically with scheme changes at the moment due to #202)

@@ -59,5 +59,15 @@ func fixtureTests() {
try expect(variableGroup.children.filter { $0 == refs.first?.reference }.count) == 1
}
}

$0.it("generates scheme execution actions") {
Copy link
Owner

@yonaskolb yonaskolb Jan 30, 2018

Choose a reason for hiding this comment

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

As a note for the future, tests here aren't strictly required as the tests will fail if the generated project changes. Someone just added some more explicit checks afterwards above once. No harm in having this here though 👍

@yonaskolb
Copy link
Owner

I'll do it in master 👍

@yonaskolb yonaskolb merged commit 0f87936 into master Jan 30, 2018
@yonaskolb yonaskolb deleted the scheme-actions branch January 30, 2018 01:34
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

2 participants