Improve de-duplication for non-trivial cases #4146

mrackwitz · 2015-09-09T09:37:43Z

Attempts to fix #4034.
Attempts to fix #3838.

Given that there is an integration like:

target 'FooApp' do
  platform :osx, '10.10'
  pod 'BananaKit'
end

target 'FooAppEx' do
  platform :osx, '10.10'
  pod 'BananaKit'
end

target 'BarApp' do
  platform :ios, '9.0'
  pod 'BananaKit'
end

target 'BarAppEx' do
  platform :ios, '9.0'
  pod 'BananaKit'
end

The analyzer will generate a model with the following pod targets:

Pods-FooApp-BananaKit with aggregate targets Pods-FooApp and Pods-FooAppEx
Pods-BarApp-BananaKit with aggregate targets Pods-BarApp and Pods-BarAppEx

So even though a pod is used on multiple platforms, the analyzer is able to recognize that it could be reused between app and its extension on the same platform in this example, but generally not limited to that.

The problem is that the pod target will be build in a scoped build dir, which is not available from all apps and pod targets. To stick with the example: the scoped build dir is added to the FRAMEWORK_SEARCH_PATHS of FooApp but not FooAppEx, because the deduplication/scoping logic makes at some places in the code the assumption if a pod target is scoped than just one target definition belongs to it. This may seem to be easily solved by just adding the same framework search path to FooAppEx as well, but this wouldn't work in any case, because there could be other dependencies from FooApp, which are build with e.g. other subspecs and so not thought for FooAppEx, but become visible which can cause indeterministic behavior and warnings because there are two (or more versions) of the same framework in the framework search paths.

So the solution is to change the way, they are scoped and ensure that pods targets are built to a directory, which doesn't cause clashes with other pod targets. But this will cause massive changes to the integration, likely breaking for anything relying on how scoped targets where named before.
Where a scoped target was named before Pods-MyAppExtension-BananaKit, the name/scope now indicates what's special about the build variant, which allow these cases:

BananaKit-iOS: if BananaKit was used on different platforms
BananaKit-iOS9.0: if BananaKit was used on different platforms and versions
BananaKit-PalmTree-Leaf: if BananaKit was used with different sets of subspecs, the name shows which subspecs are special to the specific target, leaving out common subspecs
~~BananaKit-MyApp-MyAppExtension: if both of the condition above apply, the name shows all target definitions where it is in use~~
BananaKit-iOS9.0-PalmTree-Leaf: if both of the condition above apply, the name uses both schemes in combination

The order here is relatively arbitrary, but tries to prefer a shorter and simpler target name first, as especially 3. & 4. can become ugly and very long.

ToDo

Test for case 2, if relevant?
Test for case 3 with multiple subspec names
~~Test for case 4 with multiple target names~~
Test for case 5 with multiple target names
Rebuild integration specs
Changelog

Note

In case of 1. the scoping of the build products could be dropped, because the default $CONFIGURATION_BUILD_DIR covers already a scoping by platform and architecture.

⚠️ Transitive dependencies are still causing a duplication with the target based naming scheme.

neonichu · 2015-09-09T12:23:34Z

🎉

I wonder though if we should simply throw an error in the application extension case, since we already know that duplicated dependencies will lead to problems when archiving apps. Maybe at least show a warning that this probably won't work?

segiddins · 2015-09-10T23:37:54Z

lib/cocoapods/installer/analyzer.rb

+          end
+        end
+
+        Hash[result.map { |d,name| [d,name.gsub('/', '_')] }]


spacing. Use tr instead of gsub

mrackwitz · 2015-09-11T12:03:22Z

I wonder though if we should simply throw an error in the application extension case, since we already know that duplicated dependencies will lead to problems when archiving apps. Maybe at least show a warning that this probably won't work?

Throw an error or print a warning when app and app extension don't share the same dependency? We would need first be able to recognize whether both belong to each other.

neonichu · 2015-09-11T12:10:39Z

@mrackwitz true, but shouldn't be too hard to obtain that information. Will look into that.

segiddins · 2015-09-25T14:44:48Z

lib/cocoapods/generator/xcconfig/aggregate_xcconfig.rb

@@ -94,15 +94,15 @@ def generate
        #
        def generate_settings_to_import_pod_targets
          if target.requires_frameworks?
-            framework_header_search_paths = pod_targets.select(&:should_build?).map do |target|
+            build_pod_targets = pod_targets.select(&:should_build?)
+            framework_header_search_paths = build_pod_targets.map do |target|
              if target.scoped?


looks like it no longer needs to be a conditional

segiddins · 2015-09-25T15:04:08Z

This looks very good! Biggest thing for me is seeing a bevy of unit tests around the method that generates scope suffixes.
We also need to investigate if there's a max length we can make a pod target's name

segiddins · 2015-10-27T01:07:49Z

@mrackwitz I can pick this up if you want, just let me know what needs to be done

segiddins · 2015-11-19T17:06:04Z

@mrackwitz I tried to rebase this yesterday but ran into some issues. Could you please handle rebasing it?

benasher44 · 2016-01-08T02:50:46Z

Noted in #3838 that I think I'm having a similar issue, and I'm using 1.0.0.beta.2. I'm happy to give this branch a try and see if it solves my issue described there and report back, once this gets PR rebased.

jafara · 2016-01-23T05:48:00Z

Would love to see this land!

mrackwitz · 2016-01-25T14:20:17Z

I have reawaked this PR and extended it with the outstanding tests. In progress of that, I fixed the naming scheme as seen in the updated comment above as well, so that the names stay consistent. But this needs still further testing with real-world projects, especially with those, which where concerned from this issue.

segiddins · 2016-01-25T14:29:40Z

CHANGELOG.md

@@ -6,6 +6,16 @@ To install release candidates run `[sudo] gem install cocoapods --pre`

 ## Master

+##### Breaking


I think this is a bug fix, not a breaking change?

I classified it as breaking because it will cause huge changes to the integration. Actually it shouldn't break anything beside podfile installation hooks, which could be affected by any change. But then I would rather say it's an enhancement. This PR is still labeled as defect, because it predates #4147. Since that was merged, everything what is left here to do was more a known feature limitation, I guess.

benasher44 · 2016-01-25T15:37:57Z

Okay, I just tried this out, and it still won't compile, but the error is slightly different. To start out, there's one thing that stands out as a something that looks weird/incorrect. The file in question that won't compile has a framework search path that looks like this:

-F/Users/basher/Code/iphone/Pods/"/Users/basher/Library/Developer/Xcode/DerivedData/Yelp-fyngslzoeilfvcblkbskorhepqvd/Build/Products/Debug-iphonesimulator/Pods-YelpShared-Yelp"

It looks like it's trying to make it so that it has the framework search paths of /Users/basher/Code/iphone/Pods/ and /Users/basher/Library/Developer/Xcode/DerivedData/Yelp-fyngslzoeilfvcblkbskorhepqvd/Build/Products/Debug-iphonesimulator/Pods-YelpShared-Yelp. The syntax here for specifying multiple framework search paths seems funny/incorrect, and the correct include paths should be

/Users/basher/Library/Developer/Xcode/DerivedData/Yelp-fyngslzoeilfvcblkbskorhepqvd/Build/Products/Debug-iphonesimulator (this is where the pod framework is whose subspec is used by one target and whose pod framework in its entirety is used by a pod in the main target)
/Users/basher/Library/Developer/Xcode/DerivedData/Yelp-fyngslzoeilfvcblkbskorhepqvd/Build/Products/Debug-iphonesimulator/Pods-YelpShared-Yelp

Let me know if you need more info to help debug this. If it helps, /Users/basher/Code/iphone/Pods is $(SRCROOT)/Pods

segiddins · 2016-01-25T16:52:13Z

Is there any chance this will also fix #4751?

Reduces code complexity a bit.

Some of these might not actually return nothing, but none of these methods give a guarantee to return a certain result.

benasher44 · 2016-02-23T00:41:12Z

Commit 4333212 still working for me.

segiddins · 2016-02-23T00:47:35Z

@mrackwitz if this is no longer a WIP, it gets a 👍 from me. Great job! 🚀

mrackwitz · 2016-02-23T10:19:50Z

Okay, let's 🚢 it!

Improve de-duplication for non-trivial cases

neonichu · 2016-02-23T10:23:33Z

orta · 2016-02-23T12:43:40Z

oh, wow

Targets use since #4146 individual build directories in all cases, not only these.

The behavior changed drastically with #4146, so that relying on this outside of this class was in most cases wrong.

Targets use since #4146 individual build directories in all cases, not only these.

The behavior changed drastically with #4146, so that relying on this outside of this class was in most cases wrong.

mrackwitz added the t2:defect These are known bugs. The issue should also contain steps to reproduce. PRs welcome! label Sep 9, 2015

mrackwitz mentioned this pull request Sep 9, 2015

[Analyzer] Build scoped pods once for each target definition #4147

Merged

1 task

segiddins reviewed Sep 10, 2015
View reviewed changes

mrackwitz mentioned this pull request Sep 25, 2015

mismatched PODS_FRAMEWORK_BUILD_PATH when building multiple targets and watchos #4231

Closed

segiddins reviewed Sep 25, 2015
View reviewed changes

neonichu mentioned this pull request Sep 30, 2015

Sample project including test fails to link tests #4295

Closed

segiddins mentioned this pull request Oct 6, 2015

Missing framework error when including a pod in one target and a subspec from that pod in another target #3838

Closed

segiddins mentioned this pull request Nov 13, 2015

FrameworkSearchPath issue for Xcode Framework (ld: framework not found) #4516

Closed

mrackwitz force-pushed the mr-fix-scoping branch from f5f0616 to 9a8adad Compare January 25, 2016 14:12

mrackwitz added a commit that referenced this pull request Jan 25, 2016

[CHANGELOG] Add entry for #4146

07c0a23

segiddins reviewed Jan 25, 2016
View reviewed changes

mrackwitz force-pushed the mr-fix-scoping branch from 9a8adad to 3b7f9fa Compare January 25, 2016 23:07

mrackwitz added a commit that referenced this pull request Jan 25, 2016

[CHANGELOG] Add entry for #4146

f3477e7

mrackwitz and others added 10 commits February 21, 2016 20:22

Pass correctly scoped path to EmbedFrameworksScript

8214547

[Analyzer] Fix transitive dependency selection

b165493

[IntegrationSpecs] Update for improved deduplication

a51c0d1

[XCConfig] Always set search paths

c774957

Reduces code complexity a bit.

[PodTarget] Introduce #build_product_path

3f915e5

[Doc] Document void return type for XCConfig methods

0bb65a6

Some of these might not actually return nothing, but none of these methods give a guarantee to return a certain result.

[CHANGELOG] Fix typo

07acbf3

[XCConfig] Fix search path inheritance support

f672cc9

[XCConfig] Avoid duplicate occurences of '$(inherited)'

e0fab2a

[IntegrationSpecs] Update for xcconfig fixes

4333212

mrackwitz force-pushed the mr-fix-scoping branch from eb30a9d to 4333212 Compare February 22, 2016 23:08

mrackwitz changed the title ~~[WIP] Improve de-duplication for non-trivial cases~~ Improve de-duplication for non-trivial cases Feb 23, 2016

mrackwitz added a commit that referenced this pull request Feb 23, 2016

Merge pull request #4146 from CocoaPods/mr-fix-scoping

d504d84

Improve de-duplication for non-trivial cases

mrackwitz merged commit d504d84 into master Feb 23, 2016

mrackwitz deleted the mr-fix-scoping branch February 23, 2016 10:19

This was referenced Feb 23, 2016

[PodVariantSet] Naive algorithm for #scope_by_specs #4934

Closed

Multi-Targets configuration and abstract_target build #4863

Closed

Fix resource bundle paths for improved deduplication #4954

Merged

mrackwitz added a commit that referenced this pull request Mar 12, 2016

[PodTargetInstaller] Fix bundle target's build dir

cbc3f48

Targets use since #4146 individual build directories in all cases, not only these.

mrackwitz added a commit that referenced this pull request Mar 12, 2016

[PodTarget] Get rid of #scoped?

250d924

The behavior changed drastically with #4146, so that relying on this outside of this class was in most cases wrong.

segiddins pushed a commit that referenced this pull request Mar 13, 2016

[PodTargetInstaller] Fix bundle target's build dir

91f2289

Targets use since #4146 individual build directories in all cases, not only these.

segiddins pushed a commit that referenced this pull request Mar 13, 2016

[PodTarget] Get rid of #scoped?

1f065f0

The behavior changed drastically with #4146, so that relying on this outside of this class was in most cases wrong.

efirestone pushed a commit to efirestone/CocoaPods that referenced this pull request May 17, 2016

[CHANGELOG] Add entries for CocoaPods#4146

6cf7dba

efirestone pushed a commit to efirestone/CocoaPods that referenced this pull request May 17, 2016

[CHANGELOG] Add further entry for CocoaPods#4146

d958553

wfilleman mentioned this pull request May 19, 2016

Storyboard IBDesignable is broken in 1.0.0.beta4 - 1.0.0 Release. #5377

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve de-duplication for non-trivial cases #4146

Improve de-duplication for non-trivial cases #4146

mrackwitz commented Sep 9, 2015

neonichu commented Sep 9, 2015

segiddins Sep 10, 2015

mrackwitz commented Sep 11, 2015

neonichu commented Sep 11, 2015

segiddins Sep 25, 2015

mrackwitz Sep 25, 2015

segiddins commented Sep 25, 2015

segiddins commented Oct 27, 2015

segiddins commented Nov 19, 2015

benasher44 commented Jan 8, 2016

jafara commented Jan 23, 2016

mrackwitz commented Jan 25, 2016

segiddins Jan 25, 2016

mrackwitz Jan 25, 2016

benasher44 commented Jan 25, 2016

segiddins commented Jan 25, 2016

benasher44 commented Feb 23, 2016

segiddins commented Feb 23, 2016

mrackwitz commented Feb 23, 2016

neonichu commented Feb 23, 2016

orta commented Feb 23, 2016

		@@ -6,6 +6,16 @@ To install release candidates run `[sudo] gem install cocoapods --pre`

		## Master

		##### Breaking

Improve de-duplication for non-trivial cases #4146

Improve de-duplication for non-trivial cases #4146

Conversation

mrackwitz commented Sep 9, 2015

ToDo

Note

neonichu commented Sep 9, 2015

segiddins Sep 10, 2015

Choose a reason for hiding this comment

mrackwitz commented Sep 11, 2015

neonichu commented Sep 11, 2015

segiddins Sep 25, 2015

Choose a reason for hiding this comment

mrackwitz Sep 25, 2015

Choose a reason for hiding this comment

segiddins commented Sep 25, 2015

segiddins commented Oct 27, 2015

segiddins commented Nov 19, 2015

benasher44 commented Jan 8, 2016

jafara commented Jan 23, 2016

mrackwitz commented Jan 25, 2016

segiddins Jan 25, 2016

Choose a reason for hiding this comment

mrackwitz Jan 25, 2016

Choose a reason for hiding this comment

benasher44 commented Jan 25, 2016

segiddins commented Jan 25, 2016

benasher44 commented Feb 23, 2016

segiddins commented Feb 23, 2016

mrackwitz commented Feb 23, 2016

neonichu commented Feb 23, 2016

orta commented Feb 23, 2016