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

Automatically checkout dependencies' submodules #69

Merged
merged 12 commits into from
Nov 16, 2014

Conversation

jspahrsummers
Copy link
Member

Resolves #67.

To do:

  • Clone all submodules upon checkout

@jspahrsummers jspahrsummers added this to the Initial Release milestone Nov 14, 2014
@jspahrsummers jspahrsummers changed the title [WIP] Automatically checkout submodules Automatically checkout submodules Nov 15, 2014
@jspahrsummers
Copy link
Member Author

💣

@jspahrsummers jspahrsummers changed the title Automatically checkout submodules Automatically checkout dependencies submodules Nov 15, 2014
@jspahrsummers jspahrsummers changed the title Automatically checkout dependencies submodules Automatically checkout dependencies' submodules Nov 15, 2014
@swizzlr
Copy link
Contributor

swizzlr commented Nov 16, 2014

Using the tip of this branch, I discovered that the dependencies' dependencies are not necessarily being checked out at the appropriate revision.

I wish I could be more helpful in this instance as to what's going on, but I'll have to give you steps to reproduce and go from there:

Cartfile:

github "AshFurrow/Moya"

Cartfile.lock:

github "AshFurrow/Moya" "0.5"

It clones Moya, and clones Moya's dependency on RAC. However, Moya@0.5 depends on
RAC@3b90e0c687.

Note that in the ReactiveCocoa/ folder lies Action.swift and other files from the Swiftening.

But in my repo, find . -name 'Action.swift' comes up empty.

The smoking gun in this was that LlamaKit was not checked out, which causes carthage build to die pretty early on.

@jspahrsummers
Copy link
Member Author

@swizzlr Huh, thanks for finding that and digging into it. I can reproduce it, and I have no idea what's happening yet.

@jspahrsummers jspahrsummers changed the title Automatically checkout dependencies' submodules [WIP] Automatically checkout dependencies' submodules Nov 16, 2014
@jspahrsummers
Copy link
Member Author

Well, that was a really dumb mistake. Fixed in 4ba4ec5, so this is again ready for review.

Thanks again, @swizzlr!

@jspahrsummers jspahrsummers changed the title [WIP] Automatically checkout dependencies' submodules Automatically checkout dependencies' submodules Nov 16, 2014
@jspahrsummers
Copy link
Member Author

These intertwined pull requests are driving me crazy. Gonna merge this, but plz to review still. 🙏

jspahrsummers added a commit that referenced this pull request Nov 16, 2014
Automatically checkout dependencies' submodules
@jspahrsummers jspahrsummers merged commit 1e8b0c2 into master Nov 16, 2014
@jspahrsummers jspahrsummers deleted the auto-checkout-submodules branch November 16, 2014 06:51
return launchGitTask([ "clone", "--quiet", submodule.URLString, submodule.path ], repositoryFileURL: workingDirectoryURL)
.then(launchGitTask([ "checkout", "--quiet", submodule.SHA ], repositoryFileURL: submoduleDirectoryURL))
// Clone nested submodules in a separate step, to quiet its output correctly.
.then(launchGitTask([ "submodule", "--quiet", "update", "--init", "--recursive" ], repositoryFileURL: submoduleDirectoryURL))
Copy link
Member

Choose a reason for hiding this comment

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

Should we be doing all these with --quiet? Maybe you just aren't capturing this output yet, but it seems like we want to save all of it.

Choose a reason for hiding this comment

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

+1. This should all be sent to some sort of reporter object (see #63). The reporter object should distinguish between Carthage and Git logs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, ideally we would yield signal events instead of using a reporter.

However, in this specific case, there's nothing to be gained from the normal output logging of these commands. Any errors will be printed no matter what, and to a different output stream.

@mdiep
Copy link
Member

mdiep commented Nov 16, 2014

These intertwined pull requests are driving me crazy. Gonna merge this, but plz to review still

It's hard to review with all the commits from earlier PRs, so I was waiting until the dependency was merged. 🙇

natanrolnik pushed a commit to natanrolnik/Carthage that referenced this pull request Feb 2, 2017
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.

Submodules of dependencies aren't checked out correctly
4 participants