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

[WIP] Allow Accio dependencies to be integrated in a CocoaPods setup #50

Closed
wants to merge 6 commits into from

Conversation

acecilia
Copy link
Contributor

Please see the readme changes, which explains this PR.

This is WIP. Todo:
☐ Get feedback about the concept and the implementation.
☐ Add tests.

PD: thanks for Accio! During the past 4 months I have been dreaming about a package manager that can integrate binaries automatically in my Xcode project, doing proper version resolution. After working in the codebase for the last 3 days I can say it is exactly that :) I am very excited about the potential it has, I hope adoption happens fast!

@@ -1,17 +1,20 @@
import Foundation

struct FrameworkProduct {
let framework: Framework
Copy link
Contributor Author

@acecilia acecilia May 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to create the .podspec files I required to know the path to the framework (in the Dependencies folder) together with the framework information (dependencies, versions... etc). I found that having a reference to the Framework inside the FrameworkProduct was the easiest and safest way to go, and also makes sense to me: a FrameworkProduct always has an associated Framework.
There may be some source code improvements that can be done around now that FrameworkProduct contains Framework, but I did not include them in this PR in order to keep it as small and as easy to review as possible.

@fredpi
Copy link
Contributor

fredpi commented May 21, 2019

@acecilia Thanks for working on this 🚀

I didn't have an in-depth look on the implementation, but thought a bit about how this fits Accio's goal of targeting compatibility with SwiftPM. From the README: It offers many improvements over Carthage and is targeted towards SwiftPM's integration into Xcode.

Still, if this a problem many face, why should we artificially constraint what Accio does? Personally, I'm okay with such changes. 👍

Nonetheless, we should think a bit about it, as this is quite a fundamental decision about Accio's further development. Maybe @mrylmz has some thoughts?

@mrylmz
Copy link
Contributor

mrylmz commented May 22, 2019

@acecilia thanks for the great work!

It is fine to have support for CocoaPods usage as I have understand you right, then you want to integrate Accio dependencies into CocoaPods projects right?

Do you have any examples where we have the use-case to choose CocoaPods for dependency management over Accio?

I have also checked your commits looks good overall maybe we could come up with a better solution for the CocoaPods usage with something like a configuration file in the project root. The naming convention inside the Package.swift as you have proposed could be error-prone and could be too implicit.

@acecilia
Copy link
Contributor Author

@mrylmz I can not think in any use case where CocoaPods is preferred over Accio. This is for projects already setup on CocoaPods. For example: at the moment I am working in a project with a CocoaPods setup with multiple dependencies. I want to go away from it due to numerous reasons:

  • I want to avoid all the modifications done by CocoaPods to the workspace
  • I want to get rid of the long release process for private pods (‘pod lint’, ‘pod repo push’, and friends)
  • I want to have my dependencies as frameworks instead of source code, for faster builds

Removing CocoaPods for me means that I have to migrate all my private pods to a Carthage setup at once (because one depends on another, and that other on a third one, and so on). This is problematic, as depending on the project configuration such work may not be easy. Also, it may be that some of them are preferred to be used in CocoaPods in order to easily develop on them (configuring them as development pods). Finally, in my personal case migrating all my dependencies at once is not possible due to the high number of them.

Instead, a better solution is to slowly move the dependencies to frameworks following the technique used in this PR, and at some point in the future remove CocoaPods completely from my project (if that is even possible :D)

About the setup, I do not quiet see what is the problem of having a suffix in the targets inside the ‘Package.swift’ file. Can you elaborate a bit more on that?

Thanks for the feedback!

@mrylmz
Copy link
Contributor

mrylmz commented May 22, 2019

@acecilia The suffix naming requires additional maintenance in the Package.swift file and could also be misspelled by accident and could become hard to debug because Accio couldn't detect that it is incorrect, it would just fallback to a default target.

My suggestion would be to add a feature for project specific Accio configuration where you can specify which dependencies should handle CocoaPods support. In that way you could just remove CocoaPods dependencies from that list on partial Accio adoption.

@acecilia
Copy link
Contributor Author

acecilia commented May 22, 2019

@mrylmz I do not fully understand what you mean by The suffix naming requires additional maintenance in the Package.swift. Which kind of maintenance are you refering to?

About the second point, I still do not see the problem. if you misspell the suffix, then your project will not build, as your CocoaPods setup will fail.

I think adding a second Accio configuration file has some caveats:

  • It is misleading for the users, as they will need to work with two configuration files: Package.swift and (for example) Accio.yml.
  • It is not that good for us (developing Accio) as we will need to support a second configuration file: add it to the Accio command line flags, parse it... In addition, I can not envision very well how this fits in the actual implementation, it will probably require a bigger refactor than what i did in this PR (which is already big).

On the other hand, related to this issue we may need to add a way to specify how a dependency is integrated in the project. Options I can think at the moment (based on that issue and this PR) are: Dynamic (the default atm), Static, CocoaPods-Dynamic and CocoaPods-Static. The two ways I can think about implementing this are:

  • Using suffixes in the Package.swift:
    • Good: everything is contained in one file, Package.swift.
    • Bad: not extensible: adding any other Accio-related configuration will be hard, nasty or impossible, as it will need to be added somehow to Package.swift.
  • Using a second Accio configuration file, like Accio.yml:
    • Good: extensible.
    • Bad: adds a second configuration file, with the overhead this entails.

With that said, I expect SPM to add support for static frameworks at some point (the same as Xcode did), so the consumer of the dependency can specify how the framework is built and linked: dynamic or static.

@acecilia
Copy link
Contributor Author

In order to continue with this PR I need to know which one of this options to go for:

a) Approve this PR as it is (then, I will work in the tests).
b) Approve this PR as it is, and after that, start working on adding an Accio specific configuration file (something like Accio.yml), with the idea of switching to it in the future.
c) Put this PR on hold, and open another PR for making an Accio specific configuration file. After the configuration file is finished, update this PR to use it.

I am more into option b) or c), as it will be useful also for #48.

@mrylmz
Copy link
Contributor

mrylmz commented May 24, 2019

If you want to finish this PR, I would suggest option a) as short term solution and then we can open a new issue for a collection of configurations which we are not able to gather from SPM and then we will know the direction we want to go.

Also involving @Dschee into the discussions would be great, so we could defer the configuration file issue for a few days.

Thanks for supporting Accio!

Copy link
Contributor

@Jeehut Jeehut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just had a look at the README changes and a cursory look into the implementation and while the implementation looks okay-ish on first glance, I'm not sure yet about the goal here. To be honest, this sounds like a very specific issue to me.

Using Accio and CocoaPods within one and the same project should be already possible with the current state of Accio. What this adds – if I understand it correctly – is an option to integrate some of the subdependencies of a dependency via Accio to save build time on clean builds while the actual dependency is still included via CocoaPods (cause it doesn't support Accio) – is that right?

If so, I'm not sure if this is worth all the code considering that this really is a performance improvement only and doesn't apply to many cases. Feel free to explain why I might be wrong here, but to me it sounds like the more "correct" fix in those situations would be to make the actual dependency compatible with Accio and include it via Accio only.

As of my understanding now, I'm against merging this until either someone clarifies a misunderstanding on my side or more people may actually run into this very problem and profit from it. Then, of course, I will take a second look.

Please also note, that I don't like the idea on adding some prefix to the name of a target like "-AccioCocoaPodsIntegration" and using that as the source for decision making. I'd rather add a comment-style parameter or use some other solution. Targets should be named as what they are IMHO.

@acecilia
Copy link
Contributor Author

acecilia commented Jun 11, 2019

@Dschee About the concept:

Using Accio and CocoaPods within one and the same project should be already possible with the current state of Accio.

This is correct, but only if the dependencies integrated using CocoaPods do not depend on the ones integrated using Accio.

What this adds – if I understand it correctly – is an option to integrate some of the subdependencies of a dependency via Accio to save build time on clean builds while the actual dependency is still included via CocoaPods (cause it doesn't support Accio) – is that right?

Correct, but with some details:

  • It may be that a dependency has Accio support, but it's preferred as a pod: there may be multiple reasons for this, but one I can think of is for working on it as a development pod.
  • Also, think about projects that integrate their dependencies using CococaPods: breaking out of CocoaPods is not easy, as the dependencies between pods forces the consumer to switch all the pods at once to Carthage or Accio. For big projects, this is a problem, as it may require updating multiple private pods to support a new package manager. This PR allows to move the dependencies from one package manager to another in a less disruptive way. As a bonus, easing the migration process may encourage CocoaPods users to switch to Accio: without something like this I do not think that a team will decide to switch their projects and private pods from CocoaPods to Accio if they do not find major issues while using CocoaPods.

If so, I'm not sure if this is worth all the code

This implementation did very few changes to the way Accio works and the existing code, it just added an extra way of integrating the dependencies. In addition, as @fredpi said: why should we artificially constraint what Accio does?

considering that this really is a performance improvement only and doesn't apply to many cases.

I do not this this is only a performance improvement:

  1. This is providing the consumers with another option to integrate their dependencies into their projects. IMO consumers of the dependencies should not be limited by the integration options provided by the dependency authors if they do not have to. For example: why when I integrate Moya using CocoaPods, I am also forced to integrate Alamofire in the same way? Shouldn't I be able to integrate Alamofire in any way I want, source code/dynamic framework/static framework? Furthermore, shouldn't I be able to change how a dependency is integrated in a seamless way, using it as source code when is under continuous development, or as a framework when is rarely updated? I believe the answer is yes to both questions.
  2. Reducing build times may be just a performance improvement for small projects, but is a deal breaker for big projects. In addition, long build times is a waste of time and money for everybody: companies and developers. For a bit more context, I am working on an app with around 25 CocoaPods dependencies, and using this PR and switching some of them to frameworks I managed to reduce the build time by 43%.
  3. This PR allows to use CocoaPods and Accio dependencies together, so the consumer is not limited to one only package manager, and can enjoy the goodies of both of them. There is no need anymore for doing the classic CocoaPods VS Carthage comparisons, as now they can coexist [some links: 1, 2].

About the implementation:

Please also note, that I don't like the idea on adding some prefix to the name of a target like "-AccioCocoaPodsIntegration" and using that as the source for decision making. I'd rather add a comment-style parameter or use some other solution. Targets should be named as what they are IMHO.

Of course, the implementation may not be ideal, and as commented here there may be other ways of doing this instead of using custom target names. This is also very much related to the outcome of #54.

@Jeehut
Copy link
Contributor

Jeehut commented Jun 12, 2019

I'm still not completely sold on this feature, but I can see how big projects with old dependencies who are unwilling to update to a modern package manager would profit from it. Therefore I'm not against merging this, once it's implementation is reviewed and tested by at least one other person who also runs into such problems to evaluate if this is useful for them as well.

Having that said, since this is more of an edge case rather than "the usual use case" for Accio, having tests (also integration tests) for this feature is very important. For that, you could either actually write unit tests (at least for some of the code changes) or at least provide a Demo-project like setup that we can run manually and then run the unit tests there to ensure this actually continues to work. Please have a look into the existing Demo project, which I use to manually check if some of the most popular frameworks can still be built and integrated via Accio by running accio update in the Demo project and then running it's unit tests.

Also I think it should be made clearer in the documentation what the setup where users could use this feature looks like. An example like "App Project >> Dependency via CocoaPods >> Subdependency via Accio (mixed into CocoaPods by Accio)" would be great, also listing the advantages of the feature (like less subdependency build time, indirect support for CocoaPods-only frameworks).

Another idea: What if we introduced a way of specifying that a dependency doesn't support Accio but only CocoaPods and Accio would automatically generate the required Podfile itself? Is your use case, that "only some" libraries don't support CocoaPods (or are developed upon) so that this would be even cleaner or is it more like "most dependencies are defined via CocoaPods, I just want to speed some supdependencies up"? I think I would feel much better if the Podfile wasn't ever touched by the users directly but instead be an implementation detail generated by Accio. But that's just an idea ... I could think of other modes as well, like the Podfile only being generated temporarily to run CocoaPods commands and then deleting it again so it's never checked in into source control. But I'm not sure if such things are possible with the very "controlling" way of how CocoaPods integrates with Xcode ...

@acecilia
Copy link
Contributor Author

I will close this, as I will be working on it on another branch

@acecilia acecilia closed this Aug 15, 2019
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

4 participants