From aae7d7dd0afc0b86c10e1552292f56f550a0974d Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Mon, 4 Nov 2019 11:05:18 +0100 Subject: [PATCH 1/6] [grubcfg] Update documentation of config file - add some more general description - document new-to-implement *keepDistributor* flag SEE #1201 --- src/modules/grubcfg/grubcfg.conf | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/modules/grubcfg/grubcfg.conf b/src/modules/grubcfg/grubcfg.conf index b354ec35a..ba31d6070 100644 --- a/src/modules/grubcfg/grubcfg.conf +++ b/src/modules/grubcfg/grubcfg.conf @@ -1,9 +1,12 @@ +# Create, overwrite or update /etc/default/grub in the target system. +# # Write lines to /etc/default/grub (in the target system) based # on calculated values and the values set in the *defaults* key # in this configuration file. # # Calculated values are: -# - GRUB_DISTRIBUTOR, branding module, *bootloaderEntryName* +# - GRUB_DISTRIBUTOR, branding module, *bootloaderEntryName* (this +# string is sanitized, and see also setting *keepDistributor*) # - GRUB_ENABLE_CRYPTODISK, based on the presence of filesystems # that use LUKS # - GRUB_CMDLINE_LINUX_DEFAULT, adding LUKS setup and plymouth @@ -14,6 +17,12 @@ # already existed. If set to false, edits the existing file instead. overwrite: false +# If set to true, an **existing** setting for GRUB_DISTRIBUTOR is +# kept, not updated to the *bootloaderEntryName* from the branding file. +# Use this if the GRUB_DISTRIBUTOR setting in the file is "smart" in +# some way (e.g. uses shell-command substitution). +keepDistributor: false + # Default entries to write to /etc/default/grub if it does not exist yet or if # we are overwriting it. # From ac3b50fabba63ca9c29d645aba88790798f66b94 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Mon, 4 Nov 2019 16:02:36 +0100 Subject: [PATCH 2/6] [grubcfg] Only replace a GRUB_DISTRIBUTOR line if wanted --- src/modules/grubcfg/main.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/modules/grubcfg/main.py b/src/modules/grubcfg/main.py index 83441a736..d09f15fed 100644 --- a/src/modules/grubcfg/main.py +++ b/src/modules/grubcfg/main.py @@ -45,7 +45,10 @@ def modify_grub_default(partitions, root_mount_point, distributor): :param partitions: :param root_mount_point: - :param distributor: + :param distributor: name of the distributor to fill in for + GRUB_DISTRIBUTOR. Must be a string, but if it is empty ("") + then the existing GRUB_DISTRIBUTOR lines are kept instead + of replaced by a new line. :return: """ default_dir = os.path.join(root_mount_point, "etc/default") @@ -172,7 +175,8 @@ def modify_grub_default(partitions, root_mount_point, distributor): have_kernel_cmd = True elif (lines[i].startswith("#GRUB_DISTRIBUTOR") or lines[i].startswith("GRUB_DISTRIBUTOR")): - lines[i] = distributor_line + if distributor: + lines[i] = distributor_line have_distributor_line = True else: lines = [] @@ -236,6 +240,10 @@ def run(): root_mount_point = libcalamares.globalstorage.value("rootMountPoint") branding = libcalamares.globalstorage.value("branding") - distributor = branding["bootloaderEntryName"] + + if libcalamares.job.configuration.get("keepDistributor", false): + distributor = "" + else: + distributor = branding["bootloaderEntryName"] return modify_grub_default(partitions, root_mount_point, distributor) From c6c861654dfe4ae73e96ae64e7545c274234a38b Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Mon, 4 Nov 2019 16:06:59 +0100 Subject: [PATCH 3/6] [grubcfg] Update GRUB_DISTRIBUTION as needed - Previous fix would erase the distribution information (using an empty string to flag 'preserve existing GRUB_DISTRIBUTION lines'), but that is fragile. A distro might set that, and yet **not** set a GRUB_DISTRIBUTION line, in which case it would end up with a setup without any GRUB_DISTRIBUTION set. - When a GRUB_DISTRIBUTION line is found, **then** check if it should update the line or not. This way, we have a suitable distribution to write if no GRUB_DISTRIBUTION is found at all. --- src/modules/grubcfg/main.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/modules/grubcfg/main.py b/src/modules/grubcfg/main.py index d09f15fed..866cd4815 100644 --- a/src/modules/grubcfg/main.py +++ b/src/modules/grubcfg/main.py @@ -46,9 +46,12 @@ def modify_grub_default(partitions, root_mount_point, distributor): :param partitions: :param root_mount_point: :param distributor: name of the distributor to fill in for - GRUB_DISTRIBUTOR. Must be a string, but if it is empty ("") - then the existing GRUB_DISTRIBUTOR lines are kept instead - of replaced by a new line. + GRUB_DISTRIBUTOR. Must be a string. If the job setting + *keepDistributor* is set, then this is only used if no + GRUB_DISTRIBUTOR is found at all (otherwise, when *keepDistributor* + is set, the GRUB_DISTRIBUTOR lines are left unchanged). + If *keepDistributor* is unset or false, then GRUB_DISTRIBUTOR + is always updated to set this value. :return: """ default_dir = os.path.join(root_mount_point, "etc/default") @@ -175,7 +178,7 @@ def modify_grub_default(partitions, root_mount_point, distributor): have_kernel_cmd = True elif (lines[i].startswith("#GRUB_DISTRIBUTOR") or lines[i].startswith("GRUB_DISTRIBUTOR")): - if distributor: + if libcalamares.job.configuration.get("keepDistributor", false): lines[i] = distributor_line have_distributor_line = True else: @@ -240,10 +243,6 @@ def run(): root_mount_point = libcalamares.globalstorage.value("rootMountPoint") branding = libcalamares.globalstorage.value("branding") - - if libcalamares.job.configuration.get("keepDistributor", false): - distributor = "" - else: - distributor = branding["bootloaderEntryName"] + distributor = branding["bootloaderEntryName"] return modify_grub_default(partitions, root_mount_point, distributor) From 4a0a8083f389e0aa8efbfd4b52c5b3ebf6ed3ad0 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Mon, 4 Nov 2019 16:32:14 +0100 Subject: [PATCH 4/6] [grubcfg] If we only see #GRUB_DISTRIBUTION, it's not been set - If we update the line, then GRUB_DISTRIBUTION has been set - If we don't update the line (e.g. because of *keepDistribution*) then a comment doesn't count as "have seen that line". This means that if we get to the end of the file, with only commented- out GRUB_DISTRIBUTION lines, and *keepDistribution* is set, then we'll still write a distribution line -- because otherwise it's not set at all. --- src/modules/grubcfg/main.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/modules/grubcfg/main.py b/src/modules/grubcfg/main.py index 866cd4815..79679790c 100644 --- a/src/modules/grubcfg/main.py +++ b/src/modules/grubcfg/main.py @@ -180,7 +180,11 @@ def modify_grub_default(partitions, root_mount_point, distributor): or lines[i].startswith("GRUB_DISTRIBUTOR")): if libcalamares.job.configuration.get("keepDistributor", false): lines[i] = distributor_line - have_distributor_line = True + have_distributor_line = True + else: + # We're not updating because of *keepDistributor*, but if + # this was a comment line, then it's still not been set. + have_distributor_line = not lines[i].startsdwith("#") else: lines = [] From f727362a90ca5e5e0f5eeed2481c3b4abffe3e83 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Mon, 4 Nov 2019 16:36:57 +0100 Subject: [PATCH 5/6] [grubcfg] Guard against stupid configurations - Scenario: *keepDistribution* is true, and the existing file contains a GRUB_DISTRIBUTION line **followed** by a commented-out GRUB_DISTRIBUTION line. - In that case, the commented-out line would change the flag back to False, and we'd end up writing a second GRUB_DISTRIBUTION line at the end. Prevent that: the flag can only go to "True" and then stays there. Editorial: If your grub configuration would have tripped this up, then you're doing something wrong. Clean up the configuration file first. --- src/modules/grubcfg/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/grubcfg/main.py b/src/modules/grubcfg/main.py index 79679790c..1f2574965 100644 --- a/src/modules/grubcfg/main.py +++ b/src/modules/grubcfg/main.py @@ -184,7 +184,7 @@ def modify_grub_default(partitions, root_mount_point, distributor): else: # We're not updating because of *keepDistributor*, but if # this was a comment line, then it's still not been set. - have_distributor_line = not lines[i].startsdwith("#") + have_distributor_line = have_distributor_line or not lines[i].startsdwith("#") else: lines = [] From 67de4af4a49225dccaa02a64bcfe6d60b9bb3e23 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Mon, 4 Nov 2019 16:40:49 +0100 Subject: [PATCH 6/6] [grubcfg] Add some test configurations --- src/modules/grubcfg/tests/1.global | 2 ++ src/modules/grubcfg/tests/2.global | 10 ++++++++++ src/modules/grubcfg/tests/2.job | 9 +++++++++ 3 files changed, 21 insertions(+) create mode 100644 src/modules/grubcfg/tests/1.global create mode 100644 src/modules/grubcfg/tests/2.global create mode 100644 src/modules/grubcfg/tests/2.job diff --git a/src/modules/grubcfg/tests/1.global b/src/modules/grubcfg/tests/1.global new file mode 100644 index 000000000..02ae840cb --- /dev/null +++ b/src/modules/grubcfg/tests/1.global @@ -0,0 +1,2 @@ +--- +bogus: true diff --git a/src/modules/grubcfg/tests/2.global b/src/modules/grubcfg/tests/2.global new file mode 100644 index 000000000..83e79db28 --- /dev/null +++ b/src/modules/grubcfg/tests/2.global @@ -0,0 +1,10 @@ +--- +bogus: true +firmwareType: bios +bootLoader: grub +rootMountPoint: /tmp/calamares + +branding: + bootloaderEntryName: generic +partitions: [] + diff --git a/src/modules/grubcfg/tests/2.job b/src/modules/grubcfg/tests/2.job new file mode 100644 index 000000000..d7b8db9d1 --- /dev/null +++ b/src/modules/grubcfg/tests/2.job @@ -0,0 +1,9 @@ +--- +overwrite: true +keepDistributor: false +defaults: + GRUB_TIMEOUT: 5 + GRUB_DEFAULT: "saved" + GRUB_DISABLE_SUBMENU: true + GRUB_TERMINAL_OUTPUT: "console" + GRUB_DISABLE_RECOVERY: true