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

Chimera - fetch carthage dependencies. #2060

Merged
merged 89 commits into from
Dec 29, 2020
Merged

Chimera - fetch carthage dependencies. #2060

merged 89 commits into from
Dec 29, 2020

Conversation

laxmorek
Copy link
Collaborator

@laxmorek laxmorek commented Nov 22, 2020

Short description 📝

Working on #1674

Adds possibility to install carthage dependencies defined in Tuist/Dependencies.swift using tuist dependencies fetch and tuist dependencies update commands.

Comments 🗒

  • In .gitignore of app_with_framework_and_tests_and_dependencies I added Tuist/Dependencies/*. I don't want to track generated files.
  • Frameworks are rebuilding with every run of tuist dependencies *. Im going to fix it in the next PR. I have done it
  • I didn't add documentation and changelog changes. Feature isn't production ready.
  • Let me know if documentation in the code is understandable. Please suggest me improvements if any need.

Implementation 👩‍💻👨‍💻

  • Implement tuist dependencies fetch/update for carthage
  • Add unit tests
  • Add new fixture app_with_framework_and_tests_and_dependencies
  • Add acceptance tests

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Hey @laxmorek. Despite all my comments, I think the work is going in the right direction and the work is very aligned with how we do things in the project. Congratulations fur such a great contribution 👏

@laxmorek
Copy link
Collaborator Author

@pepibumur Thank you for your review. I adjusted PR to your suggestion. Could you revisit it?

@ferologics
Copy link
Contributor

Reminder about the Carthage --use-netrc flag. It's essential when installing dependencies such as MapBox:

Install the SDK with carthage update --platform iOS --use-netrc
Always use the --use-netrc flag when running carthage bootstrap

Comment on lines 30 to 35
public struct DependenciesDirectory {
public static let name = "Dependencies"
public static let graphName = "graph.json"
public static let lockfilesDirectoryName = "Lockfiles"
public static let cartfileResolvedName = "Cartfile.resolved"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we convert it to an enum? Since everything is static, we should avoid people going DependenciesDirectory()

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that everything is a struct though 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have followed convention in this file.

Constants.Vendor, Constants.DerivedDirectory etc

Im agree with you but I don't want to introduce a mess here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically we should convert every struct in this file to enum. Should I do it in this PR? 🤔

@laxmorek
Copy link
Collaborator Author

laxmorek commented Dec 6, 2020

Update 🙂

Dependencies.swift

From now dependency's requirements depends on dependency manager. There is also no longer common Dependency model. I created CarthageDependency and in the future we will provide CocoaPodDependency and SwiftPackageManagerDependency. API of Dependencies.swift has been changed.

Before:

import ProjectDescription

let dependencies = Dependencies([
    .carthage(name: "Alamofire/Alamofire", requirement: .exact("5.0.4"), platforms: [.macOS]),
    .carthage(name: "Swinject/Swinject", requirement: .exact("2.7.1"), platforms: [.macOS]),
])

Now:

import ProjectDescription

let dependencies = Dependencies(
    carthage: [
        .init(name: "Alamofire/Alamofire", requirement: .exact("5.0.4"), platforms: [.macOS]),
        .init(name: "Swinject/Swinject", requirement: .exact("2.7.1"), platforms: [.macOS]),
    ]
)

Tuist/Dependencies directory

New structure of Tuist/Dependencies directory:

Tuist
  |- Dependencies
    |- graph.json # not part of this PR 
    |- Lockfiles
      |- Carthage.resolved
      |- Podfile.lock # not part of this PR 
      |- Package.resolved # not part of this PR 
    |- Carthage
      |- Build
      |- Alamofire
        |- iOS
          |- Alamofire.framework
        |- tvOS
          |- Alamofire.framework
    |- Cocoapods # not part of this PR 
      |- RxSwift
    |- SwftPackageManager # not part of this PR 
      |- Moya

Frameworks handling

  • I added a logic that removes frameworks from Tuist/Dependencies` if they are no longer needed.
  • I added a logic that prevents rebuilding of frameworks.

Carthage command

  • I added --use-netrc flag for carthage bootstrap and carthage update.

Others

  • I updated changelog
  • I didn't update documentation

Copy link
Collaborator

@luispadron luispadron left a comment

Choose a reason for hiding this comment

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

I haven't taken an in-depth look at the code since I lack the context here to make that useful but looking at the public API this feature is looking really nice!

Thanks for making the API more explicit and less possible to hit errors!


@testable import ProjectDescription

final class DependencyTests: XCTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should inherit from TuistUnitTestCase


@testable import ProjectDescription

final class DependenciesTests: XCTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should inherit from TuistUnitTestCase

@testable import TuistCore
@testable import TuistSupportTesting

final class CarthageDependencyTests: TuistTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a unit test, it should inherit from TuistUnitTestCase.

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Brilliant work @laxmorek. I think we are ready to merge here after addressing the final comments.

@pepicrft pepicrft merged commit 56a72d6 into main Dec 29, 2020
@pepicrft pepicrft deleted the chimera/carthage branch December 29, 2020 12:00
@laxmorek laxmorek changed the title Chimera - carthage support. Chimera - carthage - update support. Jan 1, 2021
@laxmorek laxmorek changed the title Chimera - carthage - update support. Chimera - fetch carthage dependencies. Jan 1, 2021
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

6 participants