Support for defining pods per build config #1668

Closed
wants to merge 8 commits into
from

4 participants

@nevyn

This is no longer WIP, and seems to work for my purposes. It changes the way pods are linked so I'm sure it breaks something…

Related issue: #731

My random notes:

Here's how I'm thinking it could be built:
1. Figure out which configurations we should provide xcconfig files for. Either:
  a) Enumerate configs in target. Tricky: finding target, finding configs,
     mapping them to configs in Podfile. Or:
  b) Use the set of configurations mentioned in the Podfile. Easy and
     straightforward. Have a "default" xcconfig for configurations not 
     explicitly mentioned in the Podfile.
  (it seems that (a) is the right solution, since all_user_build_configurations exists)

2. Make each "pod instance" (called 'target'?) reply whether they should be
   included in the given configuration. (this includes updating the Podfile parser, wherever that is)

3. Make aggregate_pod_xcconfig.rb query the property from #2. And the installer.

4. Generate multiple public xcconfig files based on #1.

Interesting pieces of code:
* Pod::AggregateTarget#xcconfig -> xcconfig_for_configuration
* Pod::Installer::UserProjectIntegrator::TargetIntegrator#add_xcconfig_base_configuration
  calls xcconfig_for_configuration
* Pod::Installer::AggregateTargetInstaller#create_xcconfig_file creates
  multiple config files.
* Pod::Installer::Analyzer::AnalysisResult#all_user_build_configurations looks useful

Random thoughts:
Hmm. For (1a), one way to forward those build configs is to make Pods.xcodeproj
have the same configs as the hosting project (only works when integrating) ----
woot, it does that already! :D

Notes from alloy:

So I imagine a Podfile DSL like this:
pod 'Reveal', :configurations => ['AdHoc', 'Debug']
And possibly:
pod 'Reveal', :configuration => 'Debug'

So for that screenshot, this is what the xcconfigs would look like:
# Pods.AdHoc.xcconfig

    OTHER_LDFLAGS = lxml2 -lz -framework HockeySDK

# Pods.Debug.xcconfig

    OTHER_LDFLAGS = lxml2 -lz -lSocketRocket -lPonyDebugger -licucore

# Pods.Release.xcconfig

    OTHER_LDFLAGS = lxml2 -lz
nevyn added some commits Dec 10, 2013
@nevyn nevyn WIP: Support for defining pods per build config
This is not even nearly functional, I'm just prodding pieces of code to
see how it works.
f89dd4d
@nevyn nevyn [integrator] Add build-specific xcconfig to builds
Also fixes some typos from previous commit. Different xcconfigs are now
actually generated, correctly assigned, and it all builds! Doesn't pass
tests and all xcconfigs are still identical, but still: getting there!
d802e9b
@alloy
CocoaPods member

Regarding question 1, we already indeed retrieve the configurations from the user’s project, because we need to have those exist in Pods.xcodeproj as well.

@alloy
CocoaPods member

Please always use parens in method signatures.

@alloy

Please use parens here as well. Basically, I only ever don't use them if it forms the complete line and has more of a DSL-feel than normal method dispatching.

@fabiopelosin
CocoaPods member

🚀 So happy to see somebody working on this.

I'm still of the opinion that the syntax that I have described in #731 (comment) would be the best solution. I also think that the original proposal described in #731 and further explored in #731 (comment) would be the best solution:

So here is how I would implement the feature:

  • Add the DSL to the Podfile
  • CocoaPods already enumerates the configurations of the user target in the analyzer. Somewhere after this step it should store in the target representation of each Pod in which build configurations it should be linked and the default to all if no setting was specified by the user.
  • When the aggregate xcconfig is creates the Pod targets should be inspected – they are already available in the aggregate target – to generate the linking.
  • The aggregate target (i.e. Pods) should not include the Pods targets (i.e. Pods-AFNetworking) via the Link Binary With Libraries build phase anymore, instead it should use the xcconfig file (for that purpose it would be better to generate a private one).

I think that the Xcconfig should be something along the following untested xcconfig:

SHARED_OTHER_LDFLAGS = -ObjC -framework CoreServices
OTHER_LDFLAGS[config=Debug] = ${SHARED_OTHER_LDFLAGS} -framework Pods-AFNetworking
OTHER_LDFLAGS[config=Release] = ${SHARED_OTHER_LDFLAGS}
OTHER_LDFLAGS[config=AdHoc] = ${SHARED_OTHER_LDFLAGS}
@alloy
CocoaPods member

@irrationalfab That namespacing trick does not work (it only works with arch and sdk), which is why I told @nevyn to generate separate xconfig files for each configuration. Not pretty to have more files, but it works and should be clear to the user.

Regarding the DSL, I disagree that the patch should immediately follow that. I think that in the end it makes sense to add such a config block, but that would internally basically do the same thing, which is why told @nevyn to hold back on that for now and just implement the basics. I propose we look at this once this patch is done and working, but at least once we cleanup the Podfile DSL (and remove the implicit target etc).

@fabiopelosin
CocoaPods member

@irrationalfab That namespacing trick does not work (it only works with arch and sdk), which is why I told @nevyn to generate separate xconfig files for each configuration. Not pretty to have more files, but it works and should be clear to the user.

👍

Regarding the DSL, I disagree that the patch should immediately follow that. I think that in the end it makes sense to add such a config block, but that would internally basically do the same thing, which is why told @nevyn to hold back on that for now and just implement the basics. I propose we look at this once this patch is done and working, but at least once we cleanup the Podfile DSL (and remove the implicit target etc).

Taking into account the discussion about the clean up I still think that this would be the most appropriate DSL. So I don't see any specific reason to not to implement like that immediately. Otherwise this feature would become dependent on #840 which may take some time.

@fabiopelosin
CocoaPods member

The aggregate target (i.e. Pods) should not include the Pods targets (i.e. Pods-AFNetworking) via the Link Binary With Libraries build phase anymore, instead it should use the xcconfig file (for that purpose it would be better to generate a private one).

Thinking about it, the purpose of the aggregate targets is to link against the Pods targets in a clean way (for the user project). However if we perform the linking via the Pods xcconfig we could remove them to keep things simpler. We would still have to generate the support files & an xcconfig for each build configuration (i.e. Pods-Debug.xcconfig).

@alloy
CocoaPods member

Taking into account the discussion about the clean up I still think that this would be the most appropriate DSL. So I don't see any specific reason to not to implement like that immediately. Otherwise this feature would become dependent on #840 which may take some time.

I don’t have a problem with introducing this API immediately after this patch landing, but do we agree that both should be supported, just like Bundler does? (Also because internally the configuration(list) { ... } method would have to use an attribute like that anyways.)

@fabiopelosin
CocoaPods member

do we agree that both should be supported, just like Bundler does? (Also because internally the configuration(list) { ... } method would have to use an attribute like that anyways.)

My concern is about not overloading the options of the pod. There already many options however they all relate to the specification of the source/podspec (except the warnings inhibition) of the Pod. For this reason I find the nested approach more appropriate semantically because the tree structure of the Podfile reflects where the dependencies are used – I see build configurations as a more granular specification of a target. There is the problem of multiple configurations but not all depend ending on the same Pod; however with the inheritance proposals discussed in #840 look like a reasonable solution.

@nevyn nevyn added a commit to lookback/Core that referenced this pull request Dec 12, 2013
@nevyn nevyn [pod] Whitelist pods for configurations
An option to only link a specific pod to the target when building in a specific configuration.
The exact syntax is still TBD, pending discussion in CocoaPods/CocoaPods#1668
which implements the corresponding functionality.
affbeae
nevyn added some commits Dec 12, 2013
@nevyn nevyn [pod_target] Ask the targdef about config inclusion
lookback/Core@affbeae
defines a new ":configurations" option for a pod, and exposes the corresponding
API. Using that API in include_in_build_config now lets aggregate_xcconfig
only include the build flags for the libraries that are used.

Next step is to make CocoaPods link using build flags, so that linking
can be disabled with an xcconfig.
bffec96
@nevyn nevyn Fix code style issues reported by @alloy 6b0f4aa
@nevyn nevyn [installer] link using LDFLAGS instead of product reference
Since we want to do conditional linking, the list of Pods to link with 
needs to be provided by an xcconfig file. Only way to do that is to
remove the linking responsibility from Xcode and give it to ld directly.
b358bc2
@nevyn nevyn config whitelisting: fix specs I broke
Fixes all the tests that broke with API changes of cocoapods internals
related to build configuration whitelisting

Should've fixed these inline when I made the changes, but I didn't :(
4126283
@nevyn

Fixing the integration steps by hand seems to be a lot of work, so I'll wait until everyone seems happy to merge this before fixing them.

And at that, I think I'm done, pending discussion in and merge of CocoaPods/Core#52 .

@fabiopelosin
CocoaPods member

Fixing the integration steps by hand seems to be a lot of work

Have you seen the following rake task:

rake spec:rebuild_integration_fixtures

The idea is to rebuild them automatically and the check the diff.

nevyn added some commits Dec 18, 2013
@nevyn nevyn config whitelisting: fix some more specs
* missed fixing three calls to xcconfig_relative_path
* Added parens to a call while at it
86ad200
@nevyn nevyn config whitelisting: bump integration specs fc10a2e
@nevyn

Thanks fab, didn't know about that script.

Now the example isn't building, looking into it... Lookback was a bad pod to test with since it has no sources; now testing with AFNetworking and finding more bugs. Stand by...

@nevyn

Ok, the bug is that Xcodeproj does not respect merging quoted build options, so this line fails when the project has a space in its name:

      @xcconfig.merge!({
        'OTHER_LDFLAGS' => "-l#{pod_target.label}"
      })
@nevyn nevyn referenced this pull request in CocoaPods/cocoapods-integration-specs Dec 19, 2013
Closed

Update for build config whitelisting #5

@nevyn

aand the reason for the Xcodeproj failure is line 155 in config.rb#merge!:

libraries  = flags.scan(/(?:\A|\s)-l ?([^\s]+)/).map { |m| m[0] }

...which does not handle neither quoted strings nor escaped spaces.

If you're fine with that working right now (alloy is working on a real parser), then feel free to merge away.

@alloy alloy was assigned Dec 19, 2013
@alloy
CocoaPods member

After a discussion @irrationalfab and I had, we decided to remove ‘aggregate targets’ altogether, but also focus on getting v0.29 out of the door first. Once that’s out I will work on integrating this.

@nevyn

Saw the discussion. Sounds like a good plan. If you wish to keep the explicit dependency list of the user target small, you could retain the aggregate target, but remove all its build steps and use it as a pure aggregate target that only has dependencies. Of course, easier to just remove it altogether…

Again, thanks for the assistance.

@tonyxiao

@alloy Is there an ETA for when this PR would make it into a Cocoapods release? If there's anything the community can help to speed the process up please let me know!

@tonyxiao

and @nevyn ?

@nevyn

@tonyxiao: @alloy wanted to make a large architectural change to this patch before submitting it (basically killing the aggregate target abstraction), and have it planned for a specific version. I was getting really frustrated with working on the patch so @alloy very kindly offered to finish the work. It sounded like a late-Dec-early-Jan kind of plan. Haven't heard anything since before Christmas, but that's understandable. If I can help out in a pair coding session I'll gladly do so, but it's too scary a change to make on my own.

@alloy alloy added a commit to CocoaPods/Core that referenced this pull request Feb 2, 2014
@nevyn nevyn [pod] Whitelist pods for configurations
An option to only link a specific pod to the target when building in a specific configuration.
The exact syntax is still TBD, pending discussion in CocoaPods/CocoaPods#1668
which implements the corresponding functionality.
cff8c54
@alloy
CocoaPods member

This PR is superseded by #1791.

@alloy alloy closed this Feb 5, 2014
@Ashton-W Ashton-W pushed a commit to Ashton-W/Core that referenced this pull request Nov 2, 2015
@nevyn nevyn [pod] Whitelist pods for configurations
An option to only link a specific pod to the target when building in a specific configuration.
The exact syntax is still TBD, pending discussion in CocoaPods/CocoaPods#1668
which implements the corresponding functionality.
afd7a79
@Ashton-W Ashton-W pushed a commit to Ashton-W/Core that referenced this pull request Nov 2, 2015
@nevyn nevyn [pod] Whitelist pods for configurations
An option to only link a specific pod to the target when building in a specific configuration.
The exact syntax is still TBD, pending discussion in CocoaPods/CocoaPods#1668
which implements the corresponding functionality.
4cc49f0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment