search

yonaskolb / Mint

A package manager that installs and runs executable Swift packages
MIT License
2.27k stars 122 forks source link

Installer cannot resolve gitPath using shortened GitHub repo name if repo has a period in its name #153

Closed liamnichols closed 4 years ago

liamnichols commented 4 years ago

I'm trying to integrate the R.swift project into my Mintfile (https://github.com/mac-cain13/R.swift/issues/593) and I've done so using the shorthand syntax like this:

liamnichols/R.swift@ln/build-rule # (using a fork for a bugfix)

However upon trying to install via mint bootstrap I get the following: 

mint bootstrap
🌱  Found 2 packages in Mintfile
🌱  SwiftLint 0.38.0 already installed
🌱  Cloning R ln/build-rule
Cloning into 'liamnichols_R.swift'...
fatal: unable to access 'https://liamnichols/R.swift.git/': Could not resolve host: liamnichols
🌱  Encountered error during "git clone --depth 1 -b ln/build-rule https://liamnichols/R.swift.git liamnichols_R.swift". Use --verbose to see full output
🌱  Couldn't clone https://liamnichols/R.swift.git ln/build-rule

After digging through the source, it looks like there is some logic in the gitPath computed property to detect when the user specifies a custom non-github.com hostname without a url scheme (i.e mycustomdomain.com/package) and this was done by checking for a . in any part of the string.

I'm not sure if this is the right approach since it seems a bit fragile, but I was able to work around the issue by checking that the first component of the repo string separated by / contains the period instead of the entire string. I've added an example and the tests verify that it hasn't caused any known regressions.

I feel like there might be a nicer way to resolve this bug however I didn't want to spend too much time on it since I don't know exactly what are the valid formats. I raised this PR as a starting point but am happy to make further changes with some help 🙂

Additionally, I also noticed that in the console log the name would also be printed incorrectly:

🌱  Cloning R ln/build-rule

My second commit patches this by doing .replacingOccurrences(of: ".git", with: "") instead of components(separatedBy: ".").first!. I don't think there are any other intended path components so this should be ok?

Thanks for all the great work on this! Let me know if I can do anything else to help with this issue 🙏

yonaskolb commented 4 years ago

Fantastic, thanks for the PR @liamnichols! Looks good to me. Could you add a changelog entry as well and then we can get this merged

liamnichols commented 4 years ago

Thanks @yonaskolb, I've updated the changelog for a 0.14.0 release with a description of this fix.. Hope that's alright 👍