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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support relative paths from current file and the manifest's directory #617

Merged
merged 12 commits into from Oct 31, 2019

Conversation

@pepibumur
Copy link
Contributor

pepibumur commented Oct 29, 2019

Resolves #249

Short description 馃摑

As it was commented on this PR, one of the challenges of reusing code across manifests is how to define paths that are relative to the file being compiled.

This PR provides a solution for that by introducing a new model, Path that users can use to declare paths. The model provides 2 getters, Path.manifestDirectory, & Path.currentFileDirectory to return the previously mentioned directories.

When Tuist loads the manifest by compiling it, it passes an extra argument, --tuist-manifest-dir, that the .manifestDirectory getter users to return the manifest directory. We might introduce a new getter on this PR to return the path to the root directory, typically where the .git directory exists alongside the TuistConfig.swift file and Tuist directory.

@pepibumur pepibumur requested a review from tuist/core Oct 29, 2019
@pepibumur pepibumur self-assigned this Oct 29, 2019
infoPlist: "Info.plist",
sources: "Sources/**",
sources: .paths([Path.currentFileDirectory() + "Sources/**"]),
Comment on lines 9 to 10

This comment has been minimized.

Copy link
@pepibumur

pepibumur Oct 29, 2019

Author Contributor

@tuist/core this is an example of usage

@pepibumur

This comment has been minimized.

Copy link
Contributor Author

pepibumur commented Oct 29, 2019

@tuist/core could you give me some feedback on this. I wonder if there are any rabbit holes that I might be disregarding.

@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Oct 29, 2019

@pepibumur your pull request is missing a changelog!

pepibumur added 2 commits Oct 29, 2019
@kwridan

This comment has been minimized.

Copy link
Contributor

kwridan commented Oct 30, 2019

Nice, I really like the idea of introducing a Path to the manifests to help provide some more semantics around paths! 馃憤

Adding PathKit introduces a large new API surface area to the manifests which may be undesired (e.g. will allow users to cause side effects by manipulating the filesystem - that said, users can already do that via Foundation :p)

We should be able to achieve the same results by adding a lighter weight Path struct to the manifests:

 sources: .paths(["Sources/**"])
 sources: .paths([.relativeToManifest("Sources/**")])
 xcconfig: .relativeToRoot("Configs/Debug.xcconfig")

Boring details:

e.g.

public struct Path: Codable {
    public enum PathType: String, Codable {
        case relativeToCurrentFile
        case relativeToManifest
        case relativeToRoot
    }
    
    public let type: PathType
    public let pathString: String
    public let callerPath: String?
    
    private init(_ pathString: String,
                 type: PathType,
                 callerPath: String? = nil) {
        self.type = type
        self.pathString = pathString
        self.callerPath = callerPath
    }
    
    public static func relativeToCurrentFile(_ pathString: String, callerPath: StaticString = #file) -> Path {
        return Path(pathString, type: .relativeToCurrentFile, callerPath: "\(callerPath)")
    }
    
    public static func relativeToManifest(_ pathString: String) -> Path {
        return Path(pathString, type: .relativeToManifest)
    }
    
    public static func relativeToRoot(_ pathString: String) -> Path {
        return Path(pathString, type: .relativeToRoot)
    }
}

extension Path: ExpressibleByStringLiteral {
    public init(stringLiteral: String) {
        self.init(stringLiteral, type: .relativeToManifest)
    }
}

At load time we should be able to translate those paths accordingly

struct ManifestPaths {
    private let rootPath: AbsolutePath
    private let manifestPath: AbsolutePath
    init(rootPath: AbsolutePath,
         manifestPath: AbsolutePath) {
        self.rootPath = rootPath
        self.manifestPath = manifestPath
    }
    
    func resolve(path: Path) throws -> AbsolutePath {
        switch path.type {
        case .relativeToCurrentFile:
            guard let callerPath = path.callerPath else {
                throw ManifestPathsError.missingCallerPath
            }
            let callerAbsolutePath = AbsolutePath(callerPath).removingLastComponent()
            return AbsolutePath(path.pathString, relativeTo: callerAbsolutePath)
        case .relativeToManifest:
            return AbsolutePath(path.pathString, relativeTo: manifestPath)
        case .relativeToRoot:
            return AbsolutePath(path.pathString, relativeTo: rootPath)
        }
    }
}
class ManifestPathsTests: XCTestCase {
    func test_relativeToManifestPath() throws {
        // Given
        let subject = ManifestPaths(rootPath: "/root", manifestPath: "/root/ProjectA")
        
        // When
        let result = try subject.resolve(path: .relativeToManifest("Sources/Foo.swift"))
        
        // Then
        XCTAssertEqual(result, "/root/ProjectA/Sources/Foo.swift")
    }
    
    func test_relativeToRootPath() throws {
        // Given
        let subject = ManifestPaths(rootPath: "/root", manifestPath: "/root/ProjectA")
        
        // When
        let result = try subject.resolve(path: .relativeToRoot("SharedConfigs/Debug.xcconfig"))
        
        // Then
        XCTAssertEqual(result, "/root/SharedConfigs/Debug.xcconfig")
    }
    
    // Not sure this use case is needed if we have a the notion of a root, but for completeness here is how it could look
    func test_relativeToCurrentFile() throws {
        // Given
        let subject = ManifestPaths(rootPath: "/root", manifestPath: "/root/Modules/FrameworkA")
        
        // When
        let result = try subject.resolve(path: Path("Shared/Constants.swift",
                                                type: .relativeToCurrentFile,
                                                callerPath: "/root/Modules/TuistConfig.swift"))
        
        // Then
        XCTAssertEqual(result, "/root/Modules/Shared/Constants.swift")
    }
}

Hope this is helpful :)

@pepibumur

This comment has been minimized.

Copy link
Contributor Author

pepibumur commented Oct 30, 2019

Adding PathKit introduces a large new API surface area to the manifests which may be undesired (e.g. will allow users to cause side effects by manipulating the filesystem - that said, users can already do that via Foundation :p)

Totally agree here. I copied the implementation from PathKit to prototype the idea and that was one of my concerns. I the API should be simple and limited to what we believe developers need.

We should be able to achieve the same results by adding a lighter weight Path struct to the manifests:

I really like your lightweight proposal and the resulting API 鉂わ笍 .relativeToRoot("Configs/Debug.xcconfig"). I'll update it following your suggestion and push the changes.

Thanks for taking your time to share such a thorough review Kas.

@pepibumur pepibumur marked this pull request as ready for review Oct 31, 2019
Copy link
Contributor

kwridan left a comment

馃憤

This should allow the introduction of a .relativeToRoot with minimal modifications in the near future to support #542

@kwridan

This comment has been minimized.

Copy link
Contributor

kwridan commented Oct 31, 2019

I reckon once .relativeToRoot is introduced it might alleviate the need for .relativeToCurrentFile

The only use cases I see for .relativeToCurrentFile is potentially within TuistConfig as it could reside at arbitrary directory levels that are neither root nor manifest:

e.g.

  • Root
    • Modules
      • TuistConfig.swift
      • FrameworkA
        • Project.swift
@pepibumur

This comment has been minimized.

Copy link
Contributor Author

pepibumur commented Oct 31, 2019

The only use cases I see for .relativeToCurrentFile is potentially within TuistConfig as it could reside at arbitrary directory levels that are neither root nor manifest:

It might be handy too for project description helpers where developers might want to reference files that are in the /Tuist/ directory in the root. Although in that case I think it makes more sense to use the root path.

@pepibumur

This comment has been minimized.

Copy link
Contributor Author

pepibumur commented Oct 31, 2019

Thanks for reviewing the PR @kwridan

@pepibumur pepibumur merged commit ce68185 into master Oct 31, 2019
8 of 12 checks passed
8 of 12 checks passed
Swiftlint
Details
Unit tests
Details
Rubocop
Details
Release build
Details
Changelog
Details
Install
Details
Features
Details
Upload
Details
Header rules Deploy canceled
Details
Mixed content Deploy canceled
Details
Pages changed Deploy canceled
Details
Redirect rules Deploy canceled
Details
@pepibumur pepibumur deleted the manifest-paths branch Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.