Conversation
43b79bf to
ded2b7f
Compare
…ncies don't clone them `cargo` always recursively clones git dependencies, even though c2rust git dependencies don't need any of the big submodules here used just for testing. Setting `update = none` is workaround (see <rust-lang/cargo#10717>). However, other users need to explicitly update submodules now. With this change, it takes ~6 s to resolve a `c2rust-bitfields` git dependency, vs. many minutes before with all of the large submodules.
ded2b7f to
a101fbe
Compare
|
Do we really need this? I feel like it could create other problems, only to optimize a rare download. |
kkysen
left a comment
There was a problem hiding this comment.
Do we really need this? I feel like it could create other problems, only to optimize a rare download.
I'm not sure if we really need this, but git dependencies aren't that rare. A bunch of other crates have done this workaround, too. So IMO it seems useful to me on its own, and didn't use to have this behavior before integrating c2rust-testsuite into this repo, so it's new breakage as well.
|
Unfortunately this is fallout from merging the test suite into the main repo. We did that to improve the development workflow, but it does have the consequence that when using the repo as a git dependency cargo is going to try pull down all the submodules. The cargo docs specifically call out One suggestion to make things a bit nice to work with: Could we update |
Yeah, that could probably work. Also, I realized that |
ahomescu
left a comment
There was a problem hiding this comment.
Could we add a comment somewhere that we're doing this because cargo doesn't let us control whether it updates the submodules?
|
For reference: I think cargo did the wrong thing here, and should have provided a way to download a dependency without submodule. |
Yeah, sure. You want me to add more here: https://github.com/immunant/c2rust/blob/a101fbe53ee2902531bfa6df756e79c792703ea7/.gitmodules?
I agree. Harder to change that, though. Especially since we're stuck on an older nightly. |
|
The cargo issue discussing submodules: rust-lang/cargo#4247 (still open). |
Cargo probably shouldn't be doing this, which is tracked in issue rust-lang/cargo#4247.
|
LGTM (other than wishing cargo let us do it the other way). Handing it over to @randomPoison since there's some more feedback from her (I wish github had gerrit's +1 votes). |
c2rust-side fix for the git dependency version of Avoid /opt path dependency for c2rust-bitfields GaloisInc/Tractor-Crisp#46.cargoalways recursively clones git dependencies, even though c2rust git dependencies don't need any of the big submodules here used just for testing. Settingupdate = noneis workaround (see rust-lang/cargo#10717). However, other users need to explicitly update submodules now.With this change, it takes ~6 s to resolve a
c2rust-bitfieldsgit dependency, vs. many minutes before with all of the large submodules.