Adds ability to include related frameworks when using Carthage

[rpassis]

Hey all,

I'm opening this PR for issue #488 early to get the ball rolling and get some feedback on the solution.

The way this is currently designed, and due to our dependency on the file system to evaluate related dependencies, has made it very difficult to test the logic around including related dependencies.

At the moment I am not exactly sure how to get around it and am open to ideas.

Other than that everything else is pretty straightforward and will hopefully make sense but if you have any questions feel free to ask away!

Rog

[yonaskolb]

Awesome work @rpassis! This will be so useful.
I’ll review this properly later, but regarding your testing issue you can write files to disk in a temporary directory during your test. The source generator tests do this. You can also leverage the carthageBuildPath option if you want to point to such a temporary directory from a programmatically created spec

[ellneal]

Is there no way to use CarthageKit to resolve the embedded dependencies rather than reading the file directly?

(I was going to try and do this myself and that's where I was planning to start).

[yonaskolb]

Yeah that's a good point @ellneal. There looks to be VersionFile in CarthageKit we can leverage

[rpassis]

Just so I understand, are you suggesting that we add Carthage/CarthageKit as a dependency?

I'm not sure that I could justify that just for reading the version file from disk. There's a lot of baggage that we will need to inherit if we go down that path, plus after a quick glance it looks like the VersionFile itself is internal to the framework so we won't be able to access it anyway.

What do you think?

[yonaskolb]

Yeah it would mean adding Carthage as a dependency. Advantage is having access to the proper formal and tested version of the file spec. Also means if Carthage updates the file format we can just easily update the dependency.
But as you say @rpassis it looks like it's internal struct, so it's fine to have it in here for now in that case.
Perhaps @ellneal was suggesting though using some of the public apis at a higher level to actually get at each of the binary paths for a dependency. Haven't looked into if that is possible, but from a quick glance it doesn't look like the api is geared towards that.
So all that said I think the approach you've taken here is good.

[rpassis]

So I played around a bit with trying to get CarthageKit included as a dependency and it seems like it could be worth a try if there is interest in using it for more than just this particular feature.

I can see potential in, for example, having a project spec that only specifies that it wants to add dependencies from a Cartfile, and xcodegen can take care of the rest.

FWIW introducing CarthageKit also means introducing a few other dependencies include ReactiveSwift which in itself is a big deal as it will definitely have a significant impact on how the code base for this project will evolve.

Let me know what you think!

[yonaskolb]

Does CarthageKit allow us to somehow achieve the goal of find a list of frameworks for a carthage dependency? If not, there's no reason to bring it in.
I don't think just using the cartfile for linking dependencies will work, as each target will link to different ones, plus you can't specify any options for them.
In terms of ReactiveSwift that doesn't need to be used in the rest of the codebase.

[ellneal]

Good points made. I have to admit that my 15 minute foray into doing this did hit a few roadblocks with CarthageKit not building correctly. Maybe something to discuss another time, and not in the interest of implementing this new feature.

[rpassis]

@yonaskolb you're right about the Cartfile.

From what I've seen so far, the goal of the .version file is to allow for the build process to use cached dependencies where possible so I can say with a high level of confidence that we will not be able to use Carthage just to find a list of frameworks for a dependency as this just an implementation detail within the build process.

I'll do a bit more digging and if it's a dead end I'll just move along with what's already here, and start working on adding unit tests.

[yonaskolb]

Nice work @rpassis, I've added some small comments above

[yonaskolb]

nitpick: can we drop the underscore

[rpassis]

Fixed in 12a46f20df8da47f928d234d9f673a1f0a19cab3

[yonaskolb]

As above, as these aren't actually part of the Cartfile this will fail. They can go into a different fixture if you want an integration test on them. You could find a real carthage dependency with multiple frameworks to add to the Cartfile in this project, but I don't want to slow down CI too much

[rpassis]

Cool I went with your suggestion, created a dependency fixture and added to the TestProject's Cartfile. At the moment it is pointing to rpassis/CarthageTestFixture but feel free to clone that repo to your own and we can update the Cartfile accordingly.

You can see the changes in ac8224ebfb8e83aa1c49a6564f7aaa7763780f9f

[rpassis]

Hey @yonaskolb CI is failing on the project diff here and I just wanted to make sure this is not a pipeline issue. This is what the configuration file looks like at the moment

      ...
      - run:
          name: Generate Fixtures
          command: scripts/gen-fixtures.sh
      - run:
          name: Check Fixture Diffs
          command: scripts/diff-fixtures.sh
      - run:
          name: Build Fixtures
          command: scripts/build-fixtures.sh

Generate Fixtures expects a Carthage/Build folder to be available but carthage bootstrap is only run during Build Fixtures. Does this need to be changed or am I misunderstanding something?

Thanks!
Rog

[yonaskolb]

Hi @rpassis

XcodeGen previously didn't require that Carthage dependencies had been built before generation, to be a bit more flexible. What has been added here now does in fact require that carthage has built it's dependencies so that the version file is in place. That should be added to the documentation here.
In terms of resolving this issue I would suggest perhaps checking in the Carthage/Build directory and removing the carthage boostrap step from the ci script. What do you think?

[yonaskolb]

@ellneal any comments on this PR as you were thinking of implementing this yourself?

[ellneal]

Looks pretty good to me. I've added some comments, but my opinion is also that it wouldn't hurt to refactor the dependencies(for:) function a bit. Took me a while to get my head around what it was doing.

[rpassis]

Hi @rpassis

XcodeGen previously didn't require that Carthage dependencies had been built before generation, to be a bit more flexible. What has been added here now does in fact require that carthage has built it's dependencies so that the version file is in place. That should be added to the documentation here.
In terms of resolving this issue I would suggest perhaps checking in the Carthage/Build directory and removing the carthage boostrap step from the ci script. What do you think?

This is an interesting one.

We could check in the dependencies but now you have an issue of built frameworks being tied in to a specific Swift toolchain, and the project will break - for example - when we all move to Swift 5.

I thought about just updating the build pipeline so we bootstrap first, then run the generation and diffing. This will work fine in CI but anyone working locally will need to run carthage bootstrap before running tests otherwise there will be diffs in the project file as the dependencies required are not available.

I think option #2 is still a better experience, we will just have to make sure it is well documented.

Thoughts?

[yonaskolb]

That's why I suggested perhaps having a different fixture that uses related dependencies, not the main one that gets built. Otherwise another solution would be to check in the version files and nothing else, but that might mean Carthage thinks they are already built?

[rpassis]

@yonaskolb I'm not sure we should add a different fixture just for testing this feature, seems like too much overhead for something that should technically work off the main project what do you think?

I favour the option of checking in the .version files as you suggested.

Based on my tests with only the .version files in Carthage's build folder, when running carthage bootstrap --cache-builds we will still get the Invalid cache found message from carthage, and the frameworks will be built as expected.

So this will work with the least amount of friction.

I'm going to push these changes up now to make sure it also works in CI.

Once that's passed I'll finish writing a couple of outstanding unit tests for the CarthageDependencyResolver type and tidy up/rebase this commit.

Let me know what you think!

[yonaskolb]

@rpassis sounds good 👍

[rpassis]

This is ready for a final pass, thanks @yonaskolb and @ellneal for the feedback and suggestions!

[yonaskolb]

Looking good @rpassis.

I think it will be useful to have a global option as well to opt in across the whole spec, as that is how it will most likely be used.

The property on dependency would be optional, and would override this global option.
The option could be called includeRelatedCarthageDependencies. That's a pretty long name though. Thoughts?

[rpassis]

Hey @yonaskolb global flag added, let me know if you can see any other issues with the PR.

Also FWIW the diff on Package.resolved here is because that file seems to be out of date in master. I took the liberty to commit the up to date version but let me know if you'd prefer to leave it out.

[yonaskolb]

Fantastic, thanks @rpassis! Though your committed Package.resolved is unnecessary as it includes Carthage and all of its dependencies

[rpassis]

Interesting @yonaskolb, I think I don't understand how SwiftPM works in this regard.

I did add Carthage as a dependency to the Package.swift file a while ago to test the viability of using Carthage with this PR, and then reverted / discarded all the changes.

But every time I run swift test on a clean copy of my local repository, the Package.resolved file gets modified and those dependencies added so I figured it was something that was changed in master and that the resolved file perhaps never got updated.

I just tried swift test again with a brand new clone of the repo and only then everything works as expected i.e. the Package.resolved file is not modified.

I then went back to the copy I was working on and removed the .build artifacts folder, and after that the Package.resolved file stopped getting modified.

Do you know why that happens?

[yonaskolb]

Yeah I think SPM can get into a weird state if you revert changes to the Package.resolved file, as it doesn't match what's in .build.

Thanks again for implementing this @rpassis! 😄
I'm gonna add a couple of doc additions in Usage.md as well