[Pkg-xen-devel] [PATCH 1/9] Rework "debian/rules: Do not try to move EFI binaries on armhf"

[Elliott Mitchell]

On Wed, Dec 02, 2020 at 01:43:04PM +0100, Diederik de Haas wrote:
> On vrijdag 17 juli 2020 07:39:45 CET Elliott Mitchell wrote:
> > This reverts commit 8ff478af61fa8a87806a89bbd618cd9da2354302.
> 
> In that commit Ian made an explicit exemption for armhf. Unfortunately the 
> commit doesn't explain why the exemption was made, which is a problem imo.
> But if you're going to revert a commit, you (also) need to have a reason why 
> you're reverting it. 
> "Reason for commit X no longer applies because of Y, thus revert commit X"
> 
> Can you explain why it needs to be reverted? (and make that part of your 
> commit message)

Next revision will replace that with "rework".

> > ---
> >  debian/rules | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/debian/rules b/debian/rules
> > index b21c9e6948..29a561b99f 100755
> > --- a/debian/rules
> > +++ b/debian/rules
> > -		*) mv $t/usr/lib64/efi/* $t/boot/. ;; \
> > +	find "$t/usr/lib64/efi" -mindepth 1 -maxdepth 1 -print0 | xargs -r -0
> > mv -t "$t/boot"
> 
> I like that you're quoting the arguments, shellcheck would approve :)

Many people actually dislike it, thinking it is overkill.  I tend towards
doing more, since in theory scripts should work even in directories with
odd names (got a 16TB file named "* ext4 limit" in your home directory?).

> > a move command which fails if the move fails, but succeeds if no files are
> > moved
> 
> That's the stated reason for why you changed a 'mv' with 'find'+'xargs'+'mv'. 
> 'find' and 'xargs' are part of the 'findutils' package, which should then also 
> be added to the Build-Depends.
> What I don't know is *why* the simple 'mv' would/could fail.
> When that's because the directory doesn't exist, couldn't that also be fixed by 
> "if [ -d "$t/usr/lib64/efi/ ] ; then" ?
> I think that would also be more efficient.

It is a quote of Ian Jackson:
https://alioth-lists.debian.net/pipermail/pkg-xen-devel/2020-September/008187.html

There are 3 actual cases:
1>  EFI/ACPI are disabled; move should not occur
    result => SUCCESS
2>  EFI/ACPI are enabled; move occurs successfully
    result => SUCCESS
3>  EFI/ACPI are enabled; move fails (filesystem full?)
    result => FAIL

What was behind 8ff478af61fa8a87806a89bbd618cd9da2354302 is EFI/ACPI are
presently disabled on armhf by default.  What I think Ian was getting at
is someone (Intel?) appears to be pushing EFI/ACPI very heavily and is
trying to make it required on ARM.  I was initially approaching this as
adding hard-coding armhf/arm64 in, but due to Ian's message I switched
to the construct in the patch.

The alternative approach would be to check for CONFIG_ACPI in the build
configuration.

Anyway, I've got an update, but I've been dragged into responding to
messages instead of sending the replacement series.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg at m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445