refactor(cargo-shuttle): remove cargo-generate dependency#1281
refactor(cargo-shuttle): remove cargo-generate dependency#1281jonaro00 merged 8 commits intoshuttle-hq:mainfrom
cargo-generate dependency#1281Conversation
Replaces network-based (e.g. clonging) features of git2 with those provided by gix. Thereby rustls can be used instead of openssl.
The purpose of the removed configuration was to prevent test crashes in CI, which were caused by the inner workings of `cargo-generate` (see the deleted `CI DEBUG` comment for more info). Thus, they can now be deleted.
|
/claim #1269 |
jonaro00
left a comment
There was a problem hiding this comment.
Looking great! Clean code.
Other things that can be added:
- Use gix instead of git2 for dirty check
- Allow passing args or syntax for specifying a branch/tag/commit to clone from
Up to you if you want to add them :)
|
Yes, I want to fully replace |
cargo-shuttle/src/init.rs
Outdated
| // dot pattern. This way, conflicts between relative path in the form | ||
| // `foo/bar` and repository name in the form `owner/name` are avoided. | ||
| match loc.subfolder { | ||
| Some(subfolder) => Ok(Self::LocalPath(Path::new(&loc.auto_path).join(subfolder))), |
There was a problem hiding this comment.
Just a thought: if we are to add branch/rev selection to the clone operation, we would need to require a local path to be handled like the other alternatives, so that the clone and checkout can be done correctly.
There was a problem hiding this comment.
Do you mean in cases where there is a Git repository at the local path? Yes, that's a good idea.
The generate function ought to be divided into a setup which ends in the same generic state, no matter what the initial form of the origin was. This state could be the raw template places in a temporary directory. After the setup, the rest of the code would be the same for local folders and repository clones. This code always uses subfolder.
I actually prefer this design over the current one. It separates the different stages and the information required at each one of them much better.
There was a problem hiding this comment.
Yeah it's a bit more streamlined.
I wonder if the depth 1 and potential branch/rev parameter clash in some way 🤔
* Add a prefix to the temporary directories used. * Clone template repositories with a depth of 1 commit. * Check the output of a few of the test cases for the parser.
38253e2 to
8fd999f
Compare
The different parts of generating the template and their use of the arguments passed to `generate_project` are now divided more clearly. This decreases the size and overhead of the implementation. The tests checking the paring are also gone. Instead, the cloning and the copying of templates is tested. The integration tests should be sufficient for ensuring that the paring in this implementation remains correct.
8fd999f to
3ac2dee
Compare
|
By replacing Compile times have improved compared to the previous implementation that uses At the current state of this PR, it takes an average 76.28 s to do a clean build of |
|
Splendid! I'll check out the changes later. Is this PR "done" now? |
|
I'm still figuring out how a So yes, besides the |
There was a problem hiding this comment.
Wow, gix was a bit tricky it seems.
I tried a local deploy, but the commit ID was malformed. Maybe the hash you get from gix is the binary data, not encoded to a hex string?
Also, the dirty flag did not turn true when having untracked files or staged files, only when having a removed (changed) file. We don't have strict requirements for these, but the former two seem like reasonable triggers, since they cause non-commited files to get deployed.
On my 20 thread CPU.
cargo b -r -p cargo-shuttlecargo cleanbetween every build
| test | main | pr 1281 |
|---|---|---|
| first test | 58 s | 54 s (should have been less due to partially missing crate downloads and sccache) |
| hot sccache (wow!!!) | 15 s | 13 s |
| registry cached, no sccache | 60 s | 56 s (ignore the above I guess) |
Haha I guess it don't matter much on such a powerful CPU. Not sure how fair my tests were either. The improvement will be bigger for weaker CPUs tho :D
cargo-shuttle/src/lib.rs
Outdated
| /// It's adapted from the `gitoxide` test suite: https://github.com/ | ||
| /// Byron/gitoxide/blob/3d60c0245ec4b787cfcb111319d730a6e5031ef4/ | ||
| /// gix-status/tests/status/index_as_worktree.rs#L259 |
There was a problem hiding this comment.
I would personally not linebreak URLs :D
|
What are the specific commands you used to get the image you linked? I'd like to know, so I can debug the issue. I'll also take a look at how to determine the strictness of the dirty check. |
|
I just found the tracking issue on I guess a reasonable way to go would be to remove my last commit and keep using |
|
Oh no :( If you want, you can move the last commit to a new draft PR, so that the work done can be reused if gix becomes more feature complete. |
0056ff8 to
aa9da60
Compare
Description of change
cargo-generatehas been removed as a dependency ofcargo-shuttle, and all functionality ofcargo-shuttlethat was used by thecargo shuttle initcommand has been re-implemented.The
openssldependency introduced by cloning the template Git repositories using thegit2crate has also been removed. Instead,gixis used to clone repositories, which usesrustls.Closes #1269
/claim #1269
How has this been tested? (if applicable)
The applicable tests have been updated where required. Additional tests have been added to ensure that the new parsing-logic accepts many relevant examples (e.g. those in the
shuttle-examplesREADME. Also, some previously ignored tests now pass.