refactor(cargo-shuttle): remove `cargo-generate` dependency #1281

thass0 · 2023-09-28T15:28:24Z

Description of change

cargo-generate has been removed as a dependency of cargo-shuttle, and all functionality of cargo-shuttle that was used by the cargo shuttle init command has been re-implemented.

The openssl dependency introduced by cloning the template Git repositories using the git2 crate has also been removed. Instead, gix is used to clone repositories, which uses rustls.

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-examples README. Also, some previously ignored tests now pass.

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.

thass0 · 2023-09-28T16:14:04Z

/claim #1269

jonaro00

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 :)

cargo-shuttle/src/init.rs

thass0 · 2023-09-28T16:18:32Z

Yes, I want to fully replace git2 with gix. I am still working on the dirty check.

cargo-shuttle/src/init.rs

jonaro00 · 2023-09-28T17:17:21Z

cargo-shuttle/src/init.rs

+            // 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))),


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.

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.

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.

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.

cargo-shuttle/src/init.rs

thass0 · 2023-09-30T12:36:39Z

By replacing git2 with gix, the number of crates used has gone up again. gix is very modular and divided up into many small crates, so that users can choose which features they need very precisely.

Compile times have improved compared to the previous implementation that uses cargo-generate and git2 with openssl.

At the current state of this PR, it takes an average 76.28 s to do a clean build of cargo-shuttle. In comparison, the version of cargo-shuttle that's currently in use compiles in an average of 103.37 s. That's a 1.36x speedup!

jonaro00 · 2023-09-30T12:54:20Z

Splendid! I'll check out the changes later. Is this PR "done" now?

thass0 · 2023-09-30T12:59:32Z

I'm still figuring out how a --rev flag would best be implemented using gix. Since adding the --rev flag as a whole also requires changes all over cargo-shuttle, I suggest to put it in another issue/PR.

So yes, besides the --rev flag, this PR should now close everything discussed in #1269 😀

jonaro00

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-shuttle
cargo clean between 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

jonaro00 · 2023-10-01T00:26:21Z

cargo-shuttle/src/lib.rs

+/// It's adapted from the `gitoxide` test suite: https://github.com/
+/// Byron/gitoxide/blob/3d60c0245ec4b787cfcb111319d730a6e5031ef4/
+/// gix-status/tests/status/index_as_worktree.rs#L259


I would personally not linebreak URLs :D

thass0 · 2023-10-01T07:38:48Z

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.

thass0 · 2023-10-01T08:19:06Z

I just found the tracking issue on gix status while searching for documentation on how to use gix to also set the dirty if changes are staged but not committed or untracked. It says that determining changes of staged and untracked files is not yet implemented 😨.

I guess a reasonable way to go would be to remove my last commit and keep using git2 for now. This should also remove the other gix-related issues mentioned by @jonaro00 in their last comment.

jonaro00 · 2023-10-01T09:35:22Z

Oh no :(
Yeah, let's keep the changes well isolated and wait with using gix for that part.

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.

jonaro00

🥳

thass0 added 4 commits September 26, 2023 20:26

Remove cargo-generate dependency

97b3650

Remove openssl dependency through git2

9908477

Replaces network-based (e.g. clonging) features of git2 with those provided by gix. Thereby rustls can be used instead of openssl.

Remove cargo-generate-specific test configuration

8184268

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.

Merge branch 'main' into refactor/remove-cargo-generate

124efdf

jonaro00 reviewed Sep 28, 2023

View reviewed changes

cargo-shuttle/src/init.rs Outdated Show resolved Hide resolved

cargo-shuttle/src/init.rs Show resolved Hide resolved

cargo-shuttle/src/init.rs Outdated Show resolved Hide resolved

jonaro00 reviewed Sep 28, 2023

View reviewed changes

Apply review suggestions

e01ca06

* 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.

algora-pbc bot mentioned this pull request Sep 29, 2023

Refactor to lighter implementation of cargo-generate cloning #1269

Closed

algora-pbc bot added the 🙋 Bounty claim label Sep 29, 2023

thass0 force-pushed the refactor/remove-cargo-generate branch 2 times, most recently from 38253e2 to 8fd999f Compare September 29, 2023 14:12

thass0 force-pushed the refactor/remove-cargo-generate branch from 8fd999f to 3ac2dee Compare September 29, 2023 14:17

jonaro00 reviewed Sep 29, 2023

View reviewed changes

cargo-shuttle/src/init.rs Outdated Show resolved Hide resolved

cargo-shuttle/src/init.rs Show resolved Hide resolved

cargo-shuttle/src/init.rs Show resolved Hide resolved

Apply suggestions

aa9da60

jonaro00 reviewed Oct 1, 2023

View reviewed changes

thass0 force-pushed the refactor/remove-cargo-generate branch 2 times, most recently from 0056ff8 to aa9da60 Compare October 1, 2023 09:44

jonaro00 approved these changes Oct 1, 2023

View reviewed changes

orhun approved these changes Oct 2, 2023

View reviewed changes

Merge branch 'main' into refactor/remove-cargo-generate

03bcdb2

jonaro00 merged commit b18b7f3 into shuttle-hq:main Oct 2, 2023
32 of 33 checks passed

thass0 deleted the refactor/remove-cargo-generate branch October 2, 2023 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(cargo-shuttle): remove `cargo-generate` dependency #1281

refactor(cargo-shuttle): remove `cargo-generate` dependency #1281

thass0 commented Sep 28, 2023 •

edited

thass0 commented Sep 28, 2023

jonaro00 left a comment

thass0 commented Sep 28, 2023 •

edited

jonaro00 Sep 28, 2023

thass0 Sep 28, 2023

jonaro00 Sep 28, 2023

thass0 commented Sep 30, 2023 •

edited

jonaro00 commented Sep 30, 2023

thass0 commented Sep 30, 2023

jonaro00 left a comment •

edited

jonaro00 Oct 1, 2023

thass0 commented Oct 1, 2023

thass0 commented Oct 1, 2023 •

edited

jonaro00 commented Oct 1, 2023

jonaro00 left a comment

refactor(cargo-shuttle): remove cargo-generate dependency #1281

refactor(cargo-shuttle): remove cargo-generate dependency #1281

Conversation

thass0 commented Sep 28, 2023 • edited

Description of change

How has this been tested? (if applicable)

thass0 commented Sep 28, 2023

jonaro00 left a comment

Choose a reason for hiding this comment

thass0 commented Sep 28, 2023 • edited

jonaro00 Sep 28, 2023

Choose a reason for hiding this comment

thass0 Sep 28, 2023

Choose a reason for hiding this comment

jonaro00 Sep 28, 2023

Choose a reason for hiding this comment

thass0 commented Sep 30, 2023 • edited

jonaro00 commented Sep 30, 2023

thass0 commented Sep 30, 2023

jonaro00 left a comment • edited

Choose a reason for hiding this comment

jonaro00 Oct 1, 2023

Choose a reason for hiding this comment

thass0 commented Oct 1, 2023

thass0 commented Oct 1, 2023 • edited

jonaro00 commented Oct 1, 2023

jonaro00 left a comment

Choose a reason for hiding this comment

refactor(cargo-shuttle): remove `cargo-generate` dependency #1281

refactor(cargo-shuttle): remove `cargo-generate` dependency #1281

thass0 commented Sep 28, 2023 •

edited

thass0 commented Sep 28, 2023 •

edited

thass0 commented Sep 30, 2023 •

edited

jonaro00 left a comment •

edited

thass0 commented Oct 1, 2023 •

edited