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

Don't lock transitive dependencies using submodules #87

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sharplet
Copy link
Contributor

When Carthage checks out Swish into a project, it also recursively copies its submodules. This means that even if Carthage finds a more recent version of Result or Argo, when Swish is built it will always link against the version in the submodule.

If the app and Swish are linked against two different versions of the same dependency, the app may crash on launch when looking up symbols.

This change now ignores Cartfile.resolved, and stops using submodules, so that Swish can dynamically be linked against any compatible version that the app uses, rather than a specific submodule revision.

When Carthage checks out Swish into a project, it also recursively
copies its submodules. This means that even if Carthage finds a more
recent version of Result or Argo, when Swish is built it will always
link against the version in the submodule.

If the app and Swish are linked against two different versions of the
same dependency, the app may crash on launch when looking up symbols.

This change now ignores Cartfile.resolved, and stops using submodules,
so that Swish can dynamically be linked against any compatible version
that the app uses, rather than a specific submodule revision.
@sharplet
Copy link
Contributor Author

Here's the linker error I was seeing:

  • App is using the current version of Result, 2.1.0
  • Swish has 2.0.0 checked out via submodule
  • The two releases are source compatible, but the >>- operator has moved from Result to the ResultType protocol
  • When building, Swish is linked against 2.0.0 but 2.1.0 is what's embedded in the app
  • At load time, Swish is expecting >>- to be defined on Result, but it's actually on the protocol. From a runtime perspective this wouldn't be a problem (because the type still ultimately adopts the protocol and thus has access to that operator), but things are screwed up. I have no idea if this would be considered a compiler bug, but it might be worth raising an issue for it.

@gfontenot
Copy link
Collaborator

I'm generally in favor of removing the submodules, but I believe this might break the build for people using git submodules themselves as a dependency management solution? If it doesn't, we'll almost certainly need to update the submodule instructions to match the new reality. Can we confirm one way or another before moving forward with this?

@sharplet
Copy link
Contributor Author

...but I believe this might break the build for people using git submodules themselves as a dependency management solution?

Right, that's something I hadn't thought of—the issue being that for non-Carthage users, transitive dependencies are no longer automatically included?

I believe that a manual, submodules-only approach should still be possible the same way Carthage manages it for you: add each transitive dependency to your project as a submodule, using an Xcode workspace to build all the dependencies together. The difference is that previously a recursive submodule update would check out the transitive deps for you.

I think I have an idea for an alternative solution: what if we could either a) not check in Swish's workspace file, or b) move it out of the way so that Carthage only uses the Swish.xcodeproj to build? That way the submodules could still be a part of the project tree, but would never be seen by a Carthage build.

Hope that makes sense 😅

@sharplet
Copy link
Contributor Author

FWIW: this is currently breaking a project of mine, but I can think of a less invasive workaround to this problem. If we updated Result to 2.1.0 and then put out a patch release, the issue would disappear for now.

@sharplet sharplet mentioned this pull request Jun 20, 2016
sharplet added 2 commits July 28, 2016 16:30
Carthage always prefers workspaces of projects, but the workspace
dependencies won't compile in a `carthage build` without using
submodules, because carthage checkouts don't share checkouts of
transitive dependencies.
@@ -29,7 +29,8 @@ build/
Pods

# Carthage
Carthage/Build
Carthage/
Cartfile.resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should be ignoring this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm uncertain myself, but thought it probably made sense to ignore it for the same reason you ignore Gemfile.lock when writing a rubygem.

@gfontenot
Copy link
Collaborator

Why get rid of the workspace?

@gfontenot
Copy link
Collaborator

I think that I'm 👍 on this if the only real change is removing submodules.

@sharplet
Copy link
Contributor Author

@gfontenot I was still having issues trying to resolve this problem today and these latest commits were my attempts to get there. To summarise:

  1. The issue with the workspace is a) with the submodules removed, the sources for transitive dependencies are no longer checked out; b) Carthage always favours building from a workspace over a project; and c) building from the workspace was failing because the dependent projects weren't checked out.
  2. After deleting the workspace to try coax Carthage into building the project instead (using the shared, prebuilt binaries from the symlinked Carthage/Build directory), I ran into the old chestnut of Quick & Nimble failing to build for tvOS.

So I'm kind of confused at this point. I ended up working around the linker error in my project by replacing Carthage binaries with workspace dependencies and letting Xcode build everything itself at the same time.

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.

2 participants