-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Embed recovery. Fixes MER#1987 #27
base: master
Are you sure you want to change the base?
Conversation
2d357e6
to
347fe8d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for your contribution!
I haven't checked the changes in .sh for now
droid-hal-device-img-boot.inc
Outdated
@@ -73,6 +75,12 @@ BuildRequires: yamui | |||
BuildRequires: openssh-clients | |||
BuildRequires: openssh-server | |||
|
|||
%if 0%{?embed_recovery:1} | |||
BuildRequires: shpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your contribution of shpp is appreciated, but I'm afraid it cannot be justified in the context of HW adaptation recovery, as we are trying to keep dependencies to a minimum. In this particular use-case a sed
command should be sufficient.
droid-hal-device-img-boot.inc
Outdated
%if 0%{?embed_recovery:1} | ||
BuildRequires: shpp | ||
%else | ||
%define embed_recovery 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This renders some parts of the code unreachable, because defining something in .spec (even if equal to 0), counts as defined.
See here: https://backreference.org/2011/09/17/some-tips-on-rpm-conditional-macros/
and here: https://github.com/mer-hybris/droid-hal-device/blob/5fd1782c7f7b898fd66469d6cd43890c82219730/droid-hal-%40DEVICE%40.spec.template#L14
@@ -98,6 +106,7 @@ Requires: oneshot | |||
%description | |||
%{summary} | |||
|
|||
%if 0%{!?embed_recovery:1} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, this %if never evaluates to true, because embed_recovery is now always defined (either to 1, or to 0). This also applies to all similar statements against embed_recovery
below
droid-hal-device-img-boot.inc
Outdated
popd | ||
%{mkbootimg_cmd} hybris-boot.img | ||
|
||
%{mkbootimg_cmd} hybris-boot.img |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra space at the end of the line (makes the diff a bit unclear)
droid-hal-device-img-boot.inc
Outdated
@@ -17,6 +17,8 @@ | |||
# battery_capacity_file: Path to read current battery charge from. | |||
# battery_capacity_threshold: Battery threshold for factory reset [0, 100]. | |||
# Default 0 (no threshold). | |||
# embed_recovery: in case the recovery needs to embeded inside |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicking a typo: "needs to be embedded"
How can I default to an alternative value like in shell Scripts with ${too:-bar} ?
About the dependencies , how would you do this with just sed?
I'm not sure how to do this in a clean way.
|
|
347fe8d
to
976a6aa
Compare
Below link is the complete rewrite without sed, I have not fully tested on a running device yet, as I have some doubts about this line: sledges@58ab8a4#diff-ae5e5322c8e93e423e4ef2ac5e20cd84R101 Apologies for coming back on this only now, but github does not send emails about updated PR. We're lucky I was checking my timesheed as it's the end of the month, and found your PR again:) |
a4339d3
to
9ecb179
Compare
@sledges No problem. I fixed that line that you mean in mksfosinitrd and cleaned that feature a bit up. |
In case the recovery needs to embeded inside the regular initramfs.
droid-hal-device-img-boot.inc
Outdated
@@ -202,11 +212,13 @@ touch %{buildroot}/lib/modules/%{kernelver}/{modules.order,modules.builtin} | |||
%defattr(-,root,root,-) | |||
/boot/hybris-boot.img | |||
%defattr(644,root,root,-) | |||
/lib/modules/%{kernelver} | |||
/Li/modules/%{kernelver} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a typo?
Yes fixed.
|
The reason why iI am asking, is that
|
The reason is that such change needs to be tested on all Jolla's supported devices to avoid regressions, and in case such risk is minimal and/or this functionality is not enabled in a given kernel, the work of updating submodule for each device is still required. Since we had had a workaround for recovery, this PR was not on a critical path and thus turned into technical debt, due to other priorities. [1] https://jolla.zendesk.com/hc/en-us/articles/360021620740 |
… and even more so by releasing SailfishOS for the Xperia 10 III recently. |
Summary of what has to be done to use this PR.
Both droid-hal and droid-hal-img-boot have to be build after applying the changes mentioned above, after doing so flashing hybris_boot.img is enough to test if the PR works. |
This PR is obsolete for devices using Android 12 and up. |
Some devices like Sonys X series have no recovery partition on these devices the recovery needs to be inside the regular initramfs.
To do so we check if
skip_initramfs
hasn't been passed (read line 56) and then start the recovery.Background:
Bootloaders for Android phones pass
skip_init_ramfs
when the system should be booted normally and don't pass it when the key combination for recovery mode was used.We piggy back on this behavior by using the kernel commandline option is mostly the samen way except that we always use a init-ramfs.