#1849 PR closed: added mods for slackware support - mkbackup working with UEFI/USB

Labels: enhancement, won't fix / can't fix / obsolete

wdmsde opened issue at 2018-07-02 16:33:

Pull Request Details:
  • Type: Enhancement

  • Impact: Normal

  • Reference to related issue (URL):
    http://lists.relax-and-recover.org/pipermail/rear-users/2018-July/003572.html

  • How was this pull request tested?
    Tested on laptop running Slackware 14.2 with UEFI. Used "rear -v mkbackup" to USB drive. The USB drive booted OK but I did not test recover function yet. The backup tar was available on the drive.

  • Brief description of the changes in this pull request:
    Added code to support Slackware 14.2 distribution. This was a brute force approach and my mktemp changes may break compatibility with other OSs. Requesting feedback to improve quality.

jsmeix commented at 2018-07-03 11:34:

@gozora
I requested your review here because it is about UEFI.

jsmeix commented at 2018-07-03 11:41:

@schlomo
I requested your review here because currently it shows

Merging is blocked
The target branch requires all commits to be signed.

but this is a pull request from an external contributor.

I would like to know how we should handle pull requests from external contributors.

I would prefer when external contributors are not forced to sign their pull requests.

I would prefer when it is sufficient that pull requests from external contributors
are reviewed by us and if the changes are o.k. for us that then we can merge it.

Is that possible or would that contradict our improved security in
https://github.com/rear/rear/issues/1419

wdmsde commented at 2018-07-03 12:47:

Hi Johannes,

I like your suggestions and will implement the cleaner one. I used the
process of muntzing to reduce the grub modules and would like you to
consider that the eliminated modules were not required and maybe should be
left out? Let me know your preference.

On Tue, Jul 3, 2018 at 7:21 AM, Johannes Meixner notifications@github.com
wrote:

@jsmeix commented on this pull request.

In usr/share/rear/output/USB/Linux-i386/100_create_efiboot.sh
https://github.com/rear/rear/pull/1849#discussion_r199770821:

@@ -88,7 +89,8 @@ EOF

             # Create bootloader, this overwrite BOOTX64.efi copied in previous step ...
             # Fail if BOOTX64.efi can't be created
  • ${GRUB_MKIMAGE} -o ${EFI_DST}/BOOTX64.efi -p ${EFI_DIR} -O x86_64-efi linux part_gpt ext2 normal gfxterm gfxterm_background gfxterm_menu test all_video loadenv fat
  • Slackware grub doesnt have gfxterm_background or gfxterm_menu...

  • ${GRUB_MKIMAGE} -o ${EFI_DST}/BOOTX64.efi -p ${EFI_DIR} -O x86_64-efi linux part_gpt ext2 normal gfxterm gfxmenu test all_video loadenv fat

I think here a test for Slackware and conditional code execution is needed
like you did it in your usr/share/rear/pack/Linux-i386/300_copy_kernel.sh

add slackware test on top to prevent error because

slackware grub doesnt have gfxterm_background or gfxterm_menu...

if [ -f /etc/slackware-version ] ; then
${GRUB_MKIMAGE} -o ${EFI_DST}/BOOTX64.efi -p ${EFI_DIR} -O x86_64-efi linux part_gpt ext2 normal gfxterm gfxmenu test all_video loadenv fat
else
${GRUB_MKIMAGE} -o ${EFI_DST}/BOOTX64.efi -p ${EFI_DIR} -O x86_64-efi linux part_gpt ext2 normal gfxterm gfxterm_background gfxterm_menu test all_video loadenv fat
fi

or perhaps even cleaner by using a menaingful variable for the GRUB
modules like

grub_modules="x86_64-efi linux part_gpt ext2 normal gfxterm gfxmenu test all_video loadenv fat"

slackware grub doesnt have gfxterm_background or gfxterm_menu

test -f /etc/slackware-version || grub_modules="$grub_modules gfxterm_background gfxterm_menu"

Create bootloader, this overwrite BOOTX64.efi copied in previous step ...

Fail if BOOTX64.efi can't be created

${GRUB_MKIMAGE} -o ${EFI_DST}/BOOTX64.efi -p ${EFI_DIR} -O $grub_modules


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rear/rear/pull/1849#pullrequestreview-133952829, or mute
the thread
https://github.com/notifications/unsubscribe-auth/Am1cd7p2DIACS0WfwCnBc0P9FsDGprtlks5uC1OhgaJpZM4U_nDx
.

wdmsde commented at 2018-07-03 12:59:

In the 300_copy_kernel.sh I will be happy to use the OS_MASTER_VERSION in
my test. I thought I was following convention by using
"/etc/slackware-version" since the following elif tests all use file
references: /etc/redhat-release, /etc/debian_version,
/etc/arch-release,... Please let me know if you want this test to be the
only one using OS_MASTER_VERSION.

Thanks for considering my pull request.

On Tue, Jul 3, 2018 at 7:32 AM, Johannes Meixner notifications@github.com
wrote:

@jsmeix commented on this pull request.

In usr/share/rear/pack/Linux-i386/300_copy_kernel.sh
https://github.com/rear/rear/pull/1849#discussion_r199773522:

@@ -10,7 +10,13 @@

Using another kernel is a TODO for now

if [ ! -s "$KERNEL_FILE" ]; then

  • if [ -r "/boot/vmlinuz-$KERNEL_VERSION" ]; then
  • add slackware test on top to prevent error on get_kernel_version ==================================================================

  • if [ -f /etc/slackware-version ]; then

The test for Slackware should be done via the usually used variables
OS_MASTER_VENDOR OS_VENDOR OS_VENDOR_VERSION and so on,
see the SetOSVendorAndVersion function in usr/share/rear/lib/config-
functions.sh
how those variables are set.

Perhaps those variables are already set right for Slackware by that
function
perhaps you need to add support to get them set right for Slackware to
that function.
To find out to what values those variables are set in your particular case
run rear dump for example in my case on SLES15 I get (excerpts):

usr/sbin/rear -D dump

...
This is a 'Linux-x86_64' system, compatible with 'Linux-i386'.
System definition:
ARCH = Linux-i386
OS = GNU/Linux
OS_MASTER_VENDOR = SUSE
OS_MASTER_VERSION = 15
OS_MASTER_VENDOR_ARCH = SUSE/i386
OS_MASTER_VENDOR_VERSION = SUSE/15
OS_MASTER_VENDOR_VERSION_ARCH = SUSE/15/i386
OS_VENDOR = SUSE_LINUX
OS_VERSION = 15
OS_VENDOR_ARCH = SUSE_LINUX/i386
OS_VENDOR_VERSION = SUSE_LINUX/15
OS_VENDOR_VERSION_ARCH = SUSE_LINUX/15/i386

For example how those variables are used see the
get_part_device_name_format function
in usr/share/rear/lib/layout-functions.sh that contains (excerpt):

        case $OS_MASTER_VENDOR in
            (SUSE)
                ...
            ;;
            (Fedora)
                ...
            ;;
            (Debian)
                ...
            ;;
            (*)
                ....
            ;;
        esac


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rear/rear/pull/1849#pullrequestreview-133956105, or mute
the thread
https://github.com/notifications/unsubscribe-auth/Am1cd9FlwUziKaBCOhvGKFiPygOOLpeCks5uC1ZrgaJpZM4U_nDx
.

gozora commented at 2018-07-03 14:54:

Hello @wdmsde,

I used the
process of muntzing to reduce the grub modules and would like you to
consider that the eliminated modules were not required and maybe should be
left out?

I'd prefer not to messing up with existing code and keep modules as they are, at lest for non-Slackware distributions. We did not had issues with non-booting ReaR recovery system because "this and that" for a quite a long time, and I really like it that way ;-).

V.

gozora commented at 2018-07-03 15:01:

@schlomo @jsmeix

requested your review here because currently it shows
Merging is blocked
The target branch requires all commits to be signed.

Interestingly enough, when I have this PR open on my phone, I have "Merge pull request" button active.... :-)

V.

jsmeix commented at 2018-07-04 07:45:

@schlomo @gdha
I assigned this pull request to you because I cannot merge it,
cf. https://github.com/rear/rear/pull/1849#issuecomment-402125330
and https://github.com/rear/rear/pull/1850#issuecomment-402392845

jsmeix commented at 2018-07-04 08:12:

@wdmsde
regarding your
https://github.com/rear/rear/pull/1849#issuecomment-402148149

You are right.
For this pull request just follow the existing convention in the current code.

Unfortunately sometimes the ReaR code is such a mess
that has grown over the time where things need to be cleaned up.
We try to do as much as we can as time permits but as long as things work
there is no pressure to clean up code that works - "never change ..." ;-)
Right now I did https://github.com/rear/rear/issues/1851
to clean up our various 300_copy_kernel.sh scripts as time permits in the future.

wdmsde commented at 2018-07-05 16:33:

@jsmeix
I upgraded my grub version to 2.02 on Slackware and was able to reduce my
patch size. I submitted another pull request that incorporates your
suggestions.

On Wed, Jul 4, 2018 at 4:12 AM, Johannes Meixner notifications@github.com
wrote:

@wdmsde https://github.com/wdmsde
regarding your
#1849 (comment)
https://github.com/rear/rear/pull/1849#issuecomment-402148149

You are right.
For this pull request just follow the existing convention in the current
code.

Unfortunately sometimes the ReaR code is such a mess
that has grown over the time where things needs to be cleaned up.
We try to do as much as we can as time permits but as long as things work
there is no pressure to clean up code that works - "never change ..." ;-)
Right now I did #1851 https://github.com/rear/rear/issues/1851
to clean up our various 300_copy_kernel.sh scripts as time permits in the
future.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rear/rear/pull/1849#issuecomment-402400509, or mute
the thread
https://github.com/notifications/unsubscribe-auth/Am1cd_hEKQgomurWeMxLNMaGeiJ19r92ks5uDHkLgaJpZM4U_nDx
.

jsmeix commented at 2018-07-09 09:13:

Superseded by https://github.com/rear/rear/pull/1853


[Export of Github issue for rear/rear.]