Import local packages about swift-sh HOT 11 CLOSED

PR welcome

from swift-sh.

Cool, sounds great!

I will see what I can figure out and report back :).

from swift-sh.

SwiftPM supports local dependencies so can be done. I think the issue will be the syntax to ensure there is no conflict with the form: import Foo // bar/baz we may need to change this to be @bar/baz which requires a breaking change.

from swift-sh.

Yeah, I went into the swift-sh cache and manually edited the Package.swift to make sure my local dependency worked.

As far as the syntax goes, good point.

• Is there any desire to prevent that breaking change?
• Maybe we could relying on a prefix like ~ or local:?
  (I am not sure how I feel about this yet though)

from swift-sh.

“Is there any desire to prevent that breaking change?”

Ideally, but I'm not sure it will be possible without having it do some kind of check if a directory exists which means our behavior is flakey depending on your directory layouts.

from swift-sh.

@mxcl After looking into this a bit, I would like your thoughts on the following:

I can move the package url logic from ImportSpecification to parse().swift and do something like:

    if depSpec.hasPrefix("@") {
        let depName = String(depSpec.dropFirst())
        if let url = URL(string: depName), url.scheme != nil {
            depUrlString = depName
        } else {
            depUrlString = """
            https://github.com/\(depName + "/" + importName).git
            """
        }
    } else {
        depUrlString = String(depSpec)
    }

This would replace the dependencyName property in ImportSpecification with a different property like dependencyUrlString, and var packageLine: String would just use the dependencyUrlString (the result from the above change) for the package url. This is assuming the current dependencyName property is not (or will not be) needed elsewhere.

Another potential solution would be to create a new DependencySource enum (e.g., github, local, etc.) that has the dependencyName as an associated value, which would replace the dependencyName property in ImportSpecification. This would then be used to switch over in var packageLine: String to produce the package url. This would probably be a bit cleaner, but more involved. I am open to doing this if there is a benefit to keeping the dependencyName property around.

Both solutions above would have @ signify a GitHub dependency, which would be a breaking change as you said.

Thoughts?

from swift-sh.

Going through this code I have to apologize, it's not great. Was a quick project TBH.

Yeah, as you concluded ImportSpecification will need to change, which is fine. Seems like an enum to replace dependencyName would do the trick:

enum Dependency {
    case remote(URL)
    case local(Path)
}

Even though most people would (probably) specify local paths relatively, we need the full path since we write the Package.swift elsewhere, which is a pity (since paths change), but not much can be done.

Then change the property name to let dependency: Dependency I guess.

Both solutions above would have @ signify a GitHub dependency, which would be a breaking change as you said.

I think we will do a gradual roll out. For now we should check if a package exists at that path and if so, use .local, otherwise behave like it does now (GitHub URL) but output a warning telling them to stick the @ at the front.

We shouldn’t break scripts for everyone instantly if possible.

from swift-sh.

Hey, just wanted to give a quick update.

I made some progress changing dependencyName over to a new Dependency enum to accommodate this change, but paused to see the fallout from the private repos issue: #77 (great stuff btw!).

I have to fully catch up and do some rework where necessary, but I am very much still working on this :).

from swift-sh.

Yeah, sorry, I was thinking of you while I was doing it, unfortunate timing, but you can more easily integrate this now I should think.

from swift-sh.

Haha, no worries! And yeah I do think my work will fit in better now — nice cleanup PR.

from swift-sh.

Just submitted PR #81 to resolve this issue. Let me know what you think!

from swift-sh.