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

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)
> ---
>  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 :)
> 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.

Diederik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL:
<http://alioth-lists.debian.net/pipermail/pkg-xen-devel/attachments/20201202/f75d30ea/attachment.sig>

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