Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add implicit type to Dependency #165

Closed
wants to merge 0 commits into from
Closed

Conversation

sbarow
Copy link
Contributor

@sbarow sbarow commented Nov 20, 2017

As a first pass, this seems like all that is needed to add an implicit framework dependency. Are there any other edge cases we need to worry about?

I'll add some tests when I get a 👍

Fixes: #163

@yonaskolb
Copy link
Owner

Nice! Have you tested it on your own project?

@sbarow
Copy link
Contributor Author

sbarow commented Nov 20, 2017

Yeah, tested in a workspace with 5 .xcodeproj (workspace and projects are all generated). Works in simulator and on device.

@yonaskolb
Copy link
Owner

Awesome. I wonder if this should be a new type of dependency or just an option on .framework?

@sbarow
Copy link
Contributor Author

sbarow commented Nov 20, 2017

That could work. Just need to be explicit in the docs that it only applies to framework type.

@yonaskolb
Copy link
Owner

Yeah, there are already some options that are only applied to certain types, or have different defaults. That reminds me, can you update ProjectSpec.md?

Maybe an implicit boolean? Thoughts anyone? As future work may impact this decision.

@jerrymarino
Copy link
Contributor

This is interesting, and perhaps adding transitive dependency logic could be useful.

Let's say I've got a project:

 name: SimpleProject
  targets:
   App:
     type: application
     dependencies:
      - target: Transitive
   Transitive:
     type: framework
     dependencies: 
      - target: Simple
   Simple:
     type: framework
     platform: iOS
     dependencies: 
       - implicit: Implicit.framework

We'd want to propagate all of the dependencies to the Application, so that they are all linked and or embedded. The App linker invocation would need -framework Transitive, -framework Simple, -framework Implicit, and the frameworks would need to be copied over.

The current way I'm achieving this is adding all transitive deps directly to top level targets, like Application, Extension, etc..
Here is what the required spec would be:

   App:
     type: application
     dependencies:
      - target: Transitive
      - target: Simple
      - implicit: Implicit.framework

Is this something that XcodeGen should handle internally, instead of doing it in the spec?

Based on #163 it seems like this may be related?

@yonaskolb
Copy link
Owner

yonaskolb commented Nov 21, 2017

@jerrymarino good point. At the moment only carthage dependencies are automatically copied into app targets even if they aren't defined in that target, but are in a dependent target. (this isn't the case for macOS targets anymore by the way as it hasn't been added back yet after the changes in #76 removed that as a side effect)

This would be possible for other dependency types as well, but I just thought there would be too many edge cases and special build settings and build scripts that people might have, to do this reliably for anything else.
Perhaps another one for ProjectSpec.options?

@jerrymarino
Copy link
Contributor

@yonaskolb agreed, I like the idea of not imposing those semantics on everyone without a way to turn it off.

The above mentioned logic should generalize for many use cases: Transitively adding frameworks and static libraries ( this is similar to how cocoa pods works ) to top level targets.

I think it's a sensible default, but ProjectSpec.options could turn it off. For most cases, it'd make XcodeGen easier to use. Based on my understanding of this, people wouldn't need to have Implicit targets since they would be implicit in XcodeGen.

@sbarow I think manually adding transitive deps to applications and extension targets may solve this without a new dependency type in XcodeGen 🤷‍♂️ ?

@sbarow
Copy link
Contributor Author

sbarow commented Nov 21, 2017

@jerrymarino the goal is to not have to do anything manually 😜

Regarding transitive dependencies, its an interesting one and something I have thought about. At Uber we use buck and do transitive dependency resolution manually (with the help of auditing that lets us know if we are missing a dependency in the BUCK file) I believe its a more sane approach to let the user know they are missing a dependency for a target, the other thing to consider is cyclic dependencies.

I'll take a look at both in another diff?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants