Add Podfile DSL for script_phase
#389
Conversation
dnkoutso
force-pushed
the
script_phase_dsl
branch
from
3feb950
to
40b7653
Compare
lib/cocoapods-core/podfile/dsl.rb
Outdated
| # | ||
| # @return [void] | ||
| # | ||
| def script_phase(options) |
There was a problem hiding this comment.
should this be modeled differently? maybe:
def script_phase(name, script, options = nil)And provide a new class perhaps?
| # | ||
| def script_phase(options) | ||
| raise Informative, 'Script phases can only be added within target definitions.' if current_target_definition.root? | ||
| raise Informative, 'Script phases cannot be added to abstract targets.' if current_target_definition.abstract? |
There was a problem hiding this comment.
hm, I think it might make sense to allow defining script phases for abstract targets, since the build env will be different for the concrete targets that inherit it
There was a problem hiding this comment.
I didn't put that much effort into it, I think it could work but for starters it might be OK?
dnkoutso
force-pushed
the
script_phase_dsl
branch
4 times, most recently
from
e80f96d
to
ca601f3
Compare
| @@ -573,6 +581,36 @@ def store_podspec(options = nil) | |||
| end | |||
| end | |||
|
|
|||
| #--------------------------------------# | |||
|
|
|||
| SCRIPT_PHASE_REQUIRED_KEYS = [:name, :script].freeze | |||
There was a problem hiding this comment.
minor update to use constants for these to make it easier to read and modify in the future.
dnkoutso
force-pushed
the
script_phase_dsl
branch
6 times, most recently
from
14de1b5
to
5ca9e9c
Compare
dnkoutso
force-pushed
the
script_phase_dsl
branch
from
5ca9e9c
to
a91b641
Compare
lib/cocoapods-core/podfile/dsl.rb
Outdated
| # @option options [Boolean] :show_env_vars_in_log | ||
| # whether this script phase should output the environment variables during execution. | ||
| # | ||
| # @option options [Symbol] :execution_order |
There was a problem hiding this comment.
@segiddins @dantoml I am curious on what do you think this API should be...
- Should we even handle this case for starters?
- If yes, how have you imagined it to work in terms of before or after compile sources, what about other phases like linking or headers? Could the API become
execute_before => 'Compile Sources'instead? - Another option could be to just let users specify and index
index => 1and we just insert that where the user asked...no questions and no special logic.
There was a problem hiding this comment.
The other way is...not to handle this at all and since this is actually adding a script phase to the users project, let the user drag the script phase and manage it manually instead.
There was a problem hiding this comment.
...and another thing is that consumers can use post_install hooks to re-arrange their script phases....
There was a problem hiding this comment.
talked with some of my team members...seems like manual drag/post_install hook will be a good way to go. Will remove it from here.
There was a problem hiding this comment.
Hmmm, I tried this today:
- Added some
script_phases - run
pod install - move phases to correct position
- run
pod installagain
The custom positions are lost. CocoaPods should either keep the positions, or provide a way of specifying the positions, I think?
There was a problem hiding this comment.
You mean the custom shell script positions are lost? If confirmed, can you please provide a sample project and file an issue with the project attached?
There was a problem hiding this comment.
Jep, that's what I mean. I'll try to create a small sample project.
There was a problem hiding this comment.
@djbe ah, I think I see what you mean. If you mean you manually readjust a script phase after you've added but then it still gets reordered after another pod install then you will need a post_install hook to arrange it. This is how it landed on master instead.
We can definitely do better but I think I now understand what you mean. Please file this in https://github.com/CocoaPods/CocoaPods so we do not comment on this merged PR further and I can mark it as an enhancement.
dnkoutso
force-pushed
the
script_phase_dsl
branch
2 times, most recently
from
4ca07e2
to
4b380ef
Compare
|
This is looking good, definitely a 1.4 feature i think tho, to avoid blocking 1.3 on bugs |
|
yeah lets wait for 1.4.0 for this. |
dnkoutso
force-pushed
the
script_phase_dsl
branch
2 times, most recently
from
a20fcce
to
165c45a
Compare
dnkoutso
force-pushed
the
script_phase_dsl
branch
3 times, most recently
from
50e632b
to
6f988d3
Compare
|
If you care this is green and still feels good to merge. The bigger splash is about allowing podspecs to include script phases which I am hoping i can introduce in a follow up PR. In the meantime this allows devs to include script phases within their podfiles for app targets. |
| def store_script_phase(options) | ||
| option_keys = options.keys | ||
| unless option_keys.all? { |key| ALL_SCRIPT_PHASE_KEYS.include?(key) } | ||
| raise StandardError, 'Unrecognized options for the shell script ' \ |
There was a problem hiding this comment.
might be nice to say which options are unrecognized, and print out the known ones?
dnkoutso
force-pushed
the
script_phase_dsl
branch
2 times, most recently
from
0683749
to
c2f444e
Compare
dnkoutso
force-pushed
the
script_phase_dsl
branch
from
c2f444e
to
2ba1e9e
Compare
|
rebased |
|
Any objections of landing this feature? It's useful but not that useful as for podspecs having the ability to declare script phases. I think we can land this and then just have it exist. |
dnkoutso
force-pushed
the
script_phase_dsl
branch
from
2ba1e9e
to
67ef132
Compare
|
rebased. Will merge on ✅ |
|
@dantoml please do let me know if you are against this. I'd be happy to revert. Its a minor useful feature that I built in 30 minutes. Its corresponding CocoaPods PR also prepares the ground for managing script phases declared within podspecs. I am planning on a follow up for podspec DSL. CocoaPods PR: CocoaPods/CocoaPods#6820 |
No description provided.