Skip to content
This repository has been archived by the owner. It is now read-only.

Add support for isorec flashing in recovery installer #609

Merged
merged 12 commits into from Sep 28, 2017

Conversation

@ata2001
Copy link
Member

@ata2001 ata2001 commented Sep 21, 2017

No description provided.

ata2001 added 4 commits Sep 17, 2017
Fix
@ata2001 ata2001 requested a review from drebrez Sep 21, 2017
@ata2001
Copy link
Member Author

@ata2001 ata2001 commented Sep 22, 2017

Please test if you have a device with isorec (only at your own risk, read the diff beforehand)!

ata2001 added 2 commits Sep 23, 2017
@ata2001 ata2001 force-pushed the feature/recovery-installer-isorec branch from cc07c6a to 09bcd0b Sep 23, 2017
ata2001 added 3 commits Sep 23, 2017
Copy link
Member

@drebrez drebrez left a comment

Tested on samsung-i9070 and everything works! Good job 👍

@ollieparanoid is it ok for you to have the pmb/flasher/variables.py script with the common function?

@ata2001 you should probably add the copyright note in the pmb/flasher/variables.py file

@ollieparanoid
Copy link
Member

@ollieparanoid ollieparanoid commented Sep 25, 2017

@drebrez: I did not make a code review yet (will try to do that tomorrow), so I can't say what's my opinion on that. But in general I'm highly in favor of splitting code up in small files and functions where it makes sense.

Copy link
Member

@ollieparanoid ollieparanoid left a comment

Code looks good to me! I agree with the missing copyright, and proposed a refactoring at one point.

(I don't really like vars["$..."] (the $ character!), but then I realized that your PR didn't introduce this and I wrote it that way myself :p - so that if we change that, it should be done in a new PR in my opinion.)

Thanks a lot for making this PR @ata2001!

@@ -42,8 +45,17 @@ def create_zip(args, suffix):
"w") as install_options:
install_options.write(
"\n".join(['DEVICE="{}"'.format(args.device),
'FLASH_BOOTIMG="{}"'.format(
str(args.recovery_flash_bootimg).lower()),
'FLAVOR="{}"'.format(flavor),

This comment has been minimized.

@ollieparanoid

ollieparanoid Sep 28, 2017
Member

Proposal to make this part a bit more readable (not tested, probably PEP8 errors!):

--- a/pmb/install/recovery.py
+++ b/pmb/install/recovery.py
@@ -41,31 +41,30 @@ def create_zip(args, suffix):
     logging.info("(" + suffix + ") create recovery zip")

     # Create config file for the recovery installer
-    with open(args.work + "/chroot_" + suffix + "/tmp/install_options",
-              "w") as install_options:
-        install_options.write(
-            "\n".join(['DEVICE="{}"'.format(args.device),
-                       'FLAVOR="{}"'.format(flavor),
-                       'FLASH_KERNEL="{}"'.format(
-                           str(args.recovery_flash_kernel).lower()),
-                       'ISOREC="{}"'.format(str(method == "heimdall-isorec")
-                                            .lower()),
-                       'KERNEL_PARTLABEL="{}"'.format(
-                           vars["$PARTITION_KERNEL"]),
-                       'INITFS_PARTLABEL="{}"'.format(
-                           vars["$PARTITION_INITFS"]),
-                       'SYSTEM_PARTLABEL="{}"'.format(
-                           vars["$PARTITION_SYSTEM"]),
-                       'INSTALL_PARTITION="{}"'.format(
-                           args.recovery_install_partition),
-                       'CIPHER="{}"'.format(args.cipher),
-                       'FDE="{}"'.format(
-                           str(args.full_disk_encryption).lower())]))
+    options = {
+        "DEVICE": args.device,
+        "FLAVOR": flavor,
+        "FLASH_KERNEL": args.recovery_flash_kernel,
+        "ISOREC": method == "heimdall-isorec",
+        "KERNEL_PARTLABEL": vars["$PARTITION_KERNEL"],
+        "INITFS_PARTLABEL": vars["$PARTITION_INITFS"],
+        "SYSTEM_PARTLABEL": vars["$PARTITION_SYSTEM"],
+        "INSTALL_PARTITION": args.recovery_install_partition,
+        "CIPHER": args.cipher,
+        "FDE": args.full_disk_encryption,
+    }

+    # Write to a temporary file
+    config_temp = args.work + "/chroot_" + suffix + "/tmp/install_options"
+    with open(config_temp, "w") as handle:
+        for key, value in options.keys():
+            if isinstance(value, bool):
+                value = str(value).lower()
+            handle.write(key + "='" + value + "'\n")
      commands = [
@ata2001 ata2001 force-pushed the feature/recovery-installer-isorec branch from 186f85d to a8f8528 Sep 28, 2017
@ata2001
Copy link
Member Author

@ata2001 ata2001 commented Sep 28, 2017

@ollieparanoid Applied your patch, only has to change options.keys() to options.items().

@ata2001 ata2001 force-pushed the feature/recovery-installer-isorec branch from 2def279 to b8d936f Sep 28, 2017
@ata2001
Copy link
Member Author

@ata2001 ata2001 commented Sep 28, 2017

@ollieparanoid added license header

@ollieparanoid ollieparanoid merged commit 3cbd0d1 into master Sep 28, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ollieparanoid ollieparanoid deleted the feature/recovery-installer-isorec branch Sep 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.