Closed Undef-a closed 3 years ago
Thanks for the review @ollieparanoid. Plenty of good points there.
Currently this has a bug which causes the external to internal installer to be invoked when not configured.
cannot reproduce. If
targetDeviceRootInternal
is not inmobile.conf
, the feature will be skipped (seeskipFeatureInstallTarget()
). I just tried it out and it got skipped as expected:
That comment was made as a not for my on testing. It seemed to happen a few times when I was recently trying to flash my phone. If it's not reproducible, its probably related to the semi-finished Mobian packaging I was using.
Looks good to me! :+1: Thanks again for all the work you've put into this, @Undef-a!
@adriaandegroot: can you please take a look at this?
If you think it's ready to merge, it would be great to merge #10 as well and tag a new release.
Lots of comments, but the decision is down to you: if it works for your use-case and gives you the UI you want, then it's fine.
Thanks a lot for the thorough review! I think these review points should be addressed before we merge it.
Use a model for string lists, rather than one for selecting locales; even better is probably writing a model (it would go in libcalamares/partition/FileSystem.h most likely) that shows a handful of filesystem names / types.
Getting the file system selection right seems like the most complex part... @adriaandegroot, can you write a draft of how the addition to FileSystem.h should look like?
I think all of the above are now resolved, thanks for the through review. I need to build/flash a test image of this on an actual phone, but pending that I believe this is ready for another review.
So, I'm going to try to set up CI for calamares-extensions first, then merge this, then apply coding-style and minor tweaks to the code. The comments I have just made in the review are "this would be a neat extra feature" kind of things, and trivia like coding-style. Overall it looks pretty solid and warrants a new release of calamares-extensions.
@Undef-a CI is now set up, just give me (or @ollieparanoid , who can then ping me on IRC) a sign if you think it can be merged; I don't want to do that if you're still working on this branch because it would mess up your workflow.
After that squash and last config change I'm pretty happy with it, and I can work on the polish you brought up in another branch. Just writing an image to test it on a real device now.
I'll yell out in #calamares when that is done.
My testing is triggering the internal from external installer again. This is using the mobian packaging, which currently does not have any logic for this function.
It also happens on the stock mobile.conf from this PR.
Until I can work out why this is happening, this probably should not be merged.
@Undef-a: weird, I'll ping you in IRC so we can work this out.
Successfully tested on device. Lucky I did as there were two blocking bugs:
@ollieparanoid Thanks for the help with these. Could you please review the fix for the external -> internal issue. I believe that when the option is not triggered, this was defaulting to "true" on Mobian, resulting in the error. I have initialized this variable to fix it, which solved the issue, but I can't confirm that it won't cause you issues.
EDIT: Specifically referring to L159 of Config.h
The user can now configure the target root filesystem type. Options provided are ext4 (default), f2fs and btrfs.
OS Maintainers are required to provide their own BTRFS install script which configures default submodules as desired.