| Message ID | fd17b5daf4a16b96d5f97841ec50c884dd6616ec.1641507696.git.dev@jpoiret.xyz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | General improvements to the installer | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | fail | View Laminar job |
| cbaines/issue | success | View issue |
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | fail | View Laminar job |
| cbaines/issue | success | View issue |
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | fail | View Laminar job |
| cbaines/issue | success | View issue |
Hello Josselin, > * gnu/installer/parted.scm (remove-logical-devices, > create-btrfs-file-system, create-ext4-file-system, > create-fat16-file-system, create-fat32-file-system, > create-jfs-file-system, create-ntfs-file-system, > create-xfs-file-system, create-swap-partition, luks-format-and-open, > luks-close): Use run-command-in-installer. > (with-null-output-ports): Remove. Overall the series looks really nice! This one is a bit problematic as it breaks the installer tests because the extra "External command" pages are not handled. --8<---------------cut here---------------start------------->8--- Jan 7 11:44:28 localhost conversation expecting pattern ((quote list-selection) ((quote title) "Partitioning method") ((quote multiple-choices?) #f) ((quote items) (not-encrypted encrypted _ ...))) /gnu/store/6c0dnvp7a1sym52s4yrjzm3wvbsv1666-shepherd-marionette.scm:1:1718: ERROR: 1. &pattern-not-matched: pattern: ((quote list-selection) ((quote title) "Partitioning method") ((quote multiple-choices?) #f) ((quote items) (not-encrypted encrypted _ ...))) sexp: (confirmation (title "External command") (text "The installer will run the following command:\n\"dmsetup\" \"remove_all\"\n")) Backtrace: Jan 7 11:44:28 localhost installer[193]: running form #<newt-form 184bd30> ("External command") with 1 clients 2 (primitive-load "/gnu/store/qpsq43z9rdb7hlabzzyz6p8pzxb?") In ice-9/eval.scm: 191:35 1 (_ #f) 619:8 0 (_ #(#<directory (guile-user) 7ffff3fd7c80> #<variabl?>)) ice-9/eval.scm:619:8: Throw to key `marionette-eval-failure' with args `((quote (choose-partitioning installer-socket #:encrypted? #f #:passphrase "thepassphrase" #:uefi-support? #f)))'. note: keeping build directory `/tmp/guix-build-installation.drv-0' builder for `/gnu/store/6xrbsa0psm30189rigjif17c6rvi8h9g-installation.drv' failed with exit code 1 --8<---------------cut here---------------end--------------->8--- Maybe we could only display those "External command" pages when the command fails? Another issue is that if any partitioning command fails, the installer keeps going. Maybe we should instead propose to abort the installation or restart the partitioning step? Thanks, Mathieu
Hello Mathieu, Mathieu Othacehe <othacehe@gnu.org> writes: > Maybe we could only display those "External command" pages when the > command fails? Seems like my mental checklist swiftly removed the "Update installer tests" part. I still like having every command the installer runs displayed to me, but that's personal preference I reckon. Maybe I could look into making the tests simply confirm every single confirmation page? > Another issue is that if any partitioning command fails, the installer > keeps going. Maybe we should instead propose to abort the installation > or restart the partitioning step? Right, this patchset is still missing the switch to exceptions, along with raising a condition on command error. I will post a follow-up patchset addressing these! One thing though, is that &invoke-error is not exported by (gnu build utils). I think for now using @@ would be the right solution to avoid a world rebuild.
Hello again Mathieu and Ludo, Here is a v2 that should follow the suggestions: the installer now only shows command output and status when the command fails, so that shouldn't break the installer tests. The internal mechanism to capture a command's output and error was reworked along Ludo's advice, and now uses open-pipe* instead (with a small workaround to avoid https://debbugs.gnu.org/cgi/bugreport.cgi?bug=52835). The second to last commit makes password objects opaque, so that installer dumps don't accidentally contain them in cleartext. Finally, the last commit (a big one) lets users choose whether to dump or not from the error page, and from there they can choose and edit the files (using nano) they would like to include in the dump archive. It expands upon the initial work of Mathieu in 84d0d8ad3d. For now, you can choose to include the installer backtrace, the installer result alist, and the syslog and dmesg. We could also include a more stripped down installer-log that the new logging facility produces, but I think that it should be enough for now. Things work smoothly on my end, but the installer test "gui-installed-os" seems to fail while running `guix system init`, when building linux-libre, but it seems unrelated to this patchset. Best, Josselin Josselin Poiret (18): installer: Use define instead of let at top-level. installer: Generalize logging facility. installer: Use new installer-log-line everywhere. installer: Un-export syslog syntax. installer: Keep PATH inside the install container. installer: Remove specific logging code. installer: Capture external commands output. installer: Add installer-specific run command process. installer: Use run-command-in-installer in (gnu installer parted). installer: Raise condition when mklabel fails. installer: Fix run-file-textbox-page when edit-button is #f. installer: Replace run-command by invoke in newt/page.scm. installer: Add nano to PATH. installer: Use named prompt to abort or break installer steps. installer: Add error page when running external commands. installer: Use dynamic-wind to setup installer. installer: Turn passwords into opaque records. installer: Make dump archive creation optional and selective. gnu/installer.scm | 95 ++++++++++-------- gnu/installer/dump.scm | 67 ++++++++----- gnu/installer/final.scm | 28 +++--- gnu/installer/newt.scm | 126 +++++++++++++++++++----- gnu/installer/newt/dump.scm | 36 ------- gnu/installer/newt/ethernet.scm | 8 +- gnu/installer/newt/final.scm | 12 +-- gnu/installer/newt/keymap.scm | 8 +- gnu/installer/newt/locale.scm | 25 ++--- gnu/installer/newt/network.scm | 16 +-- gnu/installer/newt/page.scm | 163 +++++++++++++++++++++++++++++-- gnu/installer/newt/partition.scm | 10 +- gnu/installer/newt/services.scm | 16 +-- gnu/installer/newt/timezone.scm | 4 +- gnu/installer/newt/user.scm | 11 +-- gnu/installer/newt/welcome.scm | 2 +- gnu/installer/newt/wifi.scm | 4 +- gnu/installer/parted.scm | 104 +++++++++----------- gnu/installer/record.scm | 12 ++- gnu/installer/steps.scm | 127 +++++++++++------------- gnu/installer/user.scm | 18 +++- gnu/installer/utils.scm | 158 +++++++++++++++++++++++++----- gnu/local.mk | 1 - 23 files changed, 656 insertions(+), 395 deletions(-) delete mode 100644 gnu/installer/newt/dump.scm
diff --git a/gnu/installer/parted.scm b/gnu/installer/parted.scm index ced7a757d7..c8bb73ee64 100644 --- a/gnu/installer/parted.scm +++ b/gnu/installer/parted.scm @@ -343,8 +343,7 @@ (define* (force-device-sync device) (define (remove-logical-devices) "Remove all active logical devices." - (with-null-output-ports - (invoke "dmsetup" "remove_all"))) + ((run-command-in-installer) "dmsetup" "remove_all")) (define (installer-root-partition-path) "Return the root partition path, or #f if it could not be detected." @@ -1115,53 +1114,37 @@ (define (set-user-partitions-file-name user-partitions) (file-name file-name)))) user-partitions)) -(define-syntax-rule (with-null-output-ports exp ...) - "Evaluate EXP with both the output port and the error port pointing to the -bit bucket." - (with-output-to-port (%make-void-port "w") - (lambda () - (with-error-to-port (%make-void-port "w") - (lambda () exp ...))))) - (define (create-btrfs-file-system partition) "Create a btrfs file-system for PARTITION file-name." - (with-null-output-ports - (invoke "mkfs.btrfs" "-f" partition))) + ((run-command-in-installer) "mkfs.btrfs" "-f" partition)) (define (create-ext4-file-system partition) "Create an ext4 file-system for PARTITION file-name." - (with-null-output-ports - (invoke "mkfs.ext4" "-F" partition))) + ((run-command-in-installer) "mkfs.ext4" "-F" partition)) (define (create-fat16-file-system partition) "Create a fat16 file-system for PARTITION file-name." - (with-null-output-ports - (invoke "mkfs.fat" "-F16" partition))) + ((run-command-in-installer) "mkfs.fat" "-F16" partition)) (define (create-fat32-file-system partition) "Create a fat32 file-system for PARTITION file-name." - (with-null-output-ports - (invoke "mkfs.fat" "-F32" partition))) + ((run-command-in-installer) "mkfs.fat" "-F32" partition)) (define (create-jfs-file-system partition) "Create a JFS file-system for PARTITION file-name." - (with-null-output-ports - (invoke "jfs_mkfs" "-f" partition))) + ((run-command-in-installer) "jfs_mkfs" "-f" partition)) (define (create-ntfs-file-system partition) "Create a JFS file-system for PARTITION file-name." - (with-null-output-ports - (invoke "mkfs.ntfs" "-F" "-f" partition))) + ((run-command-in-installer) "mkfs.ntfs" "-F" "-f" partition)) (define (create-xfs-file-system partition) "Create an XFS file-system for PARTITION file-name." - (with-null-output-ports - (invoke "mkfs.xfs" "-f" partition))) + ((run-command-in-installer) "mkfs.xfs" "-f" partition)) (define (create-swap-partition partition) "Set up swap area on PARTITION file-name." - (with-null-output-ports - (invoke "mkswap" "-f" partition))) + ((run-command-in-installer) "mkswap" "-f" partition)) (define (call-with-luks-key-file password proc) "Write PASSWORD in a temporary file and pass it to PROC as argument." @@ -1190,15 +1173,16 @@ (define (luks-format-and-open user-partition) (lambda (key-file) (installer-log-line "formatting and opening LUKS entry ~s at ~s" label file-name) - (system* "cryptsetup" "-q" "luksFormat" file-name key-file) - (system* "cryptsetup" "open" "--type" "luks" - "--key-file" key-file file-name label))))) + ((run-command-in-installer) "cryptsetup" "-q" "luksFormat" + file-name key-file) + ((run-command-in-installer) "cryptsetup" "open" "--type" "luks" + "--key-file" key-file file-name label))))) (define (luks-close user-partition) "Close the encrypted partition pointed by USER-PARTITION." (let ((label (user-partition-crypt-label user-partition))) (installer-log-line "closing LUKS entry ~s" label) - (system* "cryptsetup" "close" label))) + ((run-command-in-installer) "cryptsetup" "close" label))) (define (format-user-partitions user-partitions) "Format the <user-partition> records in USER-PARTITIONS list with