Merge ~ubuntu-core-dev/grub/+git/ubuntu:sil2100/use-auto-nvram into ~ubuntu-core-dev/grub/+git/ubuntu:ubuntu

Proposed by Łukasz Zemczak
Status: Rejected
Rejected by: Łukasz Zemczak
Proposed branch: ~ubuntu-core-dev/grub/+git/ubuntu:sil2100/use-auto-nvram
Merge into: ~ubuntu-core-dev/grub/+git/ubuntu:ubuntu
Diff against target: 25 lines (+2/-1)
2 files modified
debian/changelog (+1/-0)
debian/postinst.in (+1/-1)
Reviewer Review Type Date Requested Status
Steve Langasek Disapprove
Review via email: mp+345340@code.launchpad.net

Commit message

debian/postinst.in: use --auto-nvram with grub-install calls.

Description of the change

debian/postinst.in: use --auto-nvram with grub-install calls.

To post a comment you must log in.
Revision history for this message
Steve Langasek (vorlon) wrote :

This doesn't look correct to me. The debian/postinst.in is used to generate the postinst of (among others) grub-efi-amd64 and grub-pc. In the EFI+BIOS coinstallability case, I would expect to see grub-pc installed, but not grub-efi-amd64: we would only install shim-signed + grub-efi-amd64-bin (+signed). Therefore, the one version of the script that actually *cares* about --auto-nvram is the one we would normally not expect to have installed.

Also see the $NO_NVRAM variable elsewhere in the code, which is specifically set only for EFI packages and passed as an argument to run_grub_install. If any changes should be made to this script to support --auto-nvram (which I think is unnecessary), they should be made there.

review: Disapprove
Revision history for this message
Łukasz Zemczak (sil2100) wrote :

Agreed, maybe I rushed with this MP a bit. It doesn't seem to be needed here. Let's drop it.

As for $NO_NVRAM, I agree it might be slightly cleaner to do it that way - but since --auto-nvram respects --no-nvram, I decided to introduce a one-line-diff instead of a multi-line one having to modify $NO_NVRAM (or introducing something like $AUTO_NVRAM). It seemed like useless copy-pasting of code. When --no-nvram is given along with --auto-nvram, no magic is done and simply no NVRAM operations are being done.

Unmerged commits

79813c8... by Łukasz Zemczak

Use --auto-nvram with grub-install calls

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/changelog b/debian/changelog
index 07c31df..b0d1d6e 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -3,6 +3,7 @@ grub2 (2.02-2ubuntu9) UNRELEASED; urgency=medium
3 * debian/patches/add-an-auto-nvram-option-to-grub-install.patch: Add the3 * debian/patches/add-an-auto-nvram-option-to-grub-install.patch: Add the
4 --auto-nvram option to grub-install for auto-detecting NVRAM availability4 --auto-nvram option to grub-install for auto-detecting NVRAM availability
5 before attempting NVRAM updates.5 before attempting NVRAM updates.
6 * debian/postinst.in: use --auto-nvram with grub-install calls.
67
7 -- Łukasz 'sil2100' Zemczak <lukasz.zemczak@ubuntu.com> Fri, 20 Apr 2018 19:41:26 +02008 -- Łukasz 'sil2100' Zemczak <lukasz.zemczak@ubuntu.com> Fri, 20 Apr 2018 19:41:26 +0200
89
diff --git a/debian/postinst.in b/debian/postinst.in
index b1435d3..a6b4fb1 100644
--- a/debian/postinst.in
+++ b/debian/postinst.in
@@ -308,7 +308,7 @@ no_nvram_arg() {
308308
309run_grub_install()309run_grub_install()
310{310{
311 if ! grub-install $@ ; then311 if ! grub-install --auto-nvram $@ ; then
312 echo "Failed: grub-install $@" >&2312 echo "Failed: grub-install $@" >&2
313 echo "WARNING: Bootloader is not properly installed, system may not be bootable" >&2313 echo "WARNING: Bootloader is not properly installed, system may not be bootable" >&2
314 fi314 fi

Subscribers

People subscribed via source and target branches