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

Support multiple frameworks in archive command #1005

Merged
merged 11 commits into from Jan 18, 2016

Conversation

ikesyo
Copy link
Member

@ikesyo ikesyo commented Dec 18, 2015

This would be useful for some libraries, for example, which have a Objective-C framework and the corresponding Swift one (e.g. https://github.com/CocoaLumberjack/CocoaLumberjack).

carthage archive CocoaLumberjack CocoaLumberjackSwift

@ikesyo
Copy link
Member Author

ikesyo commented Dec 18, 2015

Or should we remove this line and recommend to attach multiple .framework.zip files to their GitHub releases as CocoaLumberjack currently does?

@NachoSoto
Copy link
Contributor

Yeah, I think I'd rather keep this simple and encourage running carthage archive separately for each framework.

@mdiep
Copy link
Member

mdiep commented Dec 18, 2015

I would be 👍 with this, post #892 especially. It makes sense for the .zip to contain whatever would get built if you weren't using binaries.

After #892, maybe we could make the arguments to archive optional—defaulting to archiving all the frameworks from the current project.

@ikesyo
Copy link
Member Author

ikesyo commented Dec 22, 2015

defaulting to archiving all the frameworks from the current project

Does this mean the frameworks built by --no-skip-current or all the frameworks in Carthage/Build?

@mdiep
Copy link
Member

mdiep commented Dec 22, 2015

All the frameworks in the current project (--no-skip-current), excluding dependencies.

@ikesyo
Copy link
Member Author

ikesyo commented Dec 23, 2015

I got it, thanks!

This would be useful for some libraries, for example, which have a Objective-C framework and the corresponding Swift one (e.g. https://github.com/CocoaLumberjack/CocoaLumberjack).
@ikesyo ikesyo force-pushed the archive-multiple-frameworks branch from 05c0ead to 8fcdd1f Compare January 14, 2016 14:24
@ikesyo
Copy link
Member Author

ikesyo commented Jan 14, 2016

@mdiep I've rebased and implemented #1005 (comment). ✨

@mdiep
Copy link
Member

mdiep commented Jan 14, 2016

I hope to catch up on Carthage PRs over the next few days.

@@ -61,10 +94,11 @@ public struct ArchiveCommand: CommandType {
.collect()
.flatMap(.Merge) { paths -> SignalProducer<(), CarthageError> in
if paths.isEmpty {
return SignalProducer(error: CarthageError.InvalidArgument(description: "Could not find any copies of \(options.frameworkName).framework. Make sure you're in the project’s root and that the framework has already been built using 'carthage build --no-skip-current'."))
let error = CarthageError.InvalidArgument(description: "Could not find any copies of \(frameworks). Make sure you're in the project’s root and that the frameworks has already been built using 'carthage build --no-skip-current'.")
Copy link
Member

Choose a reason for hiding this comment

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

This should read that the frameworks have already. ☺️

Copy link
Member

Choose a reason for hiding this comment

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

Also, it'd be nice to join the frameworks with commas so we don't get the []s in the output.

@mdiep
Copy link
Member

mdiep commented Jan 15, 2016

This looks mostly correct, but I left a found a couple things that should be fixed and I think we ought to clean up the implementation a little.

Thanks for doing this! It'll be a nice touch.

@ikesyo
Copy link
Member Author

ikesyo commented Jan 16, 2016

@mdiep Thank you for the review, I've addressed them! ⚡ (diff with white space ignored)

Could you please re-review? 🙏

@@ -268,6 +268,62 @@ public func schemesInProject(project: ProjectLocator) -> SignalProducer<String,
.map { (line: String) -> String in line.stringByTrimmingCharactersInSet(NSCharacterSet.whitespaceCharacterSet()) }
}

/// Finds schemes of projects or workspace, which Carthage should build, found
/// within the given directory.
public func buildableSchemesByProjectLocatorInDirectory(directoryURL: NSURL, withConfiguration configuration: String, forPlatforms platforms: Set<Platform> = []) -> SignalProducer<(ProjectLocator, [String]), CarthageError> {
Copy link
Member

Choose a reason for hiding this comment

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

Could we call this buildableSchemesInDirectory?

@mdiep
Copy link
Member

mdiep commented Jan 17, 2016

Looks good apart from the function names. 👍

@ikesyo
Copy link
Member Author

ikesyo commented Jan 18, 2016

Thanks for the feedbacks. ✨

@mdiep
Copy link
Member

mdiep commented Jan 18, 2016

mdiep added a commit that referenced this pull request Jan 18, 2016
Support multiple frameworks in `archive` command
@mdiep mdiep merged commit 1249802 into master Jan 18, 2016
@mdiep mdiep deleted the archive-multiple-frameworks branch January 18, 2016 01:41
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