search

geerlingguy / rpi-clone

A shell script to clone a booted disk on a Raspberry Pi.
https://rpi-clone.jeffgeerling.com/
BSD 3-Clause "New" or "Revised" License
172 stars 10 forks source link

Supporting Armbian - some feedback needed #21

Open matthijskooijman opened 3 months ago

matthijskooijman commented 3 months ago

I have had a number of open PRs at billfw2's repo for quite some time. Today I've moved over the simple ones (#16, #17, #18, #20) and added a new one (#19), leaving the last big one, https://github.com/billw2/rpi-clone/pull/140 which is needed to support Armbian. That PR makes two main changes:

  1. Support modifying armbianEnv.txt instead of cmdline.txt
  2. Support updating filesystem UUID's instead of just PARTUUIDs

In addition, it refactors the code to introduce some helper functions to reuse some code. In particular it uses the same code for updating fstab and cmdline.txt, and for supporting cmdline.txt and armbianEnv it just calls the same (new) function twice to try each file.

However, some changes made in this repo conflict with the changes there, in particular 63765e8e84cc978417a75bbb7e1a28746d4ca542 that adds support for /boot/firmware/cmdline.txt. Additionally, the commit order in my PR has grown over a few years, so I think it would be better to restructure the commits (first extract code into functions and only then make it more complicated).

Before I fully commit to revamping this pullrequest, I would like to get some feedback on these questions:

  1. @geerlingguy, are you interested in merging support for Armbian at all?
  2. The current support for /boot/firmware/cmdline.txt parses fstab (in a somewhat fragile way IMHO) to decide where to find cmdline.txt. My approach for this was to just try all versions (/boot/cmdline.txt, /boot/firmware/cmdline.txt and /boot/armbianEnv.txt) and upating any of these files that exist. I believe this gives cleaner code. Does that approach make sense?
  3. There is also some code to convert fstab and cmdline.txt from raw device names to use PARTUUID. This also has received support for /boot/firmware/cmdline.txt, but the structure of this conversion code does not lend itself for the "try each in turn" approach. However, it seems that raspbian started using PARTUUIDs in 2017, so I wonder if there is till value in this feature. I would be inclined to just remove the feature to simplify the code and reducing maintainance load of code that is probably unused and hard to test as well. How does that sound?
geerlingguy commented 3 months ago

Personally, I've only used this on Pis with Pi OS before, so I wouldn't want to add much complexity... as an alternative, is it possible to work on simplifying some of the bash (like extracting functions) and then add on top of that (or make a fork easier to maintain)?

framps commented 3 months ago

2. The current support for /boot/firmware/cmdline.txt parses fstab (in a somewhat fragile way IMHO)

Are you talking about this line ? Why do you think it's fragile?

3. There is also some code to convert fstab and cmdline.txt from raw device names to use PARTUUID.

I see your point. Actually this is a question whether to stay backward compatible.

  1. Armbian

I don't use Armbian and don't plan to use it. Said this I'll neither be able to review Armbian code nor to execute any tests. I think most of rpi-users use RaspbianOS. Question now is how many % of rpi-clone users use Armbian and it makes sense to support Armbian?

One general comment: I reviewed some PRs but then stopped because the review is too time consuming for me. It would be very helpful if any code updates will be enhanced with comments in PRs.

matthijskooijman commented 3 months ago

Personally, I've only used this on Pis with Pi OS before, so I wouldn't want to add much complexity... as an alternative, is it possible to work on simplifying some of the bash (like extracting functions) and then add on top of that (or make a fork easier to maintain)?

Sure, but once the extracting functions is done, then supporting armbian is a matter of one or two lines for armbianEnv.txt (and it needs FS UUID support, but that is not Armbian-specific and could be used by people running Pi OS as well). But yes, this would also be an acceptable course, if I would just need minimal changes in my fork.

To get an idea about how this might look, here's how those extracted functions look in my PR at bilwl2's repo: https://github.com/billw2/rpi-clone/pull/140/files#diff-e73eaae36cd02c1c899d7257d01e6ad2de891b767399796c10ef7458d271784cR649-R735

(You can ignore the commit history of that PR, which I would redo cleanly, and also a lot of other stuff in that PR is only partially related and I extracted to separate PRs #16 and #20)

Inside those functions, here are the two parts to support Armbian:

Are you talking about this line ? Why do you think it's fragile?

Yes. It checks on a /boot prefix on any mounted filesystem, meaning it could potentially match all kinds of other things than intended, e.g. if a user would have added /boot-foo-bar mount, that could be matched instead. Also, parsing fstab with awk in general seems fragile in general. Hence my suggestion of just trying both /boot/cmdline.txt and /boot/firmware/cmdline.txt, which is also not perfect, but unlikely to cause unexpected issues I think. However, if the current approach is maintained, I would suggest maybe parsing /proc/mounts or mount output (assuming those are reliably documented and compatible, or maybe /sys also exposes this info somewhere) and looking specificically for either /boot or /boot/firmware in there, or maybe looking for the first partition on the src device (since that is what the bootloader looks for I guess), ignoring the filesystem path entirely.

Question now is how many % of rpi-clone users use Armbian and it makes sense to support Armbian?

Currently probably not much, since it needed my unmerged PR to work, but I suspect there might be interest in this (especially since Armbian now also supports the rpi 3+).

One general comment: I reviewed some PRs but then stopped because the review is too time consuming for me. It would be very helpful if any code updates will be enhanced with comments in PRs.

Thanks for the reviews so far, they are very welcome, sorry to hear it is too time consuming for you. As for comments in PR - what do you mean exactly? I've tried to add sufficient comments in the PR description, but mostly in the individual commit messages. Are you missing detail there?

framps commented 2 months ago

if a user would have added /boot-foo-bar mount

Ok. You're right. The regex doesn't handle this rare condition.

However, if the current approach is maintained, I would suggest maybe parsing /proc/mounts or mount

I'll create a PR which will use mount | awk '$3 ~ "^/boot(/firmware)?$" { print $3 }' and not make the current fstab awk more robust. That way the actual mounts are used instead of the declared mounts.

As for comments in PR - what do you mean exactly?

Your descriptions in your PRs are very valuable but there's still some detailed understanding of the code required in order to review the PR carefully. I don't have the full picture of rpi-clone and don't want to invest too much time to understand the details. That's why I reviewed your PRs where I was able to understand your code updates only.

I've tried to add sufficient comments in the PR description, but mostly in the individual commit messages.

These comments are present at the time the PR was worked on. But then they are lost. If you place these comments in the code they will be there forever :wink: JM2C

matthijskooijman commented 2 months ago

I'll create a PR which will use mount | awk '$3 ~ "^/boot(/firmware)?$" { print $3 }' and not make the current fstab awk more robust. That way the actual mounts are used instead of the declared mounts.

Your PR makes the current approach cleaner, so I'll make sure to keep that approach in my PR when I create it. In practice, I think this means checking for the existence of /etc/armbian-release and if it exists use /boot/armbianEnv.txt instead of cmdline.txt. Does that seem reasonable?

These comments are present at the time the PR was worked on. But then they are lost. If you place these comments in the code they will be there forever 😉 JM2C

Commit messages stay part of git history and can be found by e.g. git blame later (I do that often when looking through codebases and finding bugs). But that is of course less accessible than comments directly in the code, which are more helpful when just reading and trying to understand. There is of course a balance - too many comments for things that are obvious just hinder readability (especially if the comments go stale), but what's obvious for one is not obvious for someone else. Maybe I am erring on the side of too little comments - I'll keep your point in mind for future PRs.

matthijskooijman commented 2 months ago

These comments are present at the time the PR was worked on. But then they are lost. If you place these comments in the code they will be there forever 😉 JM2C

I had another look over my PR's, and actually think they mostly contain the right amount of comments, at least for the newly introduced code (in some cases I've modified existing code which is a bit harder to understand, but adding comments there did not seem in scope of these PRs). One exception is #20, which contained a hard-to-read mostly duplicate list of rsync options, which I've now refactored to be easier to read and added a comment. If you have more specific questions or things you'd like to see clarified, feel free to add comments to the respective PR :-)