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

Allow git dependency with shorthand ssh submodules to work. #7238

Merged
merged 1 commit into from
Aug 12, 2019

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Aug 12, 2019

If a submodule is defined with a shorthand ssh url (like git@github.com/user/repo.git), then cargo was choking on it trying to convert it to a URL. The fix is to just pass around strings.

An alternate solution would be to try to detect shorthand git urls and automatically add ssh:// to the path. I'm concerned about matching git's heuristics for this, though. I'm willing to try if you think this would be better, though.

I can't think of a good way to write a test for this, since we don't have any SSH test infrastructure. I verified running locally against github.

Closes #7202

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 12, 2019
@alexcrichton
Copy link
Member

@bors: r+

Yeah this seems fine to verify manually, no worries on the test!

@bors
Copy link
Contributor

bors commented Aug 12, 2019

📌 Commit 0400879 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 12, 2019
@bors
Copy link
Contributor

bors commented Aug 12, 2019

⌛ Testing commit 0400879 with merge 130e11c...

bors added a commit that referenced this pull request Aug 12, 2019
Allow git dependency with shorthand ssh submodules to work.

If a submodule is defined with a shorthand ssh url (like `git@github.com/user/repo.git`), then cargo was choking on it trying to convert it to a URL. The fix is to just pass around strings.

An alternate solution would be to try to detect shorthand git urls and automatically add `ssh://` to the path. I'm concerned about matching git's heuristics for this, though. I'm willing to try if you think this would be better, though.

I can't think of a good way to write a test for this, since we don't have any SSH test infrastructure. I verified running locally against github.

Closes #7202
@bors
Copy link
Contributor

bors commented Aug 12, 2019

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 130e11c to master...

@bors bors merged commit 0400879 into rust-lang:master Aug 12, 2019
Veetaha added a commit to Veetaha/cargo-generate that referenced this pull request Aug 1, 2020
This is a small breaking change in cargo, because they no longer try to
normalize urls: rust-lang/cargo#7238
@Veetaha
Copy link

Veetaha commented Aug 1, 2020

Hey guys, I've just spent a decent amount of time debugging both 0.46.1 and 0.30 versions of cargo in this PR to cargo-generate: cargo-generate/cargo-generate#237 just to understand that cargo no longer tolerates git submodules with file:// scheme which have 2 slashes after file: but not 3 (as this PR removes normalization of the git submodule URL)

This is a small breaking change (since double slashes with no host are non-standard), but I just want to notify you about this fact.
Is there someplace where you put changelogs for cargo as a library on crates.io? Because I haven't found it and had to go thru the hard part finding out this breaking change.
cc @ehuss , @alexcrichton
And sorry for bringing up an old PR...

@alexcrichton
Copy link
Member

@Veetaha this is almost a year old so if it's causing issues it's probably best to open a fresh issue with fresh details to track anything necessary

Veetaha added a commit to Veetaha/cargo-generate that referenced this pull request Sep 12, 2020
This is a small breaking change in cargo, because they no longer try to
normalize urls: rust-lang/cargo#7238
Veetaha added a commit to Veetaha/cargo-generate that referenced this pull request Sep 12, 2020
This is a small breaking change in cargo, because they no longer try to
normalize urls: rust-lang/cargo#7238
@ehuss ehuss added this to the 1.39.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

git_fetch_with_cli does not fetch submodules with cli
5 participants