Skip to content

Add support for pod whitelisting based on build configuration. #1791

Merged
merged 46 commits into from Aug 12, 2014
@alloy
CocoaPods member
alloy commented Feb 5, 2014

This PR supersedes #1668. This is a rebased version and adds some more integration work. The Core counterpart can be found here.

  • Coerce configuration name objects to String, meaning Symbol will work too.
  • Accept configuration name with singular :configuration option as well.
  • Validate the configuration name that the user specifies for the whitelist in their Podfile.
  • Set the xcconfigs in already integrated user projects (by @irrationalfab).
  • Fix the environment header to properly reflect which pods are available to the user’s project. We can either generate env headers for each configuration or inject the configuration into the CPP definitions and use that to conditionally enable pods in one env header.
  • Fix the examples
  • Present a warning if all the specs of a Pod are not in the same white list as it will not work.
  • Always check that the xcconfig are $(inherited) in the user project (instead only during the first integration).
Future Step
  • Investigate if a target per spec/subspec is feasible
  • Remove the environment header
  • The acknowledgements.plist files include all the Pods regardless of the build configuration
  • All the resources are copied regardless of the build configurations
  • Make a shared xcconfig to facilitate import in users xcconfigs
  • Decide about the case of the xcconfigs
Removed
  • Set target dependencies (by @irrationalfab).
  • Only generate one xcconfig when the user has not specified any configurations to whitelist pods for. (removed by @irrationalfab)
nevyn and others 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.
c3d2356
@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!
2087db1
@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.
d59e40f
@nevyn nevyn Fix code style issues reported by @alloy 1fdce60
@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.
1e749bb
@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 :(
ea48cd4
@nevyn nevyn config whitelisting: fix some more specs
* missed fixing three calls to xcconfig_relative_path
* Added parens to a call while at it
00790a9
@alloy alloy [Bundle] Update. abee9ea
@alloy alloy Small syntax cleanups. 35b9250
@alloy alloy [Core] Update syntax change. de4fbe8
@alloy alloy [Analyzer] Get all configurations from the user project, including De…
…bug and Release.
028921a
@alloy alloy [Installer] Validate the configs a pos is whitelisted for. 7643275
@alloy alloy [xcconfig] Only quote the path to the -isystem option. d8c67f2
@alloy alloy [CHANGELOG] Document a few adhoc fixes. 5d1e01b
@alloy
CocoaPods member
alloy commented on d8c67f2 Feb 4, 2014

@swizzlr Here’s that patch.

@alloy
CocoaPods member
alloy commented Feb 5, 2014

Ping @nevyn @irrationalfab.

@alloy
CocoaPods member
alloy commented Feb 5, 2014

Regarding how we actually make this work, these are my notes after trying many things with Xcode:

  • We need to use Xcode’s normal build system (‘Link With Library’ build phase), because that’s the only way how we can trigger a build in another project (e.g. Pods.xcodeproj) than the current one (e.g. App.xcodeproj).

  • Xcode’s normal build system (‘Link With Library’ build phase) has no way to specify different libraries to link with based on the build configuration.

  • So we need:

    • An ‘aggregate’ target that the App.xcodeproj can depend on to build all the dependencies, regardless of configuration. At the moment, the only viable solution seems to be to abuse a ‘static lib’ target for this purpose, as it can be added to the ‘app’ target’s ‘Link With Library’ build phase and have its own ‘target dependencies’. This will ensure that all the pods that could be in a target, regardless of build configuration, are build. (Unfortunately it will still build the individual pod targets that are not used in a config, but at least they won’t be linked in.)
    • Then use the xcconfig-per-build-configuration to decide which individual pod libs to actually link into the app for the current configuration.
@alloy alloy added a commit to CocoaPods/cocoapods-integration-specs that referenced this pull request Feb 5, 2014
@alloy alloy Update for CocoaPods/CocoaPods#1791. 0590261
@alloy
CocoaPods member
alloy commented Feb 5, 2014

PS: These branches have been rebased onto the current master versions.

@alloy
CocoaPods member
alloy commented Feb 5, 2014

Travis is currently failing due to an encoding issue when trying to build the AFNetworking example apps. The normal tests all pass.

@nevyn
nevyn commented Feb 5, 2014

Hmm, what you're describing sounds just like what I thought I built. The aggregate target depends on all the pod targets, and thus builds all of them, but links none. The user target then links only the things it want (using LDFLAGS), triggering a build through having a dependency on the aggregate target. Is this not accomplishing what you're describing?

Obviously not since you wrote that comment, I'm just struggling to understand. You could also ignore this message for a few hours and maybe I'll understand it after thinking for a while.

@alloy
CocoaPods member
alloy commented Feb 5, 2014

No you are right, this is exactly what you built :)

The notes are just about that I have tried all the ways that we’d hope it could also work, but ended up not being possible and why.

The actual changes are simply some cleanups and refinements. The remaining TODOs are the unchecked ones in the original post.

@nevyn
nevyn commented Feb 5, 2014

Oh! Silly. I read, "this is how we may make this work". Now it makes much more sense. You're saying that the aggregate target can't be killed, because it is needed to actually perform the dependent builds. Gotcha. Wow, that is incredibly obvious in retrospect. Thanks for the insight! I feel so silly now.

I can't promise to take on the remaining two tasks, at least not until I've completed my current task at work. After that, definitely. Again, thank you so much for your help with this patch. Our product absolutely depends on it.

@swizzlr
swizzlr commented Feb 20, 2014

Let's make this happen guys. What can I do?

@fabiopelosin fabiopelosin self-assigned this Mar 29, 2014
@tonyxiao

was hoping cocoapods 0.30.0 would have this. Any chance this will make into 0.31.0?

@fabiopelosin
CocoaPods member

This is one of my top priorities but being a pretty big change I had to clean the slate first.

@nevyn
nevyn commented Apr 7, 2014

@irrationalfab, @alloy: I'll start work on this tomorrow if you haven't gotten to it yet

@fabiopelosin
CocoaPods member

@nevyn I'm focusing on the DSL refactor of the Podfile which will enable this.

@nevyn
nevyn commented Apr 7, 2014

@irrationalfab So I should stay away? You'll fix it? Or can I help somehow?

@fabiopelosin
CocoaPods member

@irrationalfab So I should stay away? You'll fix it? Or can I help somehow?

Are you crazy? 😉 I will never ask a CP contributor to stay away from code 😄 I plan to work the refactor this evening and tomorrow (CocoaPods/Core#81). So if you want to contribute I can see the following areas:

  • Merge this pull request with master, check its status and implement the missing points (I can provide support).
  • Help consolidate the final design of the DSL and share any comment in CocoaPods/Core#81
  • Contribute to CocoaPods/Core#81 directly making pull requests to that branch (I will push commits quickly to prevent stepping over each other)
  • Help with the testing of the features.
@nevyn
nevyn commented Apr 9, 2014

Ahh, the DSL refactor, now I remember... I still really think "pod 'x', :configuration=>'Release'" would be a much easier syntax for most people.

I'll start out with a master rebase/merge, then "Fix the environment header to properly reflect which pods are available to the user’s project"; that's needed in any case.

@fabiopelosin
CocoaPods member

@alloy and I finally discussed the refactor in person and concluded that the canonical way should be nesting. However that the inline syntax is convenient and it should be supported, but only in a second step.

@nevyn
nevyn commented Apr 9, 2014

Ok, sure. Thanks for the update.

@mbrgm
mbrgm commented May 12, 2014

+1 on this one. Any progress?

@shouze
shouze commented May 21, 2014

👍

@kylef
kylef commented Jul 23, 2014

However that the inline syntax is convenient and it should be supported, but only in a second step.

@alloy @irrationalfab If the inline syntax will be supported, how about we just go with that in the initial release of this feature and then add the nesting as apart of #840? This will unblock this feature and should prevent this from being held up anymore.

@rayray
rayray commented Jul 24, 2014

Hate to just 👍 this, but I am eagerly awaiting this feature, so I don't have to do it manually before submitting a new app to Veracode. It's a very annoying manual process!

@orta orta locked and limited conversation to collaborators Jul 24, 2014
@fabiopelosin fabiopelosin Merge branch 'master' into lookback-pods-by-config
* master: (294 commits)
  Bundle: update cocoapods-core
  Bundle: update Xcodeproj
  Validator: check license only in root spec
  Bundle: udpate cocoapods-core
  Integration: update for Xcodeproj
  Specs: fix for changes to Xcodeproj
  Update integration specs for Xcodeproj
  Gem: update Xcodeproj dependency
  Integration: update acknowledgements
  Bundle: make xcodeproj faster on mac
  Update integration specs for Xcodeproj
  Bundle:update
  CI: disable Ruby 1.8.7 for now
  Xcodeproj: udpate
  Bundle: update cocoapods-trunk version
  [Sandbox] Evaluate podspecs from their original path
  [Install/Update] Clarify command description. Relates to #2270.
  [README] Flatten badges
  [Gemspec] Update to nap 0.8
  Pod::Command::Spec::Lint::description update
  ...

Conflicts:
	CHANGELOG.md
	Gemfile.lock
	spec/cocoapods-integration-specs
064085e
@fabiopelosin
CocoaPods member

Only generate one xcconfig when the user has not specified any configurations to whitelist pods for.

Is worth having a different logic for this case?

@fabiopelosin
CocoaPods member

This patch is indeed producing changes to the integrations tests that need further investigation: https://github.com/CocoaPods/cocoapods-integration-specs/pull/12/files (as pointed out by @nevyn in his original pull request)

@fabiopelosin
CocoaPods member

This patch is not setting the xcconfig at least in already integrated projects. Other than that appears to be working correctly on Xcode 5 and on Xcode 6 Beta 4.

@houndci-bot houndci-bot commented on an outdated diff Jul 26, 2014
...taller/target_installer/aggregate_target_installer.rb
@@ -32,15 +32,14 @@ def install!
# @return [void]
#
def create_xcconfig_file
- path = library.xcconfig_path
- UI.message "- Generating xcconfig file at #{UI.path(path)}" do
- gen = Generator::XCConfig::AggregateXCConfig.new(library)
- gen.save_as(path)
- library.xcconfig = gen.xcconfig
- xcconfig_file_ref = add_file_to_support_group(path)
-
- target.build_configurations.each do |c|
- c.base_configuration_reference = xcconfig_file_ref
+ target.build_configurations.each do |configuration|
+ path = library.xcconfig_path(configuration)
+ UI.message "- Generating #{configuration.name} xcconfig file at #{UI.path(path)}" do
@houndci-bot
houndci-bot added a note Jul 26, 2014

Line is too long. [94/80]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@houndci-bot houndci-bot commented on an outdated diff Jul 26, 2014
...taller/target_installer/aggregate_target_installer.rb
@@ -32,15 +32,14 @@ def install!
# @return [void]
#
def create_xcconfig_file
- path = library.xcconfig_path
- UI.message "- Generating xcconfig file at #{UI.path(path)}" do
- gen = Generator::XCConfig::AggregateXCConfig.new(library)
- gen.save_as(path)
- library.xcconfig = gen.xcconfig
- xcconfig_file_ref = add_file_to_support_group(path)
-
- target.build_configurations.each do |c|
- c.base_configuration_reference = xcconfig_file_ref
+ target.build_configurations.each do |configuration|
+ path = library.xcconfig_path(configuration)
+ UI.message "- Generating #{configuration.name} xcconfig file at #{UI.path(path)}" do
+ gen = Generator::XCConfig::AggregateXCConfig.new(library, configuration.name)
@houndci-bot
houndci-bot added a note Jul 26, 2014

Line is too long. [89/80]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@houndci-bot houndci-bot commented on an outdated diff Jul 26, 2014
...nstaller/user_project_integrator/target_integrator.rb
native_target.build_configurations.each do |config|
+ path = target.xcconfig_relative_path(config.name)
+ xcconfig = user_project.files.select { |f| f.path == path }.first || user_project.new_file(path)
@houndci-bot
houndci-bot added a note Jul 26, 2014

Line is too long. [110/80]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@houndci-bot houndci-bot commented on an outdated diff Jul 26, 2014
lib/cocoapods/installer.rb
@@ -177,6 +178,23 @@ def analyze
@aggregate_targets = analyzer.result.targets
end
+ # Ensures that the white-listed build configurations are known to prevent
+ # silent typos.
+ #
+ # @raise If a unknown user configuration is found.
+ #
+ def validate_build_configurations
+ whitelisted_configs = pod_targets.map do |target|
+ target.target_definition.all_whitelisted_configurations
+ end.flatten.uniq
+ all_user_configurations = analysis_result.all_user_build_configurations.keys.map(&:downcase)
@houndci-bot
houndci-bot added a note Jul 26, 2014

Line is too long. [98/80]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabiopelosin
CocoaPods member

I'm dropping Only generate one xcconfig when the user has not specified any configurations to whitelist pods for. because I don't think that there is need to have different behaviours in this case.

The complexity for CocoaPods and for the users understanding the build system is not worth it. Moreover there is little disadvantage in having multiple Xcconfigs per build configuration.

@fabiopelosin
CocoaPods member

Is the aggregate target still needed to trigger the build of the single Pods?

@fabiopelosin fabiopelosin unlocked this conversation Jul 27, 2014
@fabiopelosin
CocoaPods member

The approach highlighted in this patch is not working:

Problem
  • The Link Binary With Libraries build phase does not allo to specify libraries per build configuration, therefore this patch correctly migrated to the usage of the xcconfig to specify the libraries to link against with.
  • The implicit dependencies dependencies feature of Xcode uses the Link Binary With Libraries build phase to detect the dependencies.
  • As the CocoaPods aggregate target (now referred as the Pod bundle) is not listed anymore in the Link Binary With Libraries build phase this target and the Pods targets are not being built anymore when the integrate target is being built.
    • To verify this point all the targets need to be cleaned ($ rm -rf /Users/[USER]/Library/Developer/Xcode/DerivedData) because if they are already built the build will succeed.
  • Targets of other projects in the workspace cannot be specified in the Target Dependencies build phase to explicitly list the targets that should be built.
Solution A
  • Add the Pods project as a subproject of integrated project (with this solution the workspace can be dropped).
    • When Xcode adds a project as a subproject it lists all the targets in the parent project. Xcodeproj is mature enough to support this behaviour but the integrations becomes much more pervasive.
      • The subproject targets are listed as: PBXContainerItemProxy
      • The subproject targets are listed as: PBXReferenceProxy which refer to the PBXContainerItemProxy via the remoteRef attribute
      • The PBXReferenceProxy objects are referenced in a Products group (PBXGroup) (further investigation is needed because they are not listed in the Products group at the root of the integrated project in the UI)
  • Include the CocoaPods aggregate target (Pod bundle) in the Target Dependencies of the integrated target (alternatively all the Pods of the project can be listed)
Solution B
  • Keep the current workspace setup
  • Use the schemes feature to specify the decencies of the target.

I didn't further investigate this approach because I think that it could lead to a lot of complexity and could be more confusing for the user (we are listing the dependencies at a higher level of abstraction). Moreover users are used to regenerate the schemes automatically with Xcode if there is any problem.

Solution C

This solution would be to keep the current behaviour except for the Pods white listed in certain build configurations and use one of the other two solutions. However I have a strong opinion against it.

@fabiopelosin
CocoaPods member

I'm going with solution A

@fabiopelosin
CocoaPods member

This patch is ready. I have tested it in a project and appears to be working correctly. Given its complexity I'm leaving a 24h window to the members of the @CocoaPods/core team to test it before merging it.

The failure is related to the quotes not being present in the integration tests generated by Travis not present locally that I will investigate in the meanwhile.

@fabiopelosin
CocoaPods member

So the issue was related to xcproj not being invoked in Travis (a good reminder of the perils of magic and auto-pickup of features). Now the patch is green a completely ready.

@thebehera thebehera referenced this pull request in pristineio/webrtc-build-scripts Aug 12, 2014
Merged

Allow to build Release config of AppRTCDemo right from Xcode. #8

fabiopelosin and others added some commits Jul 25, 2014
@fabiopelosin fabiopelosin [Integration] Update 93137c3
@fabiopelosin fabiopelosin [Bundle]: udpate 413c4f3
@fabiopelosin fabiopelosin [Changelog] b74063c
@fabiopelosin fabiopelosin [Style] Tweaks and documentation e0e98e5
@fabiopelosin fabiopelosin [Installer] Handle build configuration case insensitively fbe5812
@fabiopelosin fabiopelosin [TargetIntegrator] Remove unused method 2cbbbcc
@fabiopelosin fabiopelosin [TargetIntegrator] Factor out XCConfig logic
Also includes some small fixes here and there
d80a738
@fabiopelosin fabiopelosin [XCConfigIntegrator] Migrate old installations d897a37
@fabiopelosin fabiopelosin [XCConfigIntegrator] Downcase the name of the build configurations d0ffe31
@fabiopelosin fabiopelosin [Installer] Use downcase to validate configurations 2a871b7
@fabiopelosin fabiopelosin [Examples] Update integration a81d816
@fabiopelosin fabiopelosin [Xcodeproj]: Fix issue for libraries with a space in the name 2ef0db1
@fabiopelosin fabiopelosin [PrefixHeader] Style fixes b3a77a6
@fabiopelosin fabiopelosin [AggretateTarget] Expose specs by build configuration cccca4f
@fabiopelosin fabiopelosin [TargetEnvironmentHeader] Refactor 55a8c91
@fabiopelosin fabiopelosin [Project] Add preprocessor definition for build configurations b233a8a
@fabiopelosin fabiopelosin [PrivatePodXcconfig] Add inherited to GCC_PREPROCESSOR_DEFINITIONS 3e0f914
@fabiopelosin fabiopelosin [Project] Coherce preprocessor definitions to an array 7904720
@fabiopelosin fabiopelosin [Integration] Disable xcproj in Xcodeproj 8f23ffc
@fabiopelosin fabiopelosin [Changelog] 7df377a
@orta orta add a space to the override warning 7870047
@alloy alloy [Docs] Some small fixes. fcd5f62
@fabiopelosin fabiopelosin [PodTarget] Ensure that subspecs are on the same configurations da54718
@fabiopelosin fabiopelosin [Bundle] Update 61ff1f3
@fabiopelosin fabiopelosin [AggregateTarget] Documentation cleanup ba68b81
@fabiopelosin fabiopelosin [AggregateTarget] Add #user_targets c68ce09
@fabiopelosin fabiopelosin [UserProjectIntegrator] Warn about xcconfig override after every inst…
…allation
d56aac9
@fabiopelosin fabiopelosin [UserProjectIntegrator] Warn about xcconfig override after every inst…
…allation /2
bbcf3ce
@fabiopelosin fabiopelosin merged commit bbcf3ce into master Aug 12, 2014

1 check was pending

Details continuous-integration/travis-ci The Travis CI build is in progress
@kylef
kylef commented Aug 13, 2014

And a 💯 👍's, high five.

@shouze
shouze commented Aug 13, 2014

👍 can't wait the next pod release

@fabb
fabb commented Aug 13, 2014

Have I read correctly that in the next pod release, the generated workspace is no longer needed?

@neonichu
CocoaPods member
@fabiopelosin
CocoaPods member

@fabb, no the workspace is still used as the integration mechanism of CocoaPods.

@fabb
fabb commented Aug 13, 2014

Ok, then I probably misunderstood Solution A.

@nevyn
nevyn commented Aug 13, 2014

Holy wow you got it merged‽‽‽ thank you thank you thank you, from the bottom of my heart!

@fabiopelosin
CocoaPods member

@nevyn thank your for the original contribution! 🍻

@alloy
CocoaPods member
alloy commented Aug 13, 2014

🎊

I am so glad that this has finally been merged.

@nevyn and @FabioPelosin my greatest gratitude to you both! ❤️

@supermarin
CocoaPods member

:heart: :heart:

@shouze
shouze commented Aug 20, 2014

Can't wait to use it!

@FabioPelosin can you plz make a new tagged release today?
We definitely need a 0.33.2 or 0.34.0 release of pod with the cool feature :)

@Ekhoo
Ekhoo commented Aug 20, 2014

👍

@kylef kylef locked and limited conversation to collaborators Aug 20, 2014
@kylef kylef deleted the lookback-pods-by-config branch Oct 18, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Something went wrong with that request. Please try again.