Add dedicated targets #983

Closed
wants to merge 6 commits into
from

Projects

None yet

4 participants

@jasl8r
jasl8r commented Apr 18, 2013

This (in part) resolves issue #841

It has been tested for functionality with multiple user targets and subspecs. The primary goal was for complete spec source isolation which was mostly solved by adding the per-spec targets as well as adding the USE_HEADERMAP = NO option to the Pod targets. This is not added to the xcconfig file. It's possible that setting USE_HEADERMAP = NO will break other libraries that do not do properly include headers by specifying the correct paths.

jasl8r added some commits Apr 18, 2013
@jasl8r jasl8r Add per-spec static library targets
Static library targets are created for each combination of user-target
and spec dependency.  This addresses issue #841, although likely not 
entirely.
1d51f82
@jasl8r jasl8r Identify copy build phase by spec name fbfea26
@jasl8r jasl8r Only add a single Manifest.lock phase when dealing with multiple specs 5cb58ad
@alloy
Member
alloy commented Apr 18, 2013
@alloy
Member
alloy commented Apr 19, 2013

Related #904.

@fabiopelosin
Member

Sorry, I have been a bit busy and I didn't have enough time to review this awesome patch.

This patch currently adds the xcconfigs and the libraries generated for each pod to the client target (it is unclear to me if this is because it is still WIP). This is undesirable because we would need to have a lot more of house keeping to do in user target and there we can't simply recreate things from scratch. Moreover is one of the design goals of CP to be as lees intrusive as possible. Ideally we should have some like:

  • PodXcconfigGenerator and TargetXcconfigGenerartor
  • PodTargetInstaller and PodLibraryTargetInstaller

Other than that it looks good after a quick overview. Great work!

It's possible that setting USE_HEADERMAP = NO will break other libraries that do not do properly include headers by specifying the correct paths.

Can you provide a concrete example?

@fabiopelosin
Member

Btw, feel free to merge master to fix the specs on Travis.

@jasl8r
jasl8r commented Apr 19, 2013

What is the git workflow to use? Keep history and merge master, or rebase, squash fixups and push -f?

@jasl8r
jasl8r commented Apr 19, 2013

Yes, I don't believe this should be merged in yet. The xcconfig issue is the biggest one that is clear to me at this point. It seems like there should be a single xcconfig file generated per user target, as this is the only way to separate public headers per user target. At least that is the only way to resolve #904.

Is there even a need for the library targets to inherit from an xcconfig, or should all the library settings be put directly in the xcode target definition for the library? Then we would be back to one xcconfig per user target.

Related to #904 each library target should probably only have a public header path for the libraries in it's dependency list. With this patch I am still using the sandbox public header path for all library and user targets.

@fabiopelosin
Member

We don't obsses with history, so you can simply merge.

Sent from my iPhone

On Fri, Apr 19, 2013 at 9:38 PM, jasl8r notifications@github.com wrote:

What is the git workflow to use? Keep history and merge master, or rebase, squash fixups and push -f?

Reply to this email directly or view it on GitHub:
#983 (comment)

@fabiopelosin
Member

For the xcconfigs the pod ones should just declare variable like the POD_ROOT one. The lib xcconfigs just lists the variables. There is an issue open with more context. 

Sent from my iPhone

On Fri, Apr 19, 2013 at 9:45 PM, jasl8r notifications@github.com wrote:

Yes, I don't believe this should be merged in yet. The xcconfig issue is the biggest one that is clear to me at this point. It seems like there should be a single xcconfig file generated per user target, as this is the only way to separate public headers per user target. At least that is the only way to resolve #904.
Is there even a need for the library targets to inherit from an xcconfig, or should all the library settings be put directly in the xcode target definition for the library? Then we would be back to one xcconfig per user target.

Related to #904 each library target should probably only have a public header path for the libraries in it's dependency list. With this patch I am still using the sandbox public header path for all library and user targets.

Reply to this email directly or view it on GitHub:
#983 (comment)

@jasl8r
jasl8r commented Apr 19, 2013

Regarding the USE_HEADERMAP breakage; without testing, I assume this would break if you create a podspec with a header_mappings_dir but #import without specifying the absolute path. Since you flatten the files otherwise, I think this is the only edge case. That said, if someone is using header_mappings_dir it is likely because they are using properly namespaced imports, so maybe it's not a big deal.

For example:

Directory structure:

include/prefix/headerA.h
include/prefix/headerB.h
src/library.c

library.c:

#include <prefix/headerA.h> // This is okay
#include <headerB.h> // This will fail with USE_HEADERMAP = NO
...
@coveralls

Coverage Status

Coverage remained the same when pulling 9a71521 on jasl8r:dedicated-targets into c82eb03 on CocoaPods:master.

@fabiopelosin
Member

@jasl8r As this is great work let me know if you would like push access (so we can move this in a branch and complete it together) and whether you would like to join our Campfire room.

@jasl8r
jasl8r commented Apr 25, 2013

Sure, I'm finishing up a solution to deal with the xcconfig files right now. How does campfire work?

@jasl8r jasl8r Support xcconfig files per Podfile target
Xcconfig files are generated for every library target and target 
defined in the Podfile.  However, only the Podfile target xcconfig 
files are added to the user project.  This required tracking all the 
libraries as well as the Podfile targets through the installation and 
integration procedures.
717dc51
@coveralls

Coverage Status

Coverage remained the same when pulling 717dc51 on jasl8r:dedicated-targets into c82eb03 on CocoaPods:master.

@jasl8r
jasl8r commented Apr 26, 2013

This latest change completely sandboxes target dependencies in a project such that each target only includes headers for its own dependencies. This is more or less as a side effect of correctly creating an xcconfig file for every target in the Podfile. This may fix #904.

There are likely some missing pieces here still, particularly regarding custom flags like ARC and such that I haven't tested.

A lot of tests need to be updated and written, but I have tested a few of my own scenarios. This works well with dependencies and sub-dependencies and subspecs. Due to the sandboxing it did uncover some sloppy dependencies in my podspecs, so be aware some podspecs may need updating.

@fabiopelosin
Member

Sure, I'm finishing up a solution to deal with the xcconfig files right now. How does campfire work?

Added. Campfire is a persistent chat that we use to discuss about CP. I would need your email to invite you.

Great work... I will be able to have a more in deep look at it later today. Now that you have push access I plan to move it to a branch in this repo so we join forces to fine tune the last bits.

@fabiopelosin
Member

I've performed some tests and this patch is working fine for me. However before being merged the following areas need to be addressed:

  • There should be only one library and one xcconfig for the client target, I've just tested creating a stating lib which simply lists the other ones as dependencies and it appear that there are no issues going with this route. I'm not sure about the xcconfig as I haven't yet completely read your patch, but from my quick experimentation you are not creating one which includes all the others (and thus the resulting project is working only because the old xcconfig was not deleted).
  • Only one copy resource script should be created per target.
  • Finally in the common xccofing all the headers of all the targets should be made visible (temporarily) until an adequate DSL is implemented to fix #904.

Those changes are inline with the CocoaPods principle of being as less invasive as possible in the user project.

Other than that is looking very good. I have created a branch whit this patch and we could start a new pull request from that one if you would like to collaborate. Otherwise, rock it! 😄

@jasl8r jasl8r referenced this pull request Apr 30, 2013
Merged

Dedicated targets #1011

@jasl8r
jasl8r commented Apr 30, 2013

Closing in favor of #1011.

@jasl8r jasl8r closed this Apr 30, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment