#3103 PR closed: Delete usr/share/rear/build/USB/default/800_enforce_usb_output.sh

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

jsmeix opened issue at 2023-12-11 08:29:

Remove
build/USB/default/800_enforce_usb_output.sh
see
https://github.com/rear/rear/issues/1571#issuecomment-343461088
and
https://github.com/rear/rear/issues/1571#issuecomment-343516020

Furthermore this script reads

# Added by udev workflow

but it does its stuff in any case for USB
regardless of the workflow
so this script looks utterly broken.

According to

git log --follow -p usr/share/rear/build/USB/default/800_enforce_usb_output.sh

this script originated (as 80_enforce_usb_output.sh) in
https://github.com/rear/rear/commit/4ec9ed4aa58787f42ad2946d68358d1de3417a60
which only tells

Make sure OUTPUT=USB is enforced in udev workflow during recover

but it neither explains WHY that is needed
nor does it implement what it tells
because the script is run in any case for OUTPUT=USB
regardless whether or not the udev workflow is used.

So as far as I understand it:
This script is run in any case for OUTPUT=USB
and then (i.e. when there is OUTPUT=USB in local.conf)
it "enforces" OUTPUT=USB in ROOTFS_DIR/.../local.conf
but how could OUTPUT in ROOTFS_DIR/.../local.conf
differ from OUTPUT in local.conf?

pcahyna commented at 2023-12-12 15:06:

@jsmeix this is to be used by the udev workflow, triggered from etc/udev/rules.d/62-rear-usb.rules . Shouldn't the whole udev workflow be removed, if we can't test it?

pcahyna commented at 2023-12-12 15:13:

the udev rules in rpm spec have been disabled in 24d57b876f00ca3e54ea3168e36a4e3513dc5564

jsmeix commented at 2023-12-12 15:47:

FYI
regarding the udev workflow have a look at
(what I "just found" right now by searching
some of my older archived mails)
for example things like

https://github.com/rear/rear/issues/840
"udev workflow cannot work as it is currently implemented"

https://github.com/rear/rear/issues/838
"USB gets suspended"

https://github.com/rear/rear/issues/2180
"during boot 'rear udev' is called, causing delay during boot"

From my personal point of view
the udev workflow and all what belongs to it
should be removed the sooner the better, cf.
https://github.com/rear/rear/issues/2180#issuecomment-511700761

pcahyna commented at 2023-12-12 16:17:

RHEL has never been using the udev workflow (the udev rules have not been shipped in our RPM).

jsmeix commented at 2023-12-13 13:14:

Also SUSE has never supported the udev workflow.

In particular there is no '62-rear-usb.rules' file shipped

  • neither in a SUSE "rear..." RPM package for SLE-HA, cf.
    "... rear116 / rear1172a / rear118a / rear23a / rear27a"
    in https://en.opensuse.org/SDB:Disaster_Recovery
  • nor in an "official" openSUSE "rear" RPM package
    (i.e. what osc search rear | grep '^openSUSE:' lists)

jsmeix commented at 2023-12-14 08:04:

@rear/contributors in particular @schlomo @gdha
please also have a look here (as time permits).

If you do not object I would like to merge it
next Monday (18 Dec. 2023) afternoon.

jsmeix commented at 2023-12-15 14:39:

@schlomo
thank you for having a look here!

I agree that it would be better to first inform the user.

But here I don't see how I could properly inform the user.
I mean if I "just called" Error() in that script
it would error out in any case because that script
runs unconditioned.

I also don't know under which conditions that script
should be run because I do not understand for what reason
it is there at all.
I mean that I do not yet understand what it is that made you
"understood that we run $BACKUP and $OUTPUT and
even $OUTPUT/$BACKUP files in every stage".

It is neither about that $BACKUP and $OUTPUT and
even $OUTPUT/$BACKUP scripts are run in every stage
nor about "multiple $OUTPUT methods".

It is about WHY build/USB/default/800_enforce_usb_output.sh
exists at all because - as I explained above - I don't see
any reason why it needs to do what it does because
I don't see how OUTPUT in ROOTFS_DIR/.../local.conf
could differ from OUTPUT in local.conf ?
I.e. as far as I see OUTPUT is same in both.

schlomo commented at 2023-12-18 10:08:

@jsmeix I was referring to SourceStage():
https://github.com/rear/rear/blob/e0609c751b260f959f9caa82056ecfad0c1840cb/usr/share/rear/lib/framework-functions.sh#L110-L114

Fun fact, this script build/USB/default/800_enforce_usb_output.sh would be included for both BACKUP=USB and for OUTPUT=USB.

I did some more thinking about this issue and I would like to propose a different approach:

Insert an Error at line 8 or so of the script to error out with a deprecation warning and a link to this issue and a request to provide more context for the use case.

It would hit only if this script would do something.

I know, breaking stuff is bad but it seems to me that this is the only way how we can force feedback from users of this code.

If, OTOH, everybody else wants to just remove it, then please go ahead. Worst case, somebody will come and complain why we broke ReaR and why it took them 3 days to figure out why.

jsmeix commented at 2023-12-18 11:05:

BACKUP=USB is not supported and
not listed in "man rear" or in default.conf
and for a test with BACKUP=USB I get

# usr/sbin/rear -D mkbackup
...
ERROR: The BACKUP method 'USB' is not supported (no /root/rear.github.master/usr/share/rear/restore/USB directory)
Some latest log messages since the last called script 035_valid_backup_methods.sh:

so in practice build/USB/default/800_enforce_usb_output.sh
will not be run for BACKUP=USB
so it should be only run for OUTPUT=USB

jsmeix commented at 2023-12-18 11:28:

This one is superseded by
https://github.com/rear/rear/pull/3110

[Export of Github issue for rear/rear.]