-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add support for relative git submodule paths #9592
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
This looks great! Can you maybe change the test so that [package]
name = "base"
version = "0.1.0"
[dependencies]
deployment.path = "deployment" |
Oh, and btw, it is usually best to use a descriptive title for a PR. Just mentioned an issue number doesn't have any context (especially when looking at a git log, or a list of PRs), and isn't clickable in the github UI. Also, to link an issue to a PR requires special keywords in the description (I edited it to add that). |
@ehuss CI seems to be failing on windows, and I don't have easy access to a windows machine, so might take a few days for me to confirm fixes to those CI failures. |
b9193ca
to
8511655
Compare
8511655
to
9355b7e
Compare
9355b7e
to
4cae394
Compare
4b9dfa7
to
a194bfc
Compare
a194bfc
to
552dca1
Compare
Really like to see this working. |
Yeah sorry about the time this is taking. I likely need to get a windows vm or something to debug this and its not currently a priority, so could take a while. |
I have a regular windows laptop, if you can guide me, I am willing to help. |
☔ The latest upstream changes (presumably #10296) made this pull request unmergeable. Please resolve the merge conflicts. |
Closing due to inactivity. If someone wants to pick this back up, and has a Windows system to resolve the issues, feel free to open a new PR. |
Add support for relative git submodule paths Fixes #7992. This is a continuation of #9592, tested on Linux and Windows. > Git allows submodules to have relative URLs, but cargo does not handle this correctly, and simply fails to update submodules as described in issue #7992. This PR fixes that by passing in the parent git repo url to update_submodules. The previous PR wasn't compatible on Windows, since it used [`std::path::Path`](https://doc.rust-lang.org/std/path/struct.Path.html) for merging the parent and relative submodule urls. This PR uses [`url::Url`](https://docs.rs/url/latest/url/struct.Url.html) instead, which has the same behavior on all platforms. It also refers to the [git documentation](https://git-scm.com/docs/git-submodule#Documentation/git-submodule.txt-add-bltbranchgt-f--force--nameltnamegt--referenceltrepositorygt--depthltdepthgt--ltrepositorygtltpathgt) for the definition of a relative git submodule path.
GIt allows submodules to have relative URLs, but cargo does not handle this correctly, and simply fails to update submodules as described in issue #7992. This PR fixes that by passing in the parent git repo url to update_submodules.
Closes #7992