dw-0 / kiauh

Klipper Installation And Update Helper
GNU General Public License v3.0
3.3k stars 474 forks source link

refactor(Klipper): trim klipper git clone to only latest history #418

Open nberardi opened 9 months ago

nberardi commented 9 months ago

I was receiving the following error on my Raspberry Pi Zero 2 W during the installation of Klipper, while running the 64-bit lite version of Raspberry Pi OS (bookworm).2

error: RPC failed; curl 92 HTTP/2 stream 0 was not closed cleanly: PROTOCOL_ERROR (err 1)

I tracked this down to a likely low memory issue due to the size of the git repo. I was able to correct for this issue, by only downloading the tip of the history of the Klipper repo during install by setting --depth 1 in the git clone command. This resulted in a roughly 13x reduction in the size of the downloaded repo and allowed me to continue my install without issue using KIAUH.

dw-0 commented 9 months ago

I think this might be a good change. I tested it myself and there is roughly a saving of 140mb we don't need to download, just 10mb instead.

Though, there is another feature of KIAUH which needs to be refactored a little bit if we implement that change. There is a "rollback" feature, where its possible to enter a number and klipper gets "rolled back" (basically a hard reset) by that amount of commits. https://github.com/dw-0/kiauh/blob/master/scripts/rollback.sh#L71

If we have "no history" or a very short one, and for some reason someone want to rollback a larger amount of commits than available in the history, git will throw an error. so it may require refactoring that workflow, so that the size of the history is checked before the hard reset is executed.

ElectricalBoy commented 8 months ago

Doing a shallow clone is more expensive than doing a full clone in the long term (ref: https://github.com/CocoaPods/CocoaPods/issues/4989#issuecomment-193772935, https://github.com/Homebrew/brew/pull/9383).

Sure - Kilpper repo will (almost certainly) never get as much traffic as the two repos linked above (nor will its size grow as big), but my point still stands. Doing a full clone initially becomes cheaper in the long run (especially if you run updates).

nberardi commented 8 months ago

@ElectricalBoy more expensive than not being able to pull the repo on my Raspberry Pi Zero 2W? 😀

There are other options that could have been tried, but this was the cleanest code change that allowed me to keep moving on my install.

ElectricalBoy commented 8 months ago

I see the change of the default behavior to introduce a long-term side-effect as a regression.

Yes, I do acknowledge that doing a shallow clone does act as a short-term solution for this specific problem, especially on lower-end devices (i.e., RPi Zeros). However, this comes at a cost of introducing a regression on the majority of use cases (i.e., normal RPis). This, I believe, shows that the cleanest code is not the cleanest solution for a problem.

What I think should happen instead is keeping the default behavior to do a full clone but adding an option to do a shallow clone (which can possibly be set in the advanced menu).

dw-0 commented 8 months ago

Doing a shallow clone is more expensive than doing a full clone in the long term

I understand it that way, that it would be more expensive on githubs side than on the client side?

I see the change of the default behavior to introduce a long-term side-effect as a regression. [...] However, this comes at a cost of introducing a regression on the majority of use cases

Can you elaborate? What would be an actual regression?

dw-0 commented 8 months ago

@ElectricalBoy any more input on this? Especially in regards to my previous questions?

ElectricalBoy commented 5 months ago

I understand it that way, that it would be more expensive on githubs side than on the client side?

If the user needs to checkout an old commit (that is not available via the shallow tree) then fetching the giant tree eventually becomes inevitable - and this connects to my old regression argument, as this introduces the new overhead when rolling back.

Although I am mildly annoyed by the regression, considering the # of times a rollback is needed in general this seems like a reasonable compromise after giving some more thought. Plus it seems that Git handles this better in general than how it did few years back.

P.s.: Sorry for getting back so late - had other commitments that needed my attention.

Sineos commented 2 months ago

Since I ran into the issue as well: There are two ways to work around this issue: