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

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

Merged
merged 3 commits into from
Jan 5, 2020

Conversation

liamnichols
Copy link
Contributor

I'm trying to integrate the R.swift project into my Mintfile (mac-cain13/R.swift#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-git.luolix.top 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 🙏

@liamnichols liamnichols force-pushed the ln/github-repo-name-bug branch from aaa3c86 to 5cc9cbd Compare December 24, 2019 12:06
@liamnichols liamnichols force-pushed the ln/github-repo-name-bug branch from 5cc9cbd to 9f4e2a6 Compare December 24, 2019 12:07
@yonaskolb
Copy link
Owner

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
Copy link
Contributor Author

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

@yonaskolb yonaskolb merged commit c00436f into yonaskolb:master Jan 5, 2020
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.

2 participants